dev/core#190 / CRM-21643 ensure custom data multi record profile returns correct …

[lcdservices]

…record

Overview

Ensure the most recently created record is retrieved after submitting custom multi data profile.

Steps to replicate:

1. Create a custom field set that allows multiple records (for an Individual, say)
2. Create a field in this set
3. Create a new contact record with [email protected]
4. On this contact record, populate the custom field with an initial entry. Enter 'first data' or similar.
5. Create a profile with a) an email address field b) this custom field
6. Set the profile settings to 'update the matching contact'
7. Go to the profile list on Civi, click 'more' and go to 'Use - Create mode'
8. Enter '[email protected]' and 'second data', and submit the form
9. The confirmation page shows 'first data'.
   Expected: The custom field should show the last data entered i.e. 'second data'

See: https://lab.civicrm.org/dev/core/issues/190

• CRM-21643: Profile confirmation page shows the wrong values for multi-value custom data

Before

After submitting a profile from create view that has custom multi fields, the post-submit profile view page displays the first record for that contact instead of the one just submitted.

After

The data for custom multi fields just submitted are displayed.

Technical Details

The query that gets the values for the profile needs to cycle through and get the last record instead of assuming a single record will be returned.

See #11529 for more discussion

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

[seamuslee001]

@lcdservices brian i suspect you will need a space between teh while and (

[eileenmcnaughton]

I think there is another open PR for this same issue - I just hadn't felt comfortable that the concept was approved in that it seems there could be other opinions about 'correct' behaviour

[lcdservices]

@eileenmcnaughton you're right -- #11529
and I actually reviewed that one. which says something about my memory...

you're right as well that this PR is a bit different. @monishdeb can you review this one? it's pretty basic -- we just make sure we've cycled through the custom table rows to get to the last one before we continue. per the other PR, I really don't like the idea of concatenating values. that's not what the end user would expect and would get very confusing.

[eileenmcnaughton]

@lcdservices is it solving the same 'problem' - ie. that the 'wrong' data is displayed? If so I think it could be debated & I think some steps should be taken to canvass opinions - perhaps email the dev list

[eileenmcnaughton]

(given that there was relatively little response before I'm Ok with the principle that we will merge in 10 days if no dissent)

[lcdservices]

@eileenmcnaughton yes, it's solving the same problem. I'm not sure it's that controversial. In the discussion on the other ticket, @monishdeb brought up the idea of concatenating values, but the prior discussion and his actual PR all seemed in agreement that the user would expect to see the values they just submitted.

[eileenmcnaughton]

@lcdservices I may be wrong about people having differing opinions - but we have had conversations about multi-select fields before with people arguing that the 'right' field is the latest field - but actually they are all created equal

[eileenmcnaughton]

@monishdeb @lcdservices I just looked at this again and tried to merge it with #11529

To the extent that it affects the screen UFGroup::getValues - that called from lots of places so this is not very contained - much as I hate to suggest it could we add a $isGetHighestID param to the function & handle that way?

I was going to close one of the 2 issues in favour of the other - but I'll leave for a few days so you & Monish can choose one

[eileenmcnaughton]

@monishdeb @lcdservices what about instead of that fetch loop just adding a last sort param of 'id DESC' or similar in here

    $details = $query->searchQuery(0, 0, NULL, FALSE, FALSE,
      FALSE, FALSE, FALSE, $additionalWhereClause);

ie something like

    $details = $query->searchQuery(0, 0, NULL, FALSE, FALSE,
      FALSE, FALSE, FALSE, $additionalWhereClause, 'id DESC');

[lcdservices]

I'm not sure why, but adding the sort param doesn't work.
The original PR still works as expected...

[eileenmcnaughton]

@lcdservices I didn't test it I guessed the value - but the original PR won't work on all installs. Not all versions /configurations of mysql naturally sort by id - so for some sites the existing PR will give a random record

[eileenmcnaughton]

I'm going to merge this - I don't think it's reliable & it should be tested but I'm going to accept that the most recent field is generally the best & this results in that in more cases than otherwise.

Really we should put in a test - but this has hung around for a long time in various forms & it's really down to merge or reject at this stage & merge seems OK all considered