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 - Fix boolean fields in various tables - Split commits #23134

Merged
merged 22 commits into from
Apr 8, 2022

Conversation

totten
Copy link
Member

@totten totten commented Apr 8, 2022

Overview

This is re-commit of #22954 which aims to improve debugging.

cc @demeritcowboy @monishdeb @colemanw

Before

Numerous fields changed - monolithic commit. Hard to bisect.

After

Numerous fields changed - broken into separate commits.

Comments

I also made a code-change, splitting the large array in FiveFortyNine.php into several smaller files. Some oddities struck me, so I added comments about those.

@civibot
Copy link

civibot bot commented Apr 8, 2022

(Standard links)

@civibot civibot bot added the master label Apr 8, 2022
@demeritcowboy
Copy link
Contributor

Thanks @totten. Would also like to revisit the reasoning behind any where before it was implicitly null and now default is explicitly 1. (e.g. menu.is_public)

@demeritcowboy
Copy link
Contributor

So I'm ok to merge as-is since should be a no-op. Was that the intent?

@demeritcowboy demeritcowboy added the merge ready PR will be merged after a few days if there are no objections label Apr 8, 2022
@totten
Copy link
Member Author

totten commented Apr 8, 2022

@demeritcowboy I share the presumption that DEFAULT null is more like DEFAULT 0, though the other PR has a good point about is_active as an exception. The challenge here is that we have actually had two people give a run-through, and so probably most of the decisions are right, but it's such a long list... that some oddities slip though (like civicrm_menu.is_public and civicrm_report_instance.is_active).

If we want an exercise to improve confidence in the list as a whole, these two possible exercises come to mind:

  • Get more eyes to run through all the changed fields. (Like we each give an additional +1/-1 read on a dozen items... maybe make a table to help divide-and-conquer that effort.)
  • Generate some kind of data table comparing the various XML/DAO/SQL/API defaults (and nullability) across both versions. Give extra review to anything with a substantive flip (like is_public).

@totten
Copy link
Member Author

totten commented Apr 8, 2022

So I'm ok to merge as-is since should be a no-op. Was that the intent?

Yeah, I'm down with that.

@demeritcowboy demeritcowboy merged commit 27bcef0 into civicrm:master Apr 8, 2022
@totten totten deleted the master-buggable branch April 8, 2022 06:08
@colemanw
Copy link
Member

colemanw commented Apr 8, 2022

@monishdeb can you please check the code comments @totten added in this PR and do a followup if necessary?

@demeritcowboy
Copy link
Contributor

Note also that menu.is_public is the cause of alert popups not popping up and the oversized styling that fields have now, e.g. on New Individual.

Untitled2

Untitled3

@colemanw
Copy link
Member

colemanw commented Apr 8, 2022

So that's a regression caused by #22954?
Ping @monishdeb

@demeritcowboy
Copy link
Contributor

Yes. See thread yesterday in prod-maint chat about weird styling.

@agileware-justin
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants