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/translation#58 - Fix regression. Partially revert schema for civicrm_group.title #18925

Merged
merged 3 commits into from
Nov 5, 2020

Conversation

totten
Copy link
Member

@totten totten commented Nov 5, 2020

Overview

This aims to address a recent regression in 5.31-rc. It partially reverts a change where #18599 marked the civicrm_group.title field as <required>true</required>.

It fixes a regression and failing unit-test that can be observed in bknix-min.

See also: https://lab.civicrm.org/dev/translation/-/issues/58

Before and After

  • 5.30: From a SQL POV, civicrm_group.title is not required (DEFAULT NULL)
  • 5.31 (pre-patch): From a SQL POV, civicrm_group.title is required (NOT NULL)
  • 5.31 (post-patch): From a SQL POV, civicrm_group.title is not required (DEFAULT NULL)

Comments

I'm not certain if a revert is workable - but don't see an obvious drawback. It's simple and worth trial.

Even with this schema-level patch, the UI (civicrm/group/add?reset=1) still treats the field as required.

@artfulrobot @seamuslee001 - in the discussion of #18599, was there a specific reason for flagging the field required (perhaps something discussed elsewhere)? From the Github note, it doesn't sound too important?

@bgm - any experience with similar bugs before? thoughts?

@civibot
Copy link

civibot bot commented Nov 5, 2020

(Standard links)

@civibot civibot bot added the 5.31 label Nov 5, 2020
@artfulrobot
Copy link
Contributor

@totten in my comment I suggested the title should be not null, and @seamuslee001 suggested to make it required.

I'd said not null because I didn't think we needed to differentiate between NULL and ''; I tend to see NULL-able columns as an optional feature for when you need that, rather than a default. So I was suggesting that it could be NOT NULL (and default to ''), which is different to suggesting it's required. But I probably did think it should be required - what use is a group without a title? (hmmm. "hidden smart groups" for search results that become mailings maybe...)

Anwyay, as far as I'm concerned, I have no reason to think it must be required; so I'm happy to revert it.

@seamuslee001
Copy link
Contributor

I'm fine to revert as well

@eileenmcnaughton eileenmcnaughton merged commit 3cbb42f into civicrm:5.31 Nov 5, 2020
@totten totten deleted the 5.31-revert-titlereq branch November 5, 2020 22:41
@totten
Copy link
Member Author

totten commented Nov 5, 2020

Thanks, @artfulrobot @seamuslee001.

I think you're right that the NOT NULL makes general sense from SQL modeling point-of-view -- so it was worth trying This interaction is a bit mysterious / hard-to-foresee, but it's helpful that we can back it out for now.

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