[REF] Add in frontend fields for title and description of group Schem… #18599
Conversation
|
(Standard links)
|
0ab907c to
8fed56d
Compare
|
So this takes the existing precedent a little further - ie uf_group has frontend_title but not frontend_description - but I don't see a specific issue with this extra wriggle down the slippery slope. Likewise the precedent in uf_group is that title is 64 chars not 255 but I guess that's a half-wriggle. I will include in the dev-digest but I doubt there will be push back. I have not tested the upgrade |
|
The upgrade test should be covered enough by our upgrade tests (covers multilingual and not) |
CRM/Core/I18n/SchemaStructure.php
Outdated
| 'title' => "varchar(64) COMMENT 'Name of Group.'", | ||
| 'title' => "varchar(255) COMMENT 'Name of Group.'", | ||
| 'frontend_title' => "varchar(255) DEFAULT NULL COMMENT 'Alternate Public title for this Group.'", | ||
| 'frontend_description' => "text DEFAULT NULL COMMENT 'Optional verbose public description of the group.'", |
There was a problem hiding this comment.
Suggest to standardise the language here. Are they both optional - then say so? What does Alternate mean? I think it should be Alternative; Alternate is a verb suggesting alternating between one and another, but we're meaning an alternative; use in preference to; override, I think. And I think the override is optional.
There was a problem hiding this comment.
Yes they are both optional, maybe instead of optional should be Alternative Public ... ? would that work for you @artfulrobot ?
| @@ -32,7 +32,7 @@ | |||
| <name>title</name> | |||
| <type>varchar</type> | |||
| <title>Group Title</title> | |||
| <length>64</length> | |||
| <length>255</length> | |||
There was a problem hiding this comment.
(I'm really glad to see this here!)
Shouldn't the title be NOT NULL as well?
There was a problem hiding this comment.
@artfulrobot good point probably just set it to be required
|
Thanks @seamuslee001 I think this is a really valuable new feature and looks sensible to me. We should ping the GDPR extension people on this as they implement their own version and might like to deprecate that feature in favour of this. I'll do that on chat. |
|
@artfulrobot yeh so to be clear all this will do is add the columns, A separate PR or PRs would be required to start implementing these fields appropriately but yeh |
|
Yep,got that, but I like the direction. |
|
@seamuslee001 we didn't get any other response from including this in the dev digest so go ahead & merge once @artfulrobot is happy with this |
8fed56d to
33cb05b
Compare
|
@artfulrobot have updated the language can you check your good with that now? |
3afa67d to
ee4aa29
Compare
|
Jenkins re test this please |
|
@seamuslee001 Thank you, I think that's much clearer now. |
…a only change Add in upgrade step to expand title column to be 255 as well Update langauge as per Artfulrobot and require group title field
ee4aa29 to
35fd9d2
Compare
|
|
|
Hmm - I think this may be mistaken, and just coming up against a db schema issue. I'll have to dig a bit deeper to find the problem. |
…a only change
Overview
This makes schema only changes to add in 2 new fields frontend_title and frontend_description. The end purpose would be to hopefully eventually use these fields on the public signup forms and unsubscribe forms but for the moment just making the schema level changes
Before
Only one set of fields for title and description
After
a public title and public description separate from internal
ping @eileenmcnaughton