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.

Sign up for GitHub

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

Jump to bottom

WIP: Fix for https://lab.civicrm.org/dev/core/-/issues/5095 #29736

Closed
wants to merge 1 commit into from
Closed

WIP: Fix for https://lab.civicrm.org/dev/core/-/issues/5095 #29736

wants to merge 1 commit into from

Conversation

twomice
Copy link
Contributor

@twomice twomice commented Mar 15, 2024

Overview

Reference original issue: https://lab.civicrm.org/dev/core/-/issues/5095 :
"CRM_Report_Form_Activity: links to target / assigned contacts are often incorrect"

Before

Links often point to wrong contact.

After

Links point to the correct contact(s).
Sorting of names within each column is maintained, based on contact.sort_name.

Technical Details

Sure, it would be nice to do as the code comments in this file suggest (in several places):

      // @todo - fix up the way the tables are declared in construct & remove this.

But that seems more effort that it's worth at this point.

@civibot civibot
Copy link

civibot bot commented Mar 15, 2024

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Mar 15, 2024
@eileenmcnaughton
Copy link
Contributor

@twomice looks like you have picked a fight with jenkins https://test.civicrm.org/job/CiviCRM-Core-Matrix-PR/9893/BKPROF=dfl,SUITES=phpunit-api3,label=bknix-tmp/

@twomice
Copy link
Contributor Author

twomice commented Mar 15, 2024

@eileenmcnaughton I believe Jenkins is picking a fight with me. I'm up for it, though. Thanks for the heads-up!

@eileenmcnaughton
Copy link
Contributor

sting like a butterfly dance like a bee!

@twomice twomice force-pushed the 5095_CRM_Report_Form_Activity branch from 82401b8 to 5f3109b Compare March 18, 2024 21:43
@twomice twomice changed the title Fix for https://lab.civicrm.org/dev/core/-/issues/5095 WIP: Fix for https://lab.civicrm.org/dev/core/-/issues/5095 Mar 19, 2024
@twomice
Copy link
Contributor Author

twomice commented Mar 19, 2024

Marking as "WIP" pending passing tests.

@colemanw
Copy link
Member

@civicrm-builder retest this please

@eileenmcnaughton
Copy link
Contributor

this is the test fail

CRM_Report_Form_ActivityTest::testTargetAddressFields
Exception: Civi\Core\Exception\DBQueryException: "DB Error: no such field"

  • ERROR TYPE: DB_Error

  • ERROR CODE: -19

  • ERROR MESSAGE: DB Error: no such field

  • ERROR MODE: 16

  • ERROR USERINFO: SELECT SQL_CALC_FOUND_ROWS GROUP_CONCAT(DISTINCT civicrm_contact_contact_source_id ORDER BY civicrm_contact_contact_source, civicrm_contact_contact_assignee, civicrm_contact_contact_target SEPARATOR ';') as civicrm_contact_contact_source_id, GROUP_CONCAT(DISTINCT civicrm_contact_contact_assignee_id ORDER BY civicrm_contact_contact_source, civicrm_contact_contact_assignee, civicrm_contact_contact_target SEPARATOR ';') as civicrm_contact_contact_assignee_id, GROUP_CONCAT(DISTINCT civicrm_contact_contact_target_id ORDER BY civicrm_contact_contact_source, civicrm_contact_contact_assignee, civicrm_contact_contact_target SEPARATOR ';') as civicrm_contact_contact_target_id, GROUP_CONCAT(DISTINCT civicrm_contact_contact_source_employer_id ORDER BY civicrm_contact_contact_source, civicrm_contact_contact_assignee, civicrm_contact_contact_target SEPARATOR ';') as civicrm_contact_contact_source_employer_id, GROUP_CONCAT(DISTINCT civicrm_contact_contact_assignee_employer_id ORDER BY civicrm_contact_contact_source, civicrm_contact_contact_assignee, civicrm_contact_contact_target SEPARATOR ';') as civicrm_contact_contact_assignee_employer_id, GROUP_CONCAT(DISTINCT civicrm_contact_contact_target_employer_id ORDER BY civicrm_contact_contact_source, civicrm_contact_contact_assignee, civicrm_contact_contact_target SEPARATOR ';') as civicrm_contact_contact_target_employer_id, civicrm_activity_id, civicrm_activity_source_record_id, civicrm_activity_activity_type_id, civicrm_activity_activity_date_time, GROUP_CONCAT(DISTINCT civicrm_address_city ORDER BY civicrm_contact_contact_source, civicrm_contact_contact_assignee, civicrm_contact_contact_target SEPARATOR ';') as civicrm_address_city, GROUP_CONCAT(DISTINCT civicrm_address_country_id ORDER BY civicrm_contact_contact_source, civicrm_contact_contact_assignee, civicrm_contact_contact_target SEPARATOR ';') as civicrm_address_country_id
    FROM civicrm_tmp_e_dflt_b80f68717da6169a20a6ec31734a612e tar
    INNER JOIN civicrm_activity activity_civireport ON activity_civireport.id = tar.civicrm_activity_id
    INNER JOIN civicrm_activity_contact activity_contact_civireport ON activity_contact_civireport.activity_id = activity_civireport.id
    AND activity_contact_civireport.record_type_id = 2
    LEFT JOIN civicrm_contact contact_civireport ON contact_civireport.id = activity_contact_civireport.contact_id

    WHERE (1) AND ((`activity_civireport`.`activity_type_id` IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 17, 19, 22, 34, 35, 36, 37, 38, 39, 40, 41, 44, 45, 46, 47, 48, 49, 50, 51, 52, 54, 56)))   GROUP BY civicrm_activity_id, civicrm_activity_source_record_id, civicrm_activity_activity_type_id, civicrm_activity_activity_date_time  ORDER BY civicrm_activity_activity_date_time ASC, field(civicrm_activity_activity_type_id, 19, 37, 35, 36, 48, 52, 51, 6, 49, 3, 50, 5, 41, 54, 12, 45, 34, 1, 8, 17, 7, 4, 46, 2, 10, 11, 22, 47, 40, 44, 9, 56, 39, 38) ASC  LIMIT 0, 50 [nativecode=1054 ** Unknown column 'civicrm_contact_contact_target' in 'field list']

  • ERROR DEBUGINFO: SELECT SQL_CALC_FOUND_ROWS GROUP_CONCAT(DISTINCT civicrm_contact_contact_source_id ORDER BY civicrm_contact_contact_source, civicrm_contact_contact_assignee, civicrm_contact_contact_target SEPARATOR ';') as civicrm_contact_contact_source_id, GROUP_CONCAT(DISTINCT civicrm_contact_contact_assignee_id ORDER BY civicrm_contact_contact_source, civicrm_contact_contact_assignee, civicrm_contact_contact_target SEPARATOR ';') as civicrm_contact_contact_assignee_id, GROUP_CONCAT(DISTINCT civicrm_contact_contact_target_id ORDER BY civicrm_contact_contact_source, civicrm_contact_contact_assignee, civicrm_contact_contact_target SEPARATOR ';') as civicrm_contact_contact_target_id, GROUP_CONCAT(DISTINCT civicrm_contact_contact_source_employer_id ORDER BY civicrm_contact_contact_source, civicrm_contact_contact_assignee, civicrm_contact_contact_target SEPARATOR ';') as civicrm_contact_contact_source_employer_id, GROUP_CONCAT(DISTINCT civicrm_contact_contact_assignee_employer_id ORDER BY civicrm_contact_contact_source, civicrm_contact_contact_assignee, civicrm_contact_contact_target SEPARATOR ';') as civicrm_contact_contact_assignee_employer_id, GROUP_CONCAT(DISTINCT civicrm_contact_contact_target_employer_id ORDER BY civicrm_contact_contact_source, civicrm_contact_contact_assignee, civicrm_contact_contact_target SEPARATOR ';') as civicrm_contact_contact_target_employer_id, civicrm_activity_id, civicrm_activity_source_record_id, civicrm_activity_activity_type_id, civicrm_activity_activity_date_time, GROUP_CONCAT(DISTINCT civicrm_address_city ORDER BY civicrm_contact_contact_source, civicrm_contact_contact_assignee, civicrm_contact_contact_target SEPARATOR ';') as civicrm_address_city, GROUP_CONCAT(DISTINCT civicrm_address_country_id ORDER BY civicrm_contact_contact_source, civicrm_contact_contact_assignee, civicrm_contact_contact_target SEPARATOR ';') as civicrm_address_country_id
    FROM civicrm_tmp_e_dflt_b80f68717da6169a20a6ec31734a612e tar
    INNER JOIN civicrm_activity activity_civireport ON activity_civireport.id = tar.civicrm_activity_id
    INNER JOIN civicrm_activity_contact activity_contact_civireport ON activity_contact_civireport.activity_id = activity_civireport.id
    AND activity_contact_civireport.record_type_id = 2
    LEFT JOIN civicrm_contact contact_civireport ON contact_civireport.id = activity_contact_civireport.contact_id

    WHERE (1) AND ((`activity_civireport`.`activity_type_id` IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 17, 19, 22, 34, 35, 36, 37, 38, 39, 40, 41, 44, 45, 46, 47, 48, 49, 50, 51, 52, 54, 56)))   GROUP BY civicrm_activity_id, civicrm_activity_source_record_id, civicrm_activity_activity_type_id, civicrm_activity_activity_date_time  ORDER BY civicrm_activity_activity_date_time ASC, field(civicrm_activity_activity_type_id, 19, 37, 35, 36, 48, 52, 51, 6, 49, 3, 50, 5, 41, 54, 12, 45, 34, 1, 8, 17, 7, 4, 46, 2, 10, 11, 22, 47, 40, 44, 9, 56, 39, 38) ASC  LIMIT 0, 50 [nativecode=1054 ** Unknown column 'civicrm_contact_contact_target' in 'field list']

#0 internal function: CRM_Core_Error::exceptionHandler(Object(DB_Error))
#1 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/vendor/pear/pear-core-minimal/src/PEAR.php(945): call_user_func((Array:2), Object(DB_Error))
#2 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/vendor/pear/db/DB.php(1001): PEAR_Error->__construct("DB Error: no such field", -19, 16, (Array:2), "SELECT SQL_CALC_FOUND_ROWS GROUP_CONCAT(DISTINCT civicrm_contact_contact_sour...")
#3 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/vendor/pear/pear-core-minimal/src/PEAR.php(575): DB_Error->__construct(-19, 16, (Array:2), "SELECT SQL_CALC_FOUND_ROWS GROUP_CONCAT(DISTINCT civicrm_contact_contact_sour...")
#4 internal function: PEAR::_raiseError(Object(DB_mysqli), NULL, -19, 16, (Array:2), "SELECT SQL_CALC_FOUND_ROWS GROUP_CONCAT(DISTINCT civicrm_contact_contact_sour...", "DB_Error", TRUE)
#5 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/vendor/pear/pear-core-minimal/src/PEAR.php(221): call_user_func_array((Array:2), (Array:8))
#6 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/vendor/pear/db/DB/common.php(1951): PEAR->__call("raiseError", (Array:7))
#7 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/vendor/pear/db/DB/mysqli.php(964): DB_common->raiseError(-19, NULL, NULL, "SELECT SQL_CALC_FOUND_ROWS GROUP_CONCAT(DISTINCT civicrm_contact_contact_sour...", "1054 ** Unknown column 'civicrm_contact_contact_target' in 'field list'")
#8 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/vendor/pear/db/DB/mysqli.php(413): DB_mysqli->mysqliRaiseError()
#9 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/vendor/pear/db/DB/common.php(1241): DB_mysqli->simpleQuery("SELECT SQL_CALC_FOUND_ROWS GROUP_CONCAT(DISTINCT civicrm_contact_contact_sour...")
#10 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/packages/DB/DataObject.php(2696): DB_common->query("SELECT SQL_CALC_FOUND_ROWS GROUP_CONCAT(DISTINCT civicrm_contact_contact_sour...")
#11 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/packages/DB/DataObject.php(1829): DB_DataObject->_query("SELECT SQL_CALC_FOUND_ROWS GROUP_CONCAT(DISTINCT civicrm_contact_contact_sour...")
#12 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/CRM/Core/DAO.php(519): DB_DataObject->query("SELECT SQL_CALC_FOUND_ROWS GROUP_CONCAT(DISTINCT civicrm_contact_contact_sour...")
#13 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/CRM/Core/DAO.php(1750): CRM_Core_DAO->query("SELECT SQL_CALC_FOUND_ROWS GROUP_CONCAT(DISTINCT civicrm_contact_contact_sour...", TRUE)
#14 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/CRM/Report/Form.php(3221): CRM_Core_DAO::executeQuery("SELECT SQL_CALC_FOUND_ROWS GROUP_CONCAT(DISTINCT civicrm_contact_contact_sour...")
#15 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/CRM/Report/Form.php(3648): CRM_Report_Form->buildRows("SELECT SQL_CALC_FOUND_ROWS GROUP_CONCAT(DISTINCT civicrm_contact_contact_sour...", (Array:0))
#16 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/CRM/Report/Form.php(959): CRM_Report_Form->postProcess()
#17 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/CRM/Core/Form.php(753): CRM_Report_Form->preProcess()
#18 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviReportTestCase.php(82): CRM_Core_Form->buildForm()
#19 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/tests/phpunit/CRM/Report/Form/ActivityTest.php(130): CiviReportTestCase->getReportObject("CRM_Report_Form_Activity", (Array:2))
#20 phar:///home/homer/buildkit/extern/phpunit9/phpunit9.phar/phpunit/Framework/TestCase.php(1315): CRM_Report_Form_ActivityTest->testTargetAddressFields()
#21 /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php(289): PHPUnit\Framework\TestCase->runTest()
#22 phar:///home/homer/buildkit/extern/phpunit9/phpunit9.phar/phpunit/Framework/TestCase.php(981): CiviUnitTestCase->runTest()
#23 phar:///home/homer/buildkit/extern/phpunit9/phpunit9.phar/phpunit/Framework/TestResult.php(588): PHPUnit\Framework\TestCase->runBare()
#24 phar:///home/homer/buildkit/extern/phpunit9/phpunit9.phar/phpunit/Framework/TestCase.php(772): PHPUnit\Framework\TestResult->run(Object(CRM_Report_Form_ActivityTest))
#25 phar:///home/homer/buildkit/extern/phpunit9/phpunit9.phar/phpunit/Framework/TestSuite.php(510): PHPUnit\Framework\TestCase->run(Object(PHPUnit\Framework\TestResult))
#26 phar:///home/homer/buildkit/extern/phpunit9/phpunit9.phar/phpunit/Framework/TestSuite.php(510): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#27 phar:///home/homer/buildkit/extern/phpunit9/phpunit9.phar/phpunit/TextUI/TestRunner.php(479): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#28 phar:///home/homer/buildkit/extern/phpunit9/phpunit9.phar/phpunit/TextUI/Command.php(125): PHPUnit\TextUI\TestRunner->run(Object(PHPUnit\Framework\TestSuite), (Array:74), (Array:0), TRUE)
#29 phar:///home/homer/buildkit/extern/phpunit9/phpunit9.phar/phpunit/TextUI/Command.php(94): PHPUnit\TextUI\Command->run((Array:8), TRUE)
#30 /home/homer/buildkit/extern/phpunit9/phpunit9.phar(2307): PHPUnit\TextUI\Command::main()
#31 {main}

/home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:293
/home/homer/buildkit/extern/phpunit9/phpunit9.phar:2307

@eileenmcnaughton
Copy link
Contributor

my guess is the added order-by fields are not always present - source_contact_id is required but the others are not

@eileenmcnaughton
Copy link
Contributor

@twomice are you still up for this fight - or should we close it & track via gitlab?

@twomice
Copy link
Contributor Author

twomice commented Jun 24, 2024

@eileenmcnaughton I won't get to it this week. Gotta catch up on client work after the sprint. Feel free to close if you think best.

@eileenmcnaughton
Copy link
Contributor

@twomice yeah - lets close out & you can see how you feel - it's a tough one & you would be forgiven for letting Jenkins have this battle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@twomice @eileenmcnaughton @colemanw