Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore civicrm_action_schedule.limit_to (5.49) #23490

Merged
merged 7 commits into from
May 19, 2022

Conversation

totten
Copy link
Member

@totten totten commented May 18, 2022

Overview

(Updated variant of @monishdeb's #23477. Equivalent to #23497 on 5.50-rc)

Restore the schema for field civicrm_action_schedule.limit_to to allow NULL values. Attempt to reinstate NULL values (where possible). Warn about any records which may have changed.

See also:

Before

  • limit_to is not nullable
  • limit_to cannot be saved with null
  • The upgrade (5.48.2 ==> 5.49.1) will munge data; it coerces from limit_to=null to 0.

After

  • limit_to is nullable (as in <=5.48)
  • limit_to can be saved with null
  • Upgrade behavior varies for these perspectives:
    • If you upgrade directly from 5.48.2 ==> 5.49.2, it properly preserves data.
    • If you upgrade indirectly from 5.48.2 ==> 5.49.1 ==> 5.49.2, then the first upgrade already munged data. The second upgrade remediates by:
      • Cleaning up some records (ie anything with limit_to=0 AND group_id=NULL AND recipient_manual=NULL)
      • Showing a message to highlight records that could be inaccurate. (Some records may still have limit_to=0 even though their 5.48

monishdeb and others added 5 commits May 18, 2022 07:51
I'm not sure what it even means to limit a scheduled-reminder to a "soft_credit_type".

But it's a thing - I can use the 5.48 GUI to create it. Here's how it looks in the DB (in 5.48):

+----+------------------------------------------+------------------+----------+------------------+-------------------+----------+
| id | title                                    | recipient        | limit_to | recipient_manual | recipient_listing | group_id |
+----+------------------------------------------+------------------+----------+------------------+-------------------+----------+
| 11 | Limit To - Soft Credit                   | soft_credit_type |        1 | NULL             | in_memory_of      |     NULL |
+----+------------------------------------------+------------------+----------+------------------+-------------------+----------+

The `changeBooleanColumnLimitTo()` was coercing down to `limit_to=null` because it lacked `recipient_manual` and `group_id`.
@civibot
Copy link

civibot bot commented May 18, 2022

(Standard links)

@civibot civibot bot added the 5.49 label May 18, 2022
@eileenmcnaughton
Copy link
Contributor

@totten please merge this when you are ready as it represents a reviewer commit / agreed messaging

totten added 2 commits May 18, 2022 17:12
1. This fixes a problem where the message shows at the opposite-of-correct time
   (ie it should display on builds that have already run 5.49.{beta,0,1}, but
   it actually displayed on builds that have not run 5.49.{beta,0,1})

2. This makes the pre+post messages match.
}

public function createLimitToMessage(): ?string {
$suspectRecords = CRM_Core_DAO::singleValueQuery("SELECT GROUP_CONCAT(id SEPARATOR \", #\") FROM civicrm_action_schedule WHERE limit_to=0 AND (recipient_manual IS NOT NULL OR group_id IS NOT NULL)");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice formatted ids :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants