Make domain_id optional in membership_type api. #12461
Conversation
|
(Standard links)
|
| $this->assertEquals($membershipType['error_message'], 'Mandatory key(s) missing from params array: domain_id'); | ||
| $membershipType = $this->callAPISuccess('membership_type', 'create', $params); | ||
| $domainID = $this->callAPISuccessGetValue('MembershipType', ['return' =>'domain_id', 'id' => $membershipType['id']]); | ||
| $this->assertEquals(CRM_Core_Config::domainID(), $domainID); |
There was a problem hiding this comment.
@eileenmcnaughton I feel like we need a test where x membership type has been created with domain y but now we are on domain z and do a create call and make sure that the type stays as domain y
There was a problem hiding this comment.
yeah good idea - to stop anyone else breaking later
There was a problem hiding this comment.
I've encountered an issue where a membership renewal made via iATS's scheduled job on domain Y renewing a membership on domain Z caused some havoc - I imagine IPNs would be the same. It seemed non-trivial to fix though, and it's more a misconfiguration than a bug.
There was a problem hiding this comment.
ug - that hurts my head - only tangentally related to this though?
This is in support of civicrm#12439 & making domain_id optional
5dd0058 to
fb38ab8
Compare
|
test this please |
| @@ -126,9 +126,9 @@ public function testCreate() { | |||
| } | |||
|
|
|||
| /** | |||
| * Test update fails with no ID. | |||
| * Domain ID can be intuited.. | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Ok so it's in the dictionary... :-) I know I'm being picky but it'd be nice to have a more descriptive comment for the test - how about: "Domain ID is optional, check that it is set correctly when set automatically and manually."
This is ok to merge, @eileenmcnaughton would be great if you could update the test comment though. |
|
Merging based on Matt's Testing and feedback and i'm confident that API looks sound tho |
Overview
Change domain_id from a required parameter in membership_type api to an optional one
Before
domain_id is required
After
domain_id is calculated as being the current domain (if not set & for new transactions)
Technical Details
Comments
This is in support of #12439 & making domain_id optional @mattwire