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#1879 Fix inconsistent ability to view event information #18712

Merged
merged 1 commit into from
Oct 29, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Oct 8, 2020

Overview

Alternate to #17852

Before

A contact with 'view event info' who ALSO has permission via acl-given permission to view a specific will ONLY see that specific even in 'upcoming events' eg http://dmaster.local/civicrm/event/list?reset=1

Screen Shot 2020-10-08 at 4 46 00 PM

After

Screen Shot 2020-10-08 at 4 44 41 PM

Technical Details

@alifrumin I took another look at your PR - I would up being happier with this short-circuit approach which the code comments were already hinting at. I think your change might have allowed contacts with no event access some.

Comments

@civibot
Copy link

civibot bot commented Oct 8, 2020

(Standard links)

@alifrumin
Copy link
Contributor

@eileenmcnaughton this works for me!!! I tested it and this fixes https://lab.civicrm.org/dev/core/-/issues/1879, I also looked at the code and it seems sensible to me.

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 @colemanw can one of your merge this reviewer's cut based on the above

@seamuslee001
Copy link
Contributor

So this is potentially exposing events to ACLed users right but they won't be able to make changes to those events they don't have access to right / see the participants?

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 this makes it so that providing event info screens (icals, upcoming listing) only require 'view event info' - this is not affecting viewing participants

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 did ^^ answer your question?

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 I think this is mergeable based on review + answer

@seamuslee001
Copy link
Contributor

ok I think as long as its documented that allows for view event info to bypass ACLs for the purposes for icals, and other feeds

@seamuslee001 seamuslee001 merged commit d6789d2 into civicrm:master Oct 29, 2020
@seamuslee001 seamuslee001 deleted the locev3 branch October 29, 2020 21:04
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