Use new lookup trait to eliminate use of undefined properties userDisplayName

[eileenmcnaughton]

Overview

Use new lookup trait to eliminate use of undefined properties userDisplayName

Before

Property _userDisplayName referenced in multiple places but not defined. The only place where it is reference from a function other than where it is determined is in the Paricipant form preProcess. In addition the participant form also uses contributorDisplayName to refer to the same value - this second variant is used to set the message to be displayed to the user at the end (only). It refers to the same contact (the ability to record a payment from a different contact is only available on the Membership forms, not events)

After

Rather than define these 2 properties there are 2 approaches

1. use local variables where possible
2. use getContactValue() to get the variables when needed in the setStatus and preProcess functions

Technical Details

@colemanw I am using the new lookup function here - but I wound up wrapping in it a function - https://github.com/civicrm/civicrm-core/compare/master...eileenmcnaughton:civicrm-core:lookup?expand=1#diff-a1c8943f991b49e1c885133fad3edc222523542e1ce97b89d7fbba222197000fR576-R588 because

a) the goal is to be able to call the function anywhere on the form and if the contact is not yet defined it should return NULL - hence it can be used for things like

$this->assign('displayName', $this->getContactValue('display_name');

b) the contact ID becomes available at different points in different flows. If a contact ID (cid) is in the url or an ID is in in the url it can be looked up in preProcess. However, it is not available until postProcess otherwise. The goal is to be able to look up a value from any part of the form & get the same results.

Note that this getContactValue() is on the one form at the moment. That form has pre-existing getParticipantValue() & getEventValue() functions - all 3 of which would probably wind up in a FormTrait rather than the form once it is settled

Comments

Merging #27258 will simplify this

[civibot]

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

• If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
• For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

• ➡️ PR Demo site (can be rebuilt with the command test this please).
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

[eileenmcnaughton]

Hmm it switched to actual display name - not first/ last for the name in the email 'to'

I think that is good, not bad....

To: Anthony Anderson <[email protected]> .  not found in  MIME-Version: 1.0


From: "FIXME" <[email protected]>
To: "Mr. Anthony Anderson II" <[email protected]>

[colemanw]

@eileenmcnaughton I think the overcomplicatedness of these forms lies in the way they store and pass around entity ids as well as entity values.

• Status Quo: Entity ids are stashed "somewhere". Maybe a class variable like $this->_contactID or a controller property like $this->set('cid') or a local variable retrieved from the url. But they have to be passed around a lot because they are needed if you want to do any lookups (to get the display name of a contact, you need to know the id).
• LookupTrait: Entity ids are stored by calling define() and then you can forget about them. To get the display name of a contact you just need the entity nickname: $this->lookup('MainContact', 'display_name'). No need to pass around ids, the nicknames act like constants. But if you do need to fetch an id you can still get it: $this->lookup('MainContact', 'id').

I think the reason this PR struggles to make use of the lookup trait is because #27258 entrenches further into the status quo by adding contact id getters/setters and then this PR tries to adapt the lookup trait retroactively on top of those getter/setters.

But if you mentally shift to the new model, LookupTrait is quite elegant. You don't need to make a new getter/setter for contact_id because EntityLookupTrait:define() is already a setter for id, and lookup() is already a getter that works for id as well as any other field value.

[eileenmcnaughton]

@colemanw yeah - but then you need to call the 'define' in multiple places & check before doing so whether is already defined before doing so because flow-dependent it might or might not be.

The relevant ids (contact ID is one, others are event ID, participant ID, contribution ID price set ID and discount ID) are available at different points in time in different flows.

[colemanw]

@eileenmcnaughton if it would help, I could push up another patch to make define() do nothing if the entity has already been defined with the same id.

[eileenmcnaughton]

@colemanw possibly - but would that make it more error prone too? Although I could make a case that double calling define would add to any existing - eg. I've just saved a value & I want to update (or create) the defined entity.

The issue isn't just double calling define - it's know knowing if the entity is defined at the point in the flow you are at. We can get the various IDs from multiple places depending on the flow - the use of getContactID() ensures that if it is gettable at the point the function is called then it will be returned. So in setDefaultValues() it might be NULL but in postProcess it can be retrieved (for this specific form) regardless of whether it was there in preProcess or not when any of the various properties used ( contactId or contactID of cid or _cid or _contactId or _contactID) were set.

[eileenmcnaughton]

@colemanw I tried to use the new trait in a different scenario #27273 & in that case it would have been usable with the addition of support for

if ($this->isDefined('Event)

That scenario differs from this in that there is just one path through the code & the relevant entities either can be defined at that point or they don't exist

[colemanw]

@eileenmcnaughton ok. #27275

[eileenmcnaughton]

@colemanw that makes this look a bit nicer....

https://github.com/civicrm/civicrm-core/pull/27259/files#diff-a1c8943f991b49e1c885133fad3edc222523542e1ce97b89d7fbba222197000fR576-R593

[colemanw]

@eileenmcnaughton agreed.

[eileenmcnaughton]

Ok - it's been merge ready a couple of days - let's merge