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/core#2773 Add an ACL to demo data #22377

Merged
merged 1 commit into from
Jan 5, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jan 5, 2022

Overview

Add an ACL to demo data

Before

A lot of work to set up an ACL user to test something

After

An ACL is created for 'members in group Advisory Board can edit members in group Summer programme volunteers'. A contact 'Jenny Ling jenny@example.com' is added to the Advisory board group

Technical Details

In conjunction with a patch to buildkit we wind up with a user 'advisor' password 'advisor' who does not have permission to view all contacts

Comments

@civibot
Copy link

civibot bot commented Jan 5, 2022

(Standard links)

@civibot civibot bot added the master label Jan 5, 2022
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-buildkit that referenced this pull request Jan 5, 2022
This creates a new user 'advisor' password 'advisor' with the email jenny@example.com.

In conjunction with civicrm/civicrm-core#22377 this new login gets ACL limited
access to contacts rather than 'view all' - this makes testing easier

Change-Id: I7959149478312d7d7eaa0394c5991f03db97aa3a
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-buildkit that referenced this pull request Jan 5, 2022
This creates a new user 'advisor' password 'advisor' with the email jenny@example.com.

In conjunction with civicrm/civicrm-core#22377 this new login gets ACL limited
access to contacts rather than 'view all' - this makes testing easier

Change-Id: I7959149478312d7d7eaa0394c5991f03db97aa3a
This creates an acl user (jenny@example.com) who can view members of the Volunteer group.
A change will also be needed in buildkit to add her user account
@colemanw
Copy link
Member

colemanw commented Jan 5, 2022

This will be really helpful for testing.
And one more reason why we should stop advertising "Include demo data" as an option in our installer!

@eileenmcnaughton eileenmcnaughton changed the title Add an ACL to demo data dev/core#2773 Add an ACL to demo data Jan 5, 2022
@eileenmcnaughton eileenmcnaughton merged commit b9fe55c into civicrm:master Jan 5, 2022
@eileenmcnaughton eileenmcnaughton deleted the format branch January 5, 2022 05:23
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-buildkit that referenced this pull request Jan 5, 2022
This creates a new user 'advisor' password 'advisor' with the email jenny@example.com.

In conjunction with civicrm/civicrm-core#22377 this new login gets ACL limited
access to contacts rather than 'view all' - this makes testing easier

Change-Id: I7959149478312d7d7eaa0394c5991f03db97aa3a
@totten
Copy link
Member

totten commented Jan 5, 2022

🎉 for having some ACLs for test/demo purposes.

(@colemanw) And one more reason why we should stop advertising "Include demo data" as an option in our installer!

This statement confuses me. I agree that sample data has a list of topics/issues - and that checkbox might be rethought - but I disagree that advertising/visibility during install is specifically problematic. Some advertising/visibility seems necessary if you consider that the audience of evaluators/learners lack the depth/experience to dig-up more obscure options.

Anyway, rather than bat around abstractions a hypothetical list of issues, here's a draft wiki to brainstorm a concrete list:

https://lab.civicrm.org/dev/core/-/wikis/sample-data

@eileenmcnaughton
Copy link
Contributor Author

@totten not strictly on sample data - but I have been wondering if we should have a separate 'dev install' in build kit - the things I'm thinking of are mostly settings (debug = on, environment = dev, escape by default = on) - which we would want to differ between our local environments and the evaluator demos - ie we want low tolerance on dev environments & low messiness on demos

@colemanw
Copy link
Member

colemanw commented Jan 5, 2022

Thanks @totten I've added one more issue to the list in that wiki (case/grant types are weird)

@totten
Copy link
Member

totten commented Jan 6, 2022

@colemanw Good one on case/grant types.

@eileenmcnaughton I agree it's an important distinction on default settings for local-dev and for evaluator-demo. FWIW, I've been using /etc/civicrm.settings.d/*.php to create files like this:

$GLOBALS['civicrm_setting']['domain']['debug_enabled'] = 1;
$GLOBALS['civicrm_setting']['domain']['backtrace'] = 1;
$GLOBALS['civicrm_setting']['domain']['mailing_backend'] = [
  'outBound_option' => 0, 
  'smtpServer' => 'localhost',
  'smtpPort' => 1025,
  'smtpAuth' => FALSE,
];

The big upshot is that it applies to any/all local builds (eg {wp,drupal,drupal8,backdrop}-{clean,demo}) - so you don't need to multiply #build-types ({wp,drupal,drupal8,backdrop}-{dev}). The big downside is that it doesn't present itself; you have to "discover" it by reading the manual.

(There are some other trade-offs. Edits to /etc/civicrm.settings.d go live instantaneously - and they survive rebuilds. There's no way to insert/modify entities. A handful of unit-tests are finnicky with common overrides - iirc JobProcessMailingTest gets upset if you override mailing_backend. )

the things I'm thinking of are mostly settings (debug = on, environment = dev, escape by default = on) -

  • Yeah, I personally use debug_enabled and backtrace all the time.
  • The current environment setting never clicked for me. (For me, environment=Development sounds worse than doing nothing. I have mailcatcher/mailhog which work nicely for isolating and logging mail. Setting environment=Development means that I would have to add runInNonProductionEnvironment=1 helter-skelter.)
  • I can see the appeal of "auto-enlist developer-builds as guinea-pigs for $EXPERIMENTAL_SETTING_X=TRUE". I would acquiesce to this if (as one of the guinea-pigs) I still had an easy way to say, "oops, no, my system needs EXPERIMENTAL_SETTING_X=FALSE for now."

FWIW, I do see+like how the terms dev/demo/staging/prod/etc ("environment") help to explain some common patterns. Maybe we could link-up "environment", "civibuild", and "common default settings" with something like https://lab.civicrm.org/dev/core/-/issues/3020

@eileenmcnaughton
Copy link
Contributor Author

@totten default modifiers is currently a define in civicrm.settings.php - the main reason for this as opposed to a setting is that I feel there is a track record for using defines for transitional things (where eventually we expect it to be 'how it is' rather than an option) - having said that I'm not uber-wedded to any approach - I just think it will quickly stabilise if most of us are using sites with it defined

@totten
Copy link
Member

totten commented Jan 6, 2022

FWIW, the current civicrm.settings.d can be used for define() or $civicrm_setting - eg define(CIVICRM_MAIL...).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants