Toward CRM-19751: conditionally change On-Hold criteria to select on …

[twomice]

…Advanced Search form.

Overview

The issue CRM-19751 notes that Advanced Search criteria "email on hold" doesn't find emails with the "on hold opt-out" setting (which is only available when the CiviMail component setting "Enable multiple bulk email address for a contact" is enabled.

To fix this, we need the form to offer a set of options for this criteria, not just a checkbox.

PR #12883 addressed an underlying issue in the XML/SQL schema. Following onto that, the PR provided here makes the actual changes to the form.

Before

The "Email On Hold" criteria is a checkbox.

This allows searching for contacts having an email address with on_hold = 1, which equates to "on hold" when the "Enable multiple bulk email address for a contact" setting is disabled, and equates to "on hold - bounce" when the setting is enabled. There's no way to search for contacts having emails set to "on hold - opt out" (which is only available when this setting is enabled).

After

The "Email On Hold" criteria is still a checkbox when the "Enable multiple bulk email address for a contact" setting is disabled, so it still works as it did before in that state.

This still allows searching for contacts having an email address with on_hold = 1, which equates to "on hold" when the "Enable multiple bulk email address for a contact" setting is disabled, and equates to "on hold - bounce" when the setting is enabled.

On the other hand, when the "Enable multiple bulk email address for a contact" setting is enabled, the "Email On Hold" criteria is a multi-select, allowing to search on any of the three possible values for an email "on hold" property.

Technical Details

Comments

Needs tests.

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

[twomice]

I've removed the WIP prefix, as this PR now has unit tests is ready for review.

[alifrumin]

This is ready to be merged from my perspective.

I was able to replicate this issue on a demo site, then I tested this pr by doing the following:

1. Creating a test site with this pr

2. Logging in as an admin

3. Going to the advanced search form with "Enable multiple bulk email address for a contact" setting disabled (It is disabled by default).

   Behavior: the "Email On Hold" field shows up as a checkbox

4. Going to Administer->CiviMail->CiviMail Component Settings enabling the "Enable multiple bulk email address for a contact" setting

5. Going back to the Advanced Search form with the "Enable multiple bulk email address for a contact" setting enabled

   Behavior: the "Email On Hold" field is displayed as a select where one can choose the type of hold or "No" see screenshot below:

I tested searching using all three options (No, On Hold Bounce, On Hold Opt Out) as well as without the field populated and searching filters appropriately.

[twomice]

Thanks @alifrumin ! I guess you'd be fine with me pinging @eileenmcnaughton asking to merge.

[eileenmcnaughton]

@alifrumin you are a star! Really appreciate all the reviewing you have been doing.

There is one thing I think we need to test before merging

1. create a smart group with the on hold field checked
2. apply this patch
3. check if the smart group still works

Potentially the above could be done with it unchecked too but I think that is actually meaningless as it would just be ignored

[eileenmcnaughton]

Also @alifrumin if you do think something should be merged please ping me - I can happily prioritise your pings based on all the review you have been doing

[alifrumin]

Thank you @eileenmcnaughton. Good catch! I was not thinking about how this would effect existing smart groups.

I followed Eileen's instructions above:

1. create a smart group with the on hold field checked
2. apply this patch
3. check if the smart group still works
   Behaviour: The smart group with criteria "on hold = 1" does not include contacts having emails set to "on hold - opt out" (this is the initial issue as described by @twomice in the before section quite clearly).

This pr makes it possible to make new smart groups that include contacts having emails set to "on hold - opt out" (by searching using the new select). This pr does not fix existing smart groups that are looking for "on hold = 1", nor does it break these smart groups more than they were already broken. Seems like progress but @eileenmcnaughton I will leave it to you if you think dealing with existing smart groups should be dealt with before merging or if that could be a separate issue.

[eileenmcnaughton]

@alifrumin so do we have 4 scenarios here

1. Enable multiple bulk email address for a contact is enabled - existing smart group doesn't work still doesn't after this
2. Enable multiple bulk email address for a contact is enabled - new smart groups don't work without this but do with it
3. Enable multiple bulk email address for a contact is disabled - existing smart groups ???
4. Enable multiple bulk email address for a contact is disabled - new smart groups presumably work with this

[alifrumin]

1. Enable multiple bulk email address for a contact is enabled - existing smart group doesn't work still doesn't after this
   when we say doesn't work we mean does not include contacts having emails with the "Email On Hold" field set to "on hold - opt out" (which is only available when this setting is enabled).

2. Enable multiple bulk email address for a contact is enabled - new smart groups don't work without this but do with it
   Right

3. Enable multiple bulk email address for a contact is disabled - existing smart groups ???
   If one had never enabled the "Enable multiple bulk email address for a contact" setting then existing smart groups would work fine because no one would have "on hold - opt out" for the Email on hold field, however if someone had enabled the "Enable multiple bulk email address for a contact" setting, set the Email on hold field to "on hold - opt out" for one or more than one contacts and then disabled the "Enable multiple bulk email address for a contact" setting that contact would not show up in a smart group that is based on the search criteria on hold = 1 (the way an existing smart group would be)

4. Enable multiple bulk email address for a contact is disabled - new smart groups presumably work with this
   yes, new smart groups work but if you go to edit the search criteria the "Email on hold" field is a checkbox so then you can't edit them

After thinking thru these scenarios, I think it makes sense for the Email On Hold field on the advanced search form to always be a select.

[eileenmcnaughton]

@alifrumin I agree with ", I think it makes sense for the Email On Hold field on the advanced search form to always be a select." - @twomice what do you think?

[twomice]

@eileenmcnaughton

From the UX viewpoint, I agree.

Implementation seems tricky, though. The select list is driven by a method that returns an array with certain labels. We'd need that array to differ -- both in length and in the labels -- depending on the value of the "multiple bulk email address" setting, e.g.:

"multiple bulk email address" = yes:

• 0: none
• 1: On Hold Bounce
• 2: On Hold Opt Out

"multiple bulk email address" = no:

• 0: No
• 1: Yes

Now our neat little method isn't so neat anymore. Checkbox allows us to sidestep that issue.

[eileenmcnaughton]

@twomice I'm having a lot of trouble processing this today - partly because of other meta distractions - on reading the above do you think there is a problem for sites with existing smart groups? I've having thought through that question you & @alifrumin think this should be merged I'm happy to merge it.

[alifrumin]

I think this moves things forward without breaking things further than they have already been broken, but I could see it causing confusion for a small number of people

@twomice maybe we could add a note to the "Enable multiple bulk email address for a contact" setting field that says something along the lines of "Enabling this setting will change the options for the Email on Hold field" and a note to the screen you see when you complete an upgrade to update any existing smart groups if you have this setting enabled?

[twomice]

@alifrumin I agree with the spirit of your comment: "moves things forward without breaking things further than they have already been broken". I'm also concerned with the potential "confusion for a small number of people".

I like the idea of adding some kind of explanation for the parts that are essentially "still broken" WRT smart groups. Which leads me to wonder -- and I admit I haven't looked at this and that maybe I'm the person who should, but in any case -- do you happen to know if/how a user can update an existing smart group so that it will work "as expected" with this change? I mean, is it as simple as editing smart group critiera and re-saving the group? If so, I think we can be content with letting people know they might need to do that, and solve that problem as a separate issue.

[alifrumin]

In response to "do you happen to know if/how a user can update an existing smart group so that it will work "as expected" with this change? I mean, is it as simple as editing smart group criteria and re-saving the group?" -- short answer yes see details below (I tested this on the jenkins build.
):

IF you have a smart group with the criteria email on hold =1 (that you create before enabling the setting with the checkbox)

THEN if you enable the setting and go to edit the smart group the email on hold field shows as a select with no default see screenshot below:

From there you can edit the smart group criteria (using the select box for Email On Hold) and resave the group and it works as expected.

[twomice]

Great, thanks Ali. In this case, I think it would be good to take the "fix and warn" approach, basically what you suggested; that is, I would add these things to this PR:

• Help text under the "multiple on-hold" setting along the lines of "Enabling this setting will change the options for the Email on Hold field"
• A note at upgrade time alerting the user to update any existing smart groups if this setting is enabled.

If you're okay with that, I can add that to the PR, and we can tell Eileen we think it's good to merge. Sound good?

[alifrumin]

Perfect @twomice. I will also put something in the release notes about it

[eileenmcnaughton]

@alifrumin we cut 5.8 yesterday - if it's too much of a nightmare for you to manage the release notes part for a release that won't be cut for a month I would be OK to merge this to the rc if we tidy it up pretty quickly

[alifrumin]

We haven’t started the release notes for 5.8 yet So it won’t be an issue for release notes.

[eileenmcnaughton]

@alifrumin If we merge this now it will go into 5.9 - I'll proceed with that though on the assumption you'll still remember it by the time we cut that rc

[eileenmcnaughton]

@twomice was there a last change on you here?

[twomice]

Yep, that was on me. Done now, fingers crossed.

[twomice]

Jenkins test this please.

[twomice]

Tests passing, yay! @eileenmcnaughton I think this is ready for merge now, or at least for your review towards that end. Thanks!

[eileenmcnaughton]

@twomice I think the changes to this form are not related to your PR but are merge mess?

We have removed this chunk of code from core (ideally we would also remove the rest of the form & jut use the Generic form but we'd need to move those post actions to metadata per the todo)

[twomice]

Dang it, I though I had it this time. Trying again...

[twomice]

Ok, that's removed @eileenmcnaughton . I'll ping again when jenkins tests are done. Thanks!

[eileenmcnaughton]

@twomice I've given the code a last look over & nothing else seems amiss so I think we can merge once tests pass. Thank you & also @alifrumin for persevering with this one as it took some effort

[alifrumin]

@eileenmcnaughton thank you for persevering on this as well :)

[twomice]

Fantastic. Thanks Ali for the reviews, and Eileen for patient mentoring on PR management.