Fix regression from CRM-19920 : “Deceased” status could be translated.

[otetard]

Call for CRM_Member_BAO_Membership::buildOptions() with the match context in order to have enabled statuses untranslated so we can find if “Deceased” is enabled.

[civicrm-builder]

Can one of the admins verify this patch?

[seamuslee001]

Jenkins test this please

[totten]

Just a quick observation (without really thinking about the overall PR/issue). It looks to me like $deceasedStatusId comes out with a different value here.

$ cv cli
Psy Shell v0.8.0 (PHP 5.6.33 — cli) by Justin Hileman
New version is available (current: v0.8.0, latest: v0.8.17)
>>> CRM_Member_BAO_Membership::buildOptions('status_id', 'create');
=> [
     1 => "New",
     2 => "Current",
     3 => "Grace",
     4 => "Expired",
     5 => "Pending",
     6 => "Cancelled",
     7 => "Deceased",
   ]
>>> array_search('Deceased', CRM_Member_BAO_Membership::buildOptions('status_id', 'create'));
=> 7
>>> array_keys(CRM_Member_BAO_Membership::buildOptions('status_id', 'match'))
=> [
     "New",
     "Current",
     "Grace",
     "Expired",
     "Pending",
     "Cancelled",
     "Deceased",
   ]
>>> array_search('Deceased', array_keys(CRM_Member_BAO_Membership::buildOptions('status_id', 'match')))
=> 6

The searches return different numbers because there's no expectation for indexes of array_keys() to match IDs in DB.

[otetard]

Thanks, I’ve updated my PR: $deceasedStatusId is now retrieved using CRM_Core_PseudoConstant::getKey().

[eileenmcnaughton]

I think it should be

CRM_Core_PseudoConstant::getKey('CRM_Membership_BAO_Membership', 'status_id', 'Deceased') 

or to get the NAME (not the label which is subject to translation

CRM_Core_PseudoConstant::getKey('CRM_Membership_BAO_Membership', 'status_id', $membershipStatusID) 

[otetard]

Thanks for the feedback, I’ve just updated my PR.

Comparison is now done using status_id (returned by CRM_Core_PseudoConstant::getKey('CRM_Member_BAO_Membership', 'status_id', 'Deceased')) which avoids to have translation issues.

[eileenmcnaughton]

I think just if ($deceasedStatusId) would work wouldn't it - it returns false if no key is found

[otetard]

In my case, CRM_Core_PseudoConstant::getKey() returns a value for a membership’s status_id even if not enabled. That’s why we have to use the buildOptions() with the create argument.

Here is an example with the “New” (“Nouveau” in French) status which is disabled in my configuration:

$ cv cli
>>> CRM_Core_PseudoConstant::getKey('CRM_Member_BAO_Membership', 'status_id', 'New');
=> 1
>>> CRM_Member_BAO_Membership::buildOptions('status_id', 'create');
=> [
     2 => "Courant",
     3 => "Délai de grâce",
     4 => "Expiré",
     5 => "En instance",
     6 => "Annulé",
     7 => "Décédé",
   ]
>>>

[eileenmcnaughton]

I think it would be false if no key exists so if($deceasedStatusId) would suffice

[eileenmcnaughton]

ug sorry about the duplicate comments

[eileenmcnaughton]

I'm happy with the change now - I tested it by running through the scheduled job & checking the various variables in the debugger, after changing the status name. Merging

[civicrm-builder]

Can one of the admins verify this patch?

[eileenmcnaughton]

As an aside - we really should have a unit test on this - but I have merged anyway. I do advise you to follow up with one to lock it in