CRM-20318: Add is_public setting on Custom groups #10028
Conversation
jitendrapurohit
commented
Mar 22, 2017
•
jitendrapurohit
commented
- CRM-20318: Add is_public setting on Custom Groups.
jitendrapurohit
changed the title
jitendrapurohit
force-pushed
the
is_public_group_fix
branch
from
a07ed6a
to
ec6ef23
Compare
|
@jitendrapurohit how does this interact with ACL rules that may get set? |
|
@seamuslee001 This isn't a setting w.r.t a user but a page. So if a custom field is created with is_public disabled, it will not be shown on public pages (currently supported only for Event Info Page). So if an ACL is created to show for specific user, this setting will override it. I thought of migrating this on an Event Form(say "Hide custom groups in info page") rather than on a Custom Group Form, since it is only Event Info Page where such custom field is being shown for now, but that will hold the setting as a whole(if checked all custom groups will be hidden) and not with per custom set(flexibility to show/hide specific set on the page) which is possible now. |
|
Thank you for this @jitendrapurohit! It's definitely an annoyance that creating event custom fields adds them to the "Event Info" page. |
|
@monishdeb Can you please review this and merge? |
| @@ -315,6 +315,8 @@ public function upgrade_4_7_16($rev) { | |||
| public function upgrade_4_7_18($rev) { | |||
| $this->addTask('Update Kenyan Provinces', 'updateKenyanProvinces'); | |||
| $this->addTask(ts('Upgrade DB to %1: SQL', array(1 => $rev)), 'runSql', $rev); | |||
| $this->addTask('Add is_public column to civicrm_custom_group', 'addColumn', | |||
| 'civicrm_custom_group', 'is_public', "boolean DEFAULT '1' COMMENT 'Is this property public?'"); | |||
There was a problem hiding this comment.
@jitendrapurohit could you move the upgrade changes to upgrade_4_7_19(...) here ?
There was a problem hiding this comment.
Done. Thanks!
jitendrapurohit
force-pushed
the
is_public_group_fix
branch
from
ec6ef23
to
2756229
Compare
|
@jitendrapurohit here's my report:
However, I found that CustomGroup DAO information is missing for that new column. This is my patch on that missing DAO info https://gist.github.com/monishdeb/8fd3eb1f3d91b2195f2ba02039074144 @eileenmcnaughton do we need to add a column in Custom group listing (url : /civicrm/admin/custom/group/?action=browse) page for this new field as |
|
I hope it is not too late to include (as I mentioned in the JIRA issue) that non-public groups will not be shown in the offline registration emails (another area custom groups will show aside of the event info page). If it is possible please include that, because otherwise this will give the administrator a false idea of non-disclosure to participants (e.g. internal notes will still be mailed to participants...) |
|
@monishdeb shouldn't that will be included during the build? I just didn't included as then it is more likely for a PR to get stale(due to GenCodeChecksum changes). I'll push the changes. |
jitendrapurohit
force-pushed
the
is_public_group_fix
branch
from
2756229
to
460931c
Compare
|
@magnolia61 will check on it and get back to you soon :) |
|
@magnolia61 Is that custom group created for event or participant ? As I don't see the event custom groups is sent in registration mails. |
|
@monishdeb @jitendrapurohit my understanding is this has been reviewed & approved & now the DAO changes have been pushed - so we should add merge on pass? @magnolia61 - I agree with you that there are situations where the emails contain inappropriate custom data & it would be good to exclude - I've also seen this in contribution & pledge emails. This will help towards that but not resolve it - we should add a follow on ticket for that. It's not trivial as it would also need unit tests A note re the field - there is precedent for both 'visibility' and 'is_public' in other tables, given that I don't have a strong feeling as to which is more appropriate. |
|
@eileenmcnaughton allright. no problem to separate this so the current PR can be merged. Shall I open a follow-up issue for that? |
|
@magnolia61 yes do that - I'm not sure what priority it will get. I see @jitendrapurohit might pick it up - but he is juggling a few priorities. |
|
@eileenmcnaughton I am happy with the final change. Merging now |
|
Just opened https://issues.civicrm.org/jira/browse/CRM-20507 for follow up on preventing mailing of is_public = 0 Custom Groups |
|
It would have been great if the 'is_public' field would also have been added to the appropriate views, not only to the table. Had to add the field manually to unbork our installation |
|
@kpanic23 You're absolutely correct - and https://issues.civicrm.org/jira/browse/CRM-20623 was raised to fix this exact issue. I definitely recommend upgrading against a test instance of your site first! Come talk to us at https://chat.civicrm.org if you're able to help out with testing release candidates or similar. |
