REF Extract getDefaultRoleID for add participant

[mattwire]

Overview

Extract function to get default role ID if it's not defined.

Before

Custom query was being run whether or not it was required.

After

Custom query replaced by API call and only run when it's actually needed (ie. we don't already have a role ID).

Technical Details

Comments

[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).

[pradpnayak]

How about

'role_id' => self::getDefaultRoleID(
  CRM_Utils_Array::value('participant_role_id', $params)
),

private static function getDefaultRoleID($roleID) {

[monishdeb]

or how about:

...
'role_id' => CRM_Utils_Array::value(
   'participant_role_id',
    $params,
    civicrm_api3('OptionValue', 'getvalue', [
       'return' => "value",
       'option_group_id' => "participant_role",
       'options' => ['limit' => 1],
     ])
  ),
...

:)

[pradpnayak]

I agree with creating a new function for getDefaultRoleID() since it can be re-used elsewhere but i think we should not pass whole params array to that function

[eileenmcnaughton]

I agree - the only reason params is being passed is to check whether we need to look up the default role id - I think that should be done outside the function ie

$roleId = CRM_Utils_Array::value('participant_role_id', $params) ? : self::getDefaultRoleID();

[eileenmcnaughton]

is the is_default=1 not required? Is is_active implicit?

[mattwire]

@monishdeb @pradpnayak @eileenmcnaughton Thanks for the review. I've taken the best of each and updated the PR.

I've added in is_active (that should have been there). Also I've sorted by is_default though they're all NULL on my db and it wasn't previously checking for the is_default flag but it makes sense that it does...

[seamuslee001]

Seems like this has general agreement and has passed unit tests merging