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

Fix testMultipleMembershipsContribution to use order api #22274

Merged
merged 1 commit into from
Jan 7, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

This test was not creating valid financial records prior to this

@civibot
Copy link

civibot bot commented Dec 19, 2021

(Standard links)

@eileenmcnaughton
Copy link
Contributor Author

@colemanw can I get a merge on this?

@eileenmcnaughton
Copy link
Contributor Author

@colemanw @seamuslee001 @demeritcowboy this should be a straight forward merge - unlike the stalled one it does not have a date change in it

$this->assertEquals($result1['contact_id'], $contactId1);
$this->assertEquals($result1['status_id'], $newMembershipId);
$this->assertEquals($contactId1, $memberships[0]['contact_id']);
$this->assertEquals('test status', CRM_Core_PseudoConstant::getName('CRM_Member_BAO_Membership', 'status_id', $memberships[0]['status_id'], 'New'));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this PR is ok but the 'New' was confusing me, but it seems getName() only takes 3 params so the New does nothing. Was it meant to check that the membership is in a New state?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. Can you fix this @eileenmcnaughton and then it can be MOP.

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 @demeritcowboy fixed - thanks!

This test was not creating valid financial records prior to this
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.

3 participants