dev/core#745 do not filter on 'on_hold' if it is an empty string

[eileenmcnaughton]

Overview

Fixes a regression on smart groups where an on-hold filter is added to some legacy groups

Before

Legacy groups in correctly filter for on-hold

After

Filter shouldn't happen

Technical Details

Although I couldn't find a way to create a group on an earlier version with the format that causes this; that doesn't mean there aren't systems with those groups.

We only care about number (0 or 1) or arrays (from the select widget)
so add an extra check before filtering

Comments

@scardinius this is slightly different to yours as I see it as a temporary hack & tried to make it look like one - can you check

https://lab.civicrm.org/dev/core/issues/745

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

[eileenmcnaughton]

@twomice

[eileenmcnaughton]

test this please

[eileenmcnaughton]

test this please

[agileware-fj]

@eileenmcnaughton we've run into this issue on a few sites, happy to review – but I wonder if this needs a unit test?

[eileenmcnaughton]

@agileware-fj yes ideally it does!

I think if getting it into this week's release relies on me writing a unit test for it it won't happen though.

We do sometimes loosen the test requirement when fixing rc regressions because we generally have people who neither created the bug or are affected by the bug dropping other things to try to ensure we get a good stable release. It's kinda the vaccine theory - we get herd immunity by pushing for tests overall so when we urgently need to get a regression fixed on a release that is about to be dropped or has already gone out we can hopefully get away with an unvaccinated fix

(If I were fixing it on paid time or had not spent a lot of unpaid time trying to replicate this & other reported bugs already this week, or even if I had succeeded in replicating it I would have done a test, but in the end I just threw this in as 'might help someone, don't think it could hurt')

[eileenmcnaughton]

test this please

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@agileware-fj can you clarify how this happens exactly - ie. what format the legacy groups are in as @seamuslee001 & I couldn't replicate it so it's kinda a blind fix - but I can see it's also somewhat important & the release is due out tomorrow

[jusfreeman]

@seamuslee001 the problem is that smart groups have been previously created with the on_hold field saved as part of the search criteria, it's not that the user has explicitly set the "on hold" value in the search criteria. Because the search form used to save the "on hold" value = empty string. Our fix in all cases has been to execute SQL to remove the on_hold from the saved search field criteria and then the smart group can be loaded again.

These smart groups may have been created with CiviCRM 4.6, 4.7 or 5.x - in all cases the smart group was created many releases ago.

Below is a copy of our internal FAQ for this issue.

CiviCRM Error - civicrm_email.on_hold - Truncated incorrect DOUBLE value

Solution
Execute the following SQL on the CiviCRM database. Note that the table may have table prefix.
update civicrm_saved_search set form_values = replace(form_values, 'on_hold";s:0:""', 'on_hold";i:0');

Problem
And you will see these types of errors in the CiviCRM log files.
[type] => DB_Error [user_info] => CREATE TEMPORARY TABLE civicrm_temp_group_contact_cache432 (SELECT 72 as group_id, contact_a.id as id FROM civicrm_contact contact_a LEFT JOIN civicrm_email ON (contact_a.id = civicrm_ema il.contact_id AND civicrm_email.is_primary = 1) LEFT JOIN civicrm_relationship ON (civicrm_relationship.contact_id_a = contact_a.id ) LEFT JOIN civicrm_contact contact_b ON (civicrm_relationship.contact_id_b = contact_b.id ) WHERE ( civicrm_email.on_hold IN ("") AND (

[jusfreeman]

@eileenmcnaughton just a random thought, it might be useful to store the CiviCRM version in some metadata when a smart group is created. Could help in situations like this, possibly.

[eileenmcnaughton]

@jusfreeman so my understanding is that

'on_hold";s:0:""' means
'I don't care but the damn form saved this field'

Whereas

'on_hold";i:0' means
'is not on hold'

so your sql might change the meaning of it

Possibly we should cull empty keys in an upgrade script but I feel like 'not for 5.11'

[eileenmcnaughton]

it might be useful to store the CiviCRM version in some metadata when a smart group is created. Could help in situations like this, possibly.

Lol - then we'd have to trawl through every version to figure out what format was when - but you are right it could add some useful debugging.

[agileware-fj]

@eileenmcnaughton from what I could tell, having no value in the on_hold field was actually equivalent to "is not on hold", although that may not have been the intention when the individual searches were built. Of course, use of serialised PHP meant that we couldn't do a quick fix in the database that just removed the value anyway.

[eileenmcnaughton]

@agileware-fj & is it your read that this PR would alleviate the sympton without causing further harm for those with the original values

[agileware-fj]

@eileenmcnaughton Yes, that's what I'm reading.

More specifically, I think this is probably the workaround that causes the least harm, as if the group was really trying to exclude on_hold emails it was almost definitely to work around that bug for mailings where the on_hold status wasn't being honoured.

[eileenmcnaughton]

@seamuslee001 ^^

[agileware-fj]

So just for formality

• General standards
  • Explain (r-explain)
    • PASS : The goal/problem/solution have been adequately explained in the PR and on Gitlab
  • User impact (r-user)
    • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
    • COMMENTS: Where the change is noticeable the effect is to fix a feature that was no longer working
  • Documentation (r-doc)
    • PASS: The changes do not require documentation.
  • Run it (r-run)
    • PASS:
      1. Tested on CiviCRM database showing on_hold regression, worked around problem
      2. Tested on CiviCRM database without on_hold regression, saw no changes in output.
• Developer standards
  • Technical impact (r-tech)
    • PASS: The change potentially restores compatibility with existing installations while maintaining existing compatibility.
  • Code quality (r-code)
    • PASS: The functionality, purpose, and style of the code seems clear & sensible.
  • Maintainability (r-maint)
    • PASS: The change does not sufficiently improve test coverage, but special circumstances make it important to accept the change anyway.
  • Test results (r-test)
    • PASS: The test results are all-clear.

[seamuslee001]

@agileware-fj @jusfreeman i'm happy to go with this however i just want to check

i have only seen form values like this

'email_on_hold' => [
   'on_hold' => '',
]

or

'email_on_hold' => [
   'on_hold' => NULL,
]

or

'email_on_hold' => [
   'on_hold' => 1,
]

What i am not understanding is how this is fixing the '' strings cases maybe there is some cleaning function later on

[eileenmcnaughton]

@seamuslee001 - the first 2 above should result in no filter on on_hold being applied. The latter should render to on_hold IN (1)

[seamuslee001]

I'm going to merge this on the grounds its safe and an improvement based on Agileware's comments