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.

Sign up for GitHub

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

Jump to bottom

[REF] Add in upgrade step to populate missing contact_type.name field #17570

Merged
merged 1 commit into from
Jun 10, 2020
Merged

[REF] Add in upgrade step to populate missing contact_type.name field #17570

merged 1 commit into from
Jun 10, 2020

Conversation

seamuslee001
Copy link
Contributor

Overview

As picked up by @aydun there was no fixing up of hte current name column data when it was missing as requested by @totten in https://github.com/civicrm/civicrm-core/pull/17548/files

Before

No populating of the name column if empty

After

Upgrade script with test

ping @eileenmcnaughton

@civibot
Copy link

civibot bot commented Jun 10, 2020

(Standard links)

@civibot civibot bot added the master label Jun 10, 2020
aydun
aydun reviewed Jun 10, 2020
View reviewed changes
CRM/Upgrade/Incremental/php/FiveTwentyEight.php Outdated
$contactTypes = \Civi\Api4\ContactType::get()
->setCheckPermissions(FALSE)
->execute();
foreach ($contactTypes as $contactTypes) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't that be ($contactTypes as $contactType) ? And also $contactType (no 's') in the following lines for id and label?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks Fixed now

@seamuslee001 seamuslee001 force-pushed the conact_type_populated branch from 0268753 to e5f1ba9 Compare June 10, 2020 11:35
aydun
aydun reviewed Jun 10, 2020
View reviewed changes
CRM/Upgrade/Incremental/php/FiveTwentyEight.php Outdated
if (empty($contactType['name'])) {
\Civi\Api4\ContactType::update()
->addWhere('id', '=', $contactType['id'])
->addValue('name', ucfirst(CRM_Utils_String::munge($contactTypes['label'])))
Copy link
Contributor

Choose a reason for hiding this comment

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

$contactType['label'] not $contactTypes['label']

@seamuslee001 seamuslee001 force-pushed the conact_type_populated branch from e5f1ba9 to 471e466 Compare June 10, 2020 11:40
@aydun
Copy link
Contributor

aydun commented Jun 10, 2020

Should we also update any blank 'is_reserved' and 'is_active' since those defaults were also set in #17548 ?

@seamuslee001 seamuslee001 force-pushed the conact_type_populated branch from 471e466 to d638f47 Compare June 10, 2020 12:22
@seamuslee001
Copy link
Contributor Author

Hopefully done now @aydun

@seamuslee001 seamuslee001 force-pushed the conact_type_populated branch from d638f47 to 257383b Compare June 10, 2020 12:31
@aydun
Copy link
Contributor

aydun commented Jun 10, 2020

Looks good to me @seamuslee001 !

@seamuslee001 seamuslee001 force-pushed the conact_type_populated branch from 257383b to faf0012 Compare June 10, 2020 21:10
@eileenmcnaughton
Copy link
Contributor

Thanks @seamuslee001 and @aydun

@seamuslee001 seamuslee001 merged commit b5e99f5 into civicrm:master Jun 10, 2020
@seamuslee001 seamuslee001 deleted the conact_type_populated branch June 10, 2020 22:46
@agh1
Copy link
Contributor

agh1 commented Aug 5, 2020

Hi all just a side note here: I worry that adding [REF] to a PR like this that contains a database upgrade can obscure its impact. In preparing release notes, we threw this down with the Miscellany section, but it turns out to have prompted issues upgrading on some sites. I just wonder whether on principle a database upgrade might be incompatible with calling it a mere refactor.

@eileenmcnaughton
Copy link
Contributor

It was a follow up to a refactor but that is probably fair comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aydun aydun aydun left review comments

Assignees
No one assigned
Labels
master merge on pass
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@seamuslee001 @aydun @eileenmcnaughton @agh1