Fixing the display of checkboxes in event confirm / thank you (dev/core#1058)

[samuelsov]

Overview

Checkboxes are not displayed properly in event Confirmation and Thank You pages.

It's only a display problem on the screen : the receipt is correct and the values are correctly saved in the database.

See dev/core#1058

Before

Form:

Confirmation page :

After

The display is as expected : Choice 2, Choice 3

Technical Details

I have ensured that the change applies only to Confirm / Thank you page.

• CRM_Event_BAO_Event::displayProfile is only called from CRM_Event_Form_Registration_Confirm::assignProfiles
• CRM_Event_Form_Registration_Confirm::assignProfiles is only called from CRM_Event_Form_Registration_Confirm::buildQuickForm and CRM_Event_Form_Registration_ThankYou::preProcess

Comments

Quickly comparing Contribution form with Event form, they doesn't seems to use the same method to display the profile information. Eventually, we might want to have a uniform way of doing it...

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

[MegaphoneJon]

@danaskallman This is the same bug that NYC NoWC reported.

[colemanw]

+1 on the concept and the fix.
Nitpick: It looks to me like all of this could be written in 1 line:

$customVal = array_keys(array_filter($params[$name]));

[samuelsov]

Very possible... i'm not familiar with array_filter :)
I will fix this.

[mattwire]

@samuelsov Tracking through the contribution confirm pages I see they use CRM_Core_BAO_UFGroup::buildProfile() to do the same thing. The CRM_Event_BAO_Event::displayProfile() seems to be a different version of that with both basically doing the same thing.

As we have so much duplicate/redundant code between the event/contribution workflows I'd really like to see the function replaced with the shared one in UFGroup rather than being patched up as it will only be a matter of time before another bug comes up - either on the contribution display or the event display and we'll be playing ping-pong to fix them.

If you feel that it is too big a task to do that, consider extracting some part into it's own function that can be shared between the two, that can move us towards a single function in the future.

[samuelsov]

@mattwire Totally agree with removing duplicate code if possible.

I'm not sure what are all the implication but as it seems to be only a matter of display information in a very specific context, probably not so risky with a good test set.

I don't think it should be a blocker for this PR though.

[eileenmcnaughton]

I've checked & this PR fixes the bug - which was a regression from 6ebb4fa (@mlutfy )

I have some reservations about merging this without a unit test as a unit test on this function would have prevented breakage from the last change.

OTOH I agree that it is only called by a narrow range of functions & that it is very much on the form layer (closer to the surface than the function whose change caused this)

I'm going to settle on merging this without the test because it is not quite a recent regression but also not that old & I'd rather see a fix merged for 5.16. The test itself would be quite a big lift since we have poor cover in that area - which is a case both for rejecting this without a test & conversely for letting this squeak through