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

Add default for boolean fields on financial_type #19281

Merged
merged 1 commit into from
Dec 31, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Add default for boolean fiels on financial_type

Before

No default db value for boolean fields is_active and is_reserved on financial type table

After

defaults added (1 for is_active & 0 for is_reserved)

Technical Details

I'll add the upgrade separately

Comments

@civibot
Copy link

civibot bot commented Dec 29, 2020

(Standard links)

@civibot civibot bot added the master label Dec 29, 2020
@eileenmcnaughton eileenmcnaughton changed the title Add default for boolean fiels on financial_type Add default for boolean fields on financial_type Dec 29, 2020
@eileenmcnaughton
Copy link
Contributor Author

While non-blocking on this there is another question for the upgrade script - is the correct default for is_deductible 1 as per the schema or 0 as per the BAO - I suspect the latter & will need to fix that before doing the schema update @seamuslee001 @monishdeb

@@ -192,6 +193,7 @@ public static function &fields() {
'title' => ts('Financial Type Is Active?'),
'description' => ts('Is this property active?'),
'where' => 'civicrm_financial_type.is_active',
'default' => '1',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The BAO is actually defaulting to 0 at the moment - but that seems inconsistent with every other place we set a default for is_active and I propose switching to TRUE (removing the line in the BAO once this is merged)

@mattwire
Copy link
Contributor

This seems sensible. Please follow up with upgrader and other changes

@mattwire mattwire merged commit af5d7ae into civicrm:master Dec 31, 2020
@eileenmcnaughton eileenmcnaughton deleted the bool branch December 31, 2020 19:39
@eileenmcnaughton
Copy link
Contributor Author

@mattwire do you have an opinion on is_deductible?

@eileenmcnaughton
Copy link
Contributor Author

@mattwire @seamuslee001 this is the BAO change follow up - but it highlights 'is_deductible' & also raised another question for me #19296

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.

2 participants