Fix order by on is_primary.

[eileenmcnaughton]

Overview

As pointed out in https://lab.civicrm.org/dev/mail/issues/26 the order by is not the
correct format for order_by and has no effect. I tested to make sure it was not
some magic.

Before

Order by ignored

After

Order by works

Technical Details

I opted for (implict) ASC as the order by for non sms is ASC and further down
processing seems to overwrite each row as it happens so primary later
would overwrite earlier.

Comments

@monishdeb @seamuslee001 @mattwire @totten ping (original commit by Tim but merged by Monish)

[pradpnayak]

@eileenmcnaughton why do we need order by on is_primary when there is a where clause saying is_primary = 1.

(Sorry if its a stupid question!)

[colemanw]

I think that's a great question! Well spotted :)

[eileenmcnaughton]

@pradpnayak @colemanw maybe it should go - I lifted this from the gitlab & I see there is a further comment on the gitlab

[eileenmcnaughton]

@pradpnayak @colemanw I think @kenwest is arguing to remove the is_primary filter https://lab.civicrm.org/dev/mail/issues/25

[eileenmcnaughton]

Closing this as it's only part of the picture & #12890 seems to get a bit closer to the full fix (with tests)

[JKingsnorth]

I opted for (implict) ASC as the order by for non sms is ASC and further down
processing seems to overwrite each row as it happens so primary later
would overwrite earlier.

Is this documented in the function somewhere? It would be great to add it as comments, as the 'logical' expectation is that the 'first' record to be selected is the one that's used.

[eileenmcnaughton]

@JKingsnorth I hadn't actually merged this - I closed it because I thought is was superceded - but I think we have agreed it is needed?

[eileenmcnaughton]

(I just re-opened)

[JKingsnorth]

Ahhhh, OK, we were working on this as part of #12890 - which has now been merged. We need to fix the primary ordering at the same time, otherwise we're going to introduce a regression.

Can we revert #12890 and then work on this 'is_primary' ordering in that ticket?

[JKingsnorth]

We also need to establish why the unit tests aren't picking up that problem, because there is a test in place that should ensure that 'primary' is being selected over a 'non-primary'.

[seamuslee001]

@JKingsnorth i think the best thing would be to work on creating a new PR to address those issues but we log an issue noting that 5.7 has this issue. Noting 5.8 would be due out in I think December

[eileenmcnaughton]

@JKingsnorth is the next part (order_by) being worked on pretty actively?

[JKingsnorth]

@seamuslee001 @eileenmcnaughton yes it's being worked on actively.

I would prefer not to log an issue saying this is a problem in 5.7 because selecting mailing recipients is a critical issue if something goes wrong. I would prefer to revert the change and then work on both together.

[eileenmcnaughton]

sorry to be clear - the patch I merged was against master - 5.7 has already been branched so we are working now on what will be in 5.8.

From what I can see the line in the test I commented on on the other PR needs to change & then we need to merge a variant of this to make the test pass

[eileenmcnaughton]

@JKingsnorth are you saying we should revert & then do a PR against 5.7 rc (which is what we would do if there is a regression in a released version or in 5.7 already) or are you saying that unless we follow through to resolution there is a regression in master (5.8) as of today

[JKingsnorth]

@eileenmcnaughton Sorry to be unclear.

#12890 <- this PR should be reverted in master. If it is already part of an RC, then it should be reverted in there too.

We should then continue work on that issue against master until it is ready to be merged. In order for #12890 to be merged we need to fix the tests, fix the order_by issue (which is what is under discussion in this PR), and do some more UI testing.

[JKingsnorth]

We could have prevented this confusion by adding 'DNM' (do not merge) or WIP (work in progress) on the front of #12890 - because it was not ready to be merged yet. Sorry for the confusion.

[eileenmcnaughton]

@JKingsnorth I guess I'm just a little bit reluctant to revert because I feel like we are so close to having the full fix & I'd love to push on & just get it all sorted.

It's only merged to master (which will be rc first week of Nov)

[JKingsnorth]

@eileenmcnaughton OK, we'll push on with it! But if we don't make the RC then we really should revert. We can open a further PR with the fix and changes.

[eileenmcnaughton]

@JKingsnorth for sure

[eileenmcnaughton]

@JKingsnorth so I actually should close this one & you will put up a replacement?

If feels like it maybe should be this clause goes in & the clause in the test that I commented should be altered

[JKingsnorth]

@eileenmcnaughton yes, that should do it, @dereklewis123 is working on this now.

[eileenmcnaughton]

@JKingsnorth fantastic ... & shame on you for not making the trek north this week :-)

[JKingsnorth]

@eileenmcnaughton OK! We got to the bottom of this one.

I think we should merge THIS PR (#12765) - which removes the odd ' = 1' from the order by clause. This now makes sense, because the other PR (#12890) has now removed the filter on is_primary, as part of another fix.

Functionally, order by is_primary = 1 and order by is_primary asc do exactly the same thing in the SQL - the ordering of the results if you run the raw queries are the same.

We complicated matters because were testing this against our current version of CiviCRM (5.3.1) which has a different issue whereby the order by clause was not being added to the query at all. This has since been resolved. So #12890 does not cause any regressions in the latest version of CiviCRM.

[seamuslee001]

added merge on pass based on @JKingsnorth review