Fix API call params in updateAllMembershipStatus()

[ghost]

Overview

API implementation for Create Membership has changed to need contact_id. Otherwise it throws an error. This causes the Scheduled Job to Update Membership Statuses to fail, unfortunately without notification.

Before

Scheduled Job to Update Membership Statuses fails. It reports that records have been updated, but no change actually occurs.

After

Job now works correctly.

Technical Details

The PR removes a line of code which removed contact_id from the API parameter list in updateAllMembershipStatus(). The comment says that this is to prevent possible race conditions with other parallel activity, but I don't see that there is a risk of the Contact Id also being altered.

The API Explorer says that both Contact Id and Membership Type Id are mandatory on this API call (Create Membership). However, previously the code functioned OK to update an existing Membership with just the Membership Id being supplied. Now it returns an error if the Contact Id is missing.

In our situation it still works without the Membership Type Id, but a Contact only ever has a single Membership, I don't know what happens if there are multiple Memberships. I can see a possible (if rare) race condition on Membership Type Id, so have left this unaltered.

[civicrm-builder]

Can one of the admins verify this patch?

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

add to whitelist

[eileenmcnaughton]

unrelated test fail

[mattwire]

@eileenmcnaughton Test fail looks like it could be related to me?

[mattwire]

@maynardsmith Do you know which PR changed the API signature?

[eileenmcnaughton]

@mattwire re test - check all the other PRs over the last few hours.... It's a newish test that 'expired' as the membership dates stopped working

[ghost]

@mattwire No sorry. I upgraded from 5.24 to 5.27.2 and then noticed the problem, but can't guarantee it was OK in 5.24 since we are just hitting our renewals season and there was so little activity before that.

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@maynardsmith I think that you have hit an edge case bug rather than a change in api signature - we haven't had other reports despite people being obviously working on this. We did merge a couple of fixes recently - I would like to close this & seek more information

[lalgwebdev]

@eileenmcnaughton OK, I may have made the wrong inference about the root cause of the problem, and/or applied the wrong fix. However, my observation stands, that in our system the Scheduled Job 'Update Membership Processes' (job.process_membership) is failing.

• This is now showing on 5.28.2.
• It gives the failure message Finished execution of Update Membership Statuses with result: Failure, Error message: Expected one Contact but found 25 in the Scheduled Job Log. I have seen this elsewhere when a Contact Id is not supplied but is wanted.
• Contrary to the statement above it no longer reports that records were processed - just gives this error message.
• The patch in the PR above continues to fix the problem by supplying the Contact Id on the API call (except that the line number is now 2323). We are continuing to run with this patch applied in lieu of any alternative.

It is not obvious to me that we are doing anything that is not pretty standard, and am not sure what edge case we may be running into. The only possibility I can see is that we have renamed one Membership state, and introduced a custom one of our own, but with the change above that all works. What further information do you need?

[eileenmcnaughton]

@maynardsmith - what were these "we have renamed one Membership state, and introduced a custom one of our own"

Also - can you determine if it is stalling on a particular contact ? (I think the 2 patches merged were after 5.28.2 but one related to a scenario related to ma_related memberships - and the other was the alternate fix to the one you logged

[lalgwebdev]

@eileenmcnaughton
We have renamed 'Lapsed' as 'Overdue', otherwise no change except setting the end date 30 days after End Date.
We introduced 'Renewal Period' which runs for (currently) 42 days before End Date. We use this to change visibility of some fields on the user's webform with added jQuery.

I don't think it's stalling on any particular contact. After upgrading to 5.28.2 on 31st August I forgot to reapply the patch and it just stopped processing any changes until today when I did so.

[lalgwebdev]

Re. previous comment:

[eileenmcnaughton]

There must be something else in play - as long as id is not empty contact_id is not required & the test testProcessMembershipUpdateStatus steps right through this code.

The query to get id looks like this

SELECT     civicrm_membership.id                    as membership_id,
  civicrm_membership.is_override           as is_override,
  civicrm_membership.status_override_end_date  as status_override_end_date,
  civicrm_membership.membership_type_id    as membership_type_id,
  civicrm_membership.status_id             as status_id,
  civicrm_membership.join_date             as join_date,
  civicrm_membership.start_date            as start_date,
  civicrm_membership.end_date              as end_date,
  civicrm_membership.source                as source,
  civicrm_contact.id                       as contact_id,
  civicrm_membership.owner_membership_id   as owner_membership_id,
  civicrm_membership.contribution_recur_id as recur_id
FROM       civicrm_membership
 INNER JOIN civicrm_contact ON ( civicrm_membership.contact_id = civicrm_contact.id )
 INNER JOIN civicrm_membership_type ON
  (civicrm_membership.membership_type_id = civicrm_membership_type.id  AND civicrm_membership_type.is_active = 1)
WHERE civicrm_contact.is_deceased = 0 
  AND civicrm_membership.is_test = 0 
  AND (civicrm_membership.is_override = 0 OR civicrm_membership.is_override IS NULL)
  AND civicrm_membership.status_id NOT IN (5, 6, 4, 7)
  AND civicrm_membership.owner_membership_id IS NULL 

Are you able to put in some debug / get a backtrace?

[eileenmcnaughton]

I opened this as a cleanup after digging through the code #18432

[lalgwebdev]

@eileenmcnaughton My cock-up I'm afraid. I have another routine in the Pre hook that is throwing the error. Nothing to do with the standard code.
Please close.
Apologies.

[eileenmcnaughton]

thanks for confirming @lalgwebdev