Revert civicrm_action_schedule.limit_to boolean column to be NULL

[monishdeb]

Overview

Revert the boolean field civicrm_action_schedule.limit_to to be NULL and not required, to address issues https://lab.civicrm.org/dev/core/-/issues/3464 and https://lab.civicrm.org/dev/core/-/issues/3465

In my opinion, limit_to should be int with three options.

Comments

ping @agileware-justin @eileenmcnaughton @MegaphoneJon @colemanw

[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]

@monishdeb we need to merge to the 5.50 rc before we can merge to 5.49 stable - can you put up a PR against the rc too (this will be mergeable once that is merged, if no changes made to that one)

[monishdeb]

Created PR for 5.50 rc #23478

[eileenmcnaughton]

Test Result (11 failures / +11)
CivicrmUpgradeTest.CivicrmUpgradeTest.4_6_36-setupsh_sql_bz2
CivicrmUpgradeTest.CivicrmUpgradeTest.4_7_0-setupsh_sql_bz2
CivicrmUpgradeTest.CivicrmUpgradeTest.4_7_31-setupsh_sql_bz2
CivicrmUpgradeTest.CivicrmUpgradeTest.5_7_3-setupsh_mysql_bz2
CivicrmUpgradeTest.CivicrmUpgradeTest.5_13_3-multilingual_af_bg_en_mysql_bz2
CivicrmUpgradeTest.CivicrmUpgradeTest.5_13_5-setupsh_mysql_bz2
CivicrmUpgradeTest.CivicrmUpgradeTest.5_21_2-setupsh_mysql_bz2
CivicrmUpgradeTest.CivicrmUpgradeTest.5_27_4-setupsh_mysql_bz2
CivicrmUpgradeTest.CivicrmUpgradeTest.5_33_2-setupsh_mysql_bz2
CivicrmUpgradeTest.CivicrmUpgradeTest.5_39_1-setupsh_mysql_bz2

[totten]

@monishdeb I would suggest a second commit based on:

./tools/bin/scripts/set-version.php 5.49.2 --commit

[monishdeb]

@monishdeb I would suggest a second commit based on:

./tools/bin/scripts/set-version.php 5.49.2 --commit

done :) ..thanks!

[totten]

@monishdeb To try this, I created a 5.48 database with ~10 scheduled-reminders. Each reminder was in a different configuration status, eg

• An example with limit_to="Neither" (null)
• Two examples with limit_to="Also Include" (0) (one using "Select Group"; another using "Choose Recipients")
• Three examples with limit_to="Limit To" (1) (one using "Select Group"; another using "Choose Recipients"; another using "Soft Credit")
• A few examples which were initially saved as one thing (eg "Limit To") and then edited/downgraded to the simpler "Neither".

Then I used this DB snapshot with various upgrade paths, eg

• 5.48.2 => 5.49.1
• 5.48.2 => 5.49.1 => 5.49.2 (w/patch)
• 5.48.2 => 5.49.2 (w/patch)

The results are here: https://gist.github.com/totten/7f43420108d4e40c4d1aabbda1763f1b

A few issues:

• In 548-5492.txt, rows #11 and #12 both got munged values of limit_to. I think this could be fixed with an extra guard: if it's a direct upgrade from good-to-good (if limit_to is already nullable), then changeBooleanColumnLimitTo() should not do anything.
• In 548-5491-5492.txt , rows #7, #8, #9, #10 got munged values of limit_to=0. Of course, this really happened in the 5.49.1 step, and I don't see how to fix those ones. However, perhaps we could give a notice to point people at records which should be re-considered?

[agileware-justin]

If limit_to=NULL then group_id should also be set to NULL - to address https://lab.civicrm.org/dev/core/-/issues/3464#note_74627

If the limit_to value is 1 then remove the value of the Group ID field on save of the Scheduled Reminder form.

[monishdeb]

@totten thanks for detailed report. I have addressed the first issue by adding an upgrade code for 5.49.0 and 2nd issue by adding pre and post upgrade message.

@agileware-justin I will address this bug in the master branch PR #23487

[totten]

Well... that was worth a try... In my r-run over each of the upgrade paths, it never seems to return TRUE, so it always skipped the step.

I think I found a hidden feature which can do what this is trying to do... but still putting it through the r-run...

[totten]

Just to clarify that a bit...

What this line seems to ask: "Is the start-revision >= 5.49.beta1?" But Domain::version() doesn't tell you the start-revision. (It tells you the last-stored incremental update, which is a rolling-value during the upgrade process.)

However, the information is available. The upgrade_N_N_N() method can actually receive three parameters (upgrade_5_49_2($currentRev, $startRev, $finalRev)). The $startRev parameter is what this was looking for (aff7086).

[monishdeb]

Awesome, didn't knw that

[totten]

Replaced by #23490, which starts from similar commits but adds a few more.