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

Group Contact create -> fix to use post hook for subscription records #22419

Merged
merged 3 commits into from
Mar 19, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jan 8, 2022

Overview

Update GroupContact method to create Subscription records in the post hook

Before

Done in the create method

After

Done in a post hook

Technical Details

Some complexities in getting the tests to pass mean this was left out when @colemanw did the others - should be passing now

Comments

@civibot
Copy link

civibot bot commented Jan 8, 2022

(Standard links)

@civibot civibot bot added the master label Jan 8, 2022
}
$params = $event->object->toArray();
try {
SubscriptionHistory::save(FALSE)->setRecords([$params])->execute();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colemanw I was worried about whether the subscription is created so I added test cover for it - in the process I added the SubscriptionHistory entity so as part of that I figured I might as well call that from the hook here - however, it writes and then gets rolled back - I'm a bit mystified as to why

@eileenmcnaughton eileenmcnaughton force-pushed the gc branch 2 times, most recently from da0d300 to c0ec644 Compare January 8, 2022 19:14
@eileenmcnaughton
Copy link
Contributor Author

Test fail is Stacktrace
api_v3_GroupContactTest::testDeleteWithPending with data set "APIv4" (4)
Failed asserting that actual size 0 matches expected size 1.

/home/jenkins/bknix-dfl/build/core-22419-55dv3/web/sites/all/modules/civicrm/tests/phpunit/api/v3/GroupContactTest.php:237
/home/jenkins/bknix-dfl/build/core-22419-55dv3/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:262
/home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar:671

per comment above - I can fix by not calling the api & just calling writeRecords - but I'd like to understand why - since I've seen unexplained api rollbacks before & never gotten to the bottom

@colemanw
Copy link
Member

colemanw commented Jan 9, 2022

@eileenmcnaughton we can start by adding the entity #22435

@eileenmcnaughton
Copy link
Contributor Author

thanks @colemanw -regarding unexpected rollbacks (as replicated in the unit test here) - that seems to be an active discussion on chat too -

@colemanw
Copy link
Member

Where on chat @eileenmcnaughton ?

@eileenmcnaughton
Copy link
Contributor Author

@colemanw there is some concern about rollbacks in IATS - I haven't dug in at all though - but in this case - the record is being written to the table in the post hook but NOT being committed -

@eileenmcnaughton eileenmcnaughton force-pushed the gc branch 4 times, most recently from d3db4da to 3dbfaac Compare March 17, 2022 22:23
@eileenmcnaughton eileenmcnaughton changed the title Group Contact testing Group Contact create -> fix to use post hook for subscription records Mar 17, 2022
@eileenmcnaughton
Copy link
Contributor Author

@colemanw this is passing now ....

/**
*
* @package CRM
* @copyright CiviCRM LLC https://civicrm.org/licensing
*/
class CRM_Contact_BAO_GroupContact extends CRM_Contact_DAO_GroupContact {
class CRM_Contact_BAO_GroupContact extends CRM_Contact_DAO_GroupContact implements \Civi\Test\HookInterface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class CRM_Contact_BAO_GroupContact extends CRM_Contact_DAO_GroupContact implements \Civi\Test\HookInterface {
class CRM_Contact_BAO_GroupContact extends CRM_Contact_DAO_GroupContact implements \Civi\Core\HookInterface {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eileenmcnaughton this was renamed since Jan

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colemanw OK - fixed

@colemanw colemanw merged commit eb9d3c8 into civicrm:master Mar 19, 2022
@colemanw colemanw deleted the gc branch March 19, 2022 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants