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

(REF) Schema - Correct boolean fields in various tables #23177

Merged
merged 1 commit into from
Apr 14, 2022

Conversation

monishdeb
Copy link
Member

Overview

Followup PR #23134

@civibot
Copy link

civibot bot commented Apr 12, 2022

(Standard links)

@civibot civibot bot added the master label Apr 12, 2022
@monishdeb
Copy link
Member Author

@colemanw @demeritcowboy after reviewing each table boolean column I have prepared this patch. Can you please review this PR?

@@ -1,4 +1,6 @@
<?php
return [
// DAO changed, but nothing in upgrader?
'civicrm_acl_entity_role' => [
'is_active' => "DEFAULT 1 COMMENT 'Is this property active?",
Copy link
Contributor

Choose a reason for hiding this comment

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

The test fail is because there's a single-quote missing near the end here.

@demeritcowboy
Copy link
Contributor

I haven't r-run yet but I also see this, where there's conflicting defaults:

<default>1</default>
<comment>whether this status type is active</comment>
<default>0</default>

And then it doesn't cause a problem but both of these fields have duplicate defaults:

<default>1</default>
<required>true</required>
<comment>Is this processor active?</comment>
<add>1.8</add>
<default>1</default>
</field>
<field>
<name>is_default</name>
<title>Processor Type is Default?</title>
<type>boolean</type>
<default>0</default>
<required>true</required>
<comment>Is this processor the default?</comment>
<add>1.8</add>
<default>0</default>

@demeritcowboy
Copy link
Contributor

Thanks @monishdeb. I think there's still a problem somewhere - on upgrade all the civicrm_menu.is_public entries get set to 1. I'll try to debug.

And also sorry just noticing this should be against 5.49.

@demeritcowboy
Copy link
Contributor

Ok this line should be removed:

'is_public' => "DEFAULT 1 COMMENT 'Is this menu accessible to the public?'",

@eileenmcnaughton eileenmcnaughton changed the base branch from master to 5.49 April 12, 2022 23:12
@civibot civibot bot added 5.49 and removed master labels Apr 12, 2022
@eileenmcnaughton
Copy link
Contributor

@monishdeb I've updated the base branch to 5.49 so we don't merge to master - obviously it has a lot to rebase out now...

@eileenmcnaughton
Copy link
Contributor

@monishdeb I've put #2319 into a separate PR - as it seems to e causing much confusion - this is obviously needing rebase but we can maybe merge that first -

@monishdeb monishdeb force-pushed the booleanFields_1 branch 2 times, most recently from 525215c to caf0e2a Compare April 14, 2022 04:15
@monishdeb
Copy link
Member Author

@eileenmcnaughton I have rebased the PR. Also, should we also remove the requiredness of menu.is_public ?

@eileenmcnaughton
Copy link
Contributor

@monishdeb looks better!

I think it is correct for booleans to be required - as long as we also have a default & do the upgrade script thing

@monishdeb
Copy link
Member Author

ok reverting the Menu stuff.

@monishdeb
Copy link
Member Author

Done. @eileenmcnaughton can you please review the PR?

@eileenmcnaughton
Copy link
Contributor

OK - I took a look at what is in this patch & all the changes make sense - I haven't verified that there is complete cover of all the changes that need to be made - only that what is in this patch looks right

@eileenmcnaughton eileenmcnaughton merged commit 8b332fb into civicrm:5.49 Apr 14, 2022
@monishdeb
Copy link
Member Author

Thanks @eileenmcnaughton !

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