Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes report#85 - Don't crash Contact Logging Detail report when viewing a contribution #22242

Merged
merged 1 commit into from
Feb 28, 2022

Conversation

MegaphoneJon
Copy link
Contributor

@MegaphoneJon MegaphoneJon commented Dec 13, 2021

https://lab.civicrm.org/dev/report/-/issues/85

Attempting to view the details of a contribution on the Change Log tab fails if the contact was updated in the same database connection.

Steps to replicate

  • Turn on advanced logging.
  • Record a donation through a contribution page for either a) a new contact, b) an existing contact, but adding/changing their name.
  • Go to that contact's change log, and try to view the details of the most recent change (by clicking Update next to the change).

Before

DB Error: Syntax Error.

After

You see the changes.

This regression comes from PR 19504. Previously, not every DAO had a _labelField property - so CRM_Logging_ReportDetail::convertForeignKeyValuesToLabels() checking if the property existed was basically the same as checking if it was defined. Now it will always exist (in CRM_Core_DAO) but not always be defined. So we change the conditional to reflect that.

@civibot
Copy link

civibot bot commented Dec 13, 2021

(Standard links)

@demeritcowboy
Copy link
Contributor

demeritcowboy commented Dec 13, 2021

So oddly this was the test error I got locally before the patch, but then for me it passed after the patch. So hmm...

CRM_Report_Form_Contact_LoggingDetailTest::testLabelFieldIsntRequired
Undefined variable $original

/home/jenkins/bknix-dfl/build/core-22242-3uj0o/web/sites/all/modules/civicrm/CRM/Logging/Differ.php:217
/home/jenkins/bknix-dfl/build/core-22242-3uj0o/web/sites/all/modules/civicrm/CRM/Logging/Differ.php:442

Then there's also another error which might just be fallout from the first one because logging might still be enabled from before:

api_v3_LoggingTest::testEnableDisableLogging
Failed asserting that 0 matches expected 1.

/home/jenkins/bknix-dfl/build/core-22242-3uj0o/web/sites/all/modules/civicrm/tests/phpunit/api/v3/LoggingTest.php:63

@demeritcowboy
Copy link
Contributor

This one is being stubborn:

api_v3_LoggingTest::testEnableDisableLogging
Failed asserting that 0 matches expected 1.

/home/jenkins/bknix-dfl/build/core-22242-3uj0o/web/sites/all/modules/civicrm/tests/phpunit/api/v3/LoggingTest.php:63

I don't offhand know what the problem is, but my guess is something to do with the fact that disabling logging by itself doesn't delete the log tables (which is correct). So if civi::settings gets cleared between tests, that logging_unique_XX setting doesn't get reset even when logging is re-enabled since it doesn't recreate the log tables?

@MegaphoneJon MegaphoneJon changed the title fixes reporting#85 - Don't crash Contact Logging Detail report when viewing a contribution fixes report#85 - Don't crash Contact Logging Detail report when viewing a contribution Feb 28, 2022
@demeritcowboy demeritcowboy merged commit 7cd7784 into civicrm:master Feb 28, 2022
@demeritcowboy
Copy link
Contributor

Thanks @MegaphoneJon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants