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

dev/core#2659 Move group cleanup to latest release #20756

Merged
merged 1 commit into from
Jul 7, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

dev/core#2659 Move group cleanup to latest release

Before

A site upgrading from pre 5.20 would hit errors trying to process smart group updates due to fields being in the DAO but not yet the table.

After

The updates have been moved to 5.39 - after the fields are added

Technical Details

These updates will have no impact on sites that have already run them

Comments

@demeritcowboy

@civibot
Copy link

civibot bot commented Jul 3, 2021

(Standard links)

@civibot civibot bot added the master label Jul 3, 2021
@colemanw
Copy link
Member

colemanw commented Jul 3, 2021

A few versions back I went to a lot of trouble to fix api calls so they wouldn't fatal on not-yet-added fields during upgrade mode. Any idea why that didn't help this situation?

@eileenmcnaughton
Copy link
Contributor Author

@colemanw I didn't test - I figured this was a quick & easy way to address this. I'ts obviously a bit obscure from the report so I didn't want to put much time into it

Comment on lines -120 to -134
$this->addTask('Update smart groups where jcalendar fields have been converted to datepicker', 'updateSmartGroups', [
'datepickerConversion' => [
'birth_date',
'deceased_date',
'case_start_date',
'case_end_date',
'mailing_job_start_date',
'relationship_start_date',
'relationship_end_date',
'event',
'relation_active_period_date',
'created_date',
'modified_date',
],
]);
Copy link
Member

Choose a reason for hiding this comment

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

This task is missing from the new upgrade?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - not sure why I thought that didn't relate - added now

@eileenmcnaughton eileenmcnaughton force-pushed the 539 branch 2 times, most recently from a461f4d to a4856ea Compare July 4, 2021 00:47
@eileenmcnaughton eileenmcnaughton changed the base branch from master to 5.39 July 4, 2021 00:47
@civibot civibot bot added 5.39 and removed master labels Jul 4, 2021
@eileenmcnaughton
Copy link
Contributor Author

test this please

@demeritcowboy
Copy link
Contributor

Still confused: Nobody has reproduced the problem with or without smart groups, and it's unlikely to be investigated further or have this patch tested on the original site, which may even be a different issue (we don't have confirmation of which field or even table "No such field" is referring to on that site).

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy yeah - it feels like this would fix it - but as you say we haven't reproduced

@demeritcowboy
Copy link
Contributor

And Jenkins is making you work for it...

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy yeah! - not the quick fix I'd hoped

@demeritcowboy
Copy link
Contributor

Failure is an intermittent fail which is fixed in 5.40 but this is against 5.39. But still not sure whether this is needed/desired.

@eileenmcnaughton
Copy link
Contributor Author

Oh well if it's intermittent let's get the green tick so we can decide without a red cross in our face

@eileenmcnaughton
Copy link
Contributor Author

test this please

@totten
Copy link
Member

totten commented Jul 7, 2021

Spot-checked several of the conversion functions - agree that they appear idempotent. (TLDR: Once you rename a smart-group criterion away from an old name... it no longer matches the old name... so it shouldn't match again...)

There are some intervening changes in the civicrm_saved_search schema (circa FiveThirtySeven, FiveThirtySix, FiveThirtyTwo), but they don't appear problematic. (They're adding extra columns/indices/constraints, but they don't appear to involve substantive differences in the form_values or id - which are the relevant fields forCRM_Upgrade_Incremental_SmartGroups.)

@totten totten merged commit 76767f8 into civicrm:5.39 Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants