-
-
Notifications
You must be signed in to change notification settings - Fork 815
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
Follow up fixes on population of new civicrm_mailing fields #30648
Conversation
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
https: //github.com/civicrm/civicrm-core/pull/30648 Bug: T368999 Change-Id: I8583387732bddf61255572e26726a4097b9c4744
@@ -33,7 +33,7 @@ public function upgrade_5_76_alpha1($rev): void { | |||
$this->addTask(ts('Upgrade DB to %1: SQL', [1 => $rev]), 'runSql', $rev); | |||
$this->addTask('Add start_date to civicrm_mailing table', 'addColumn', 'civicrm_mailing', 'start_date', "timestamp NULL DEFAULT NULL COMMENT 'date on which this mailing was started.'"); | |||
$this->addTask('Add end_date to civicrm_mailing table', 'addColumn', 'civicrm_mailing', 'end_date', "timestamp NULL DEFAULT NULL COMMENT 'date on which this mailing was completed.'"); | |||
$this->addTask('Add status to civicrm_mailing table', 'addColumn', 'civicrm_mailing', 'status', "varchar(12) DEFAULT NULL COMMENT 'The status of this Mailing'"); | |||
$this->addTask('Add status to civicrm_mailing table', 'addColumn', 'civicrm_mailing', 'status', "varchar(12) DEFAULT 'Draft' COMMENT 'The status of this Mailing'"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the length be 12 or 32? In a stock 5.76 install it's 32, although oddly has maxlength
12:
civicrm-core/schema/Mailing/Mailing.entityType.php
Lines 425 to 427 in f6718f2
'sql_type' => 'varchar(32)', | |
'description' => ts('The status of this mailing'), | |
'maxlength' => 12, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@demeritcowboy OK - I'll go with 32 for maxlength & here - seems like that makes sense
thanks @demeritcowboy |
This has been confusing me since yesterday as to why it wasn't crashing for me earlier. It does appear that at one point this PR had |
Overview
This is a follow on to #30458
After deploying the rc on our staging I discovered a couple of gaps
status
field has a default value in the schema but not the upgrade scriptcivicrm_mailing_job
set to 'Complete' but theis_completed
field was not set to TRUEBefore
some rows had
status
of NULL after the upgradeAfter
status
Technical Details
There was no noticeable time cost running this script on our staging site - unlike the the prior mailing table upgrades this script deals with the number of mailings & mailing jobs, not the recipients.
Comments