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

Events visibility #4419

Merged
merged 14 commits into from Apr 23, 2020
Merged

Events visibility #4419

merged 14 commits into from Apr 23, 2020

Conversation

openprojects
Copy link
Contributor

@openprojects openprojects commented Apr 21, 2020

Show/hide Events in Category based on can_display

indico/modules/events/models/events.py Outdated Show resolved Hide resolved
indico/modules/events/models/events.py Outdated Show resolved Hide resolved
indico/modules/events/models/events.py Outdated Show resolved Hide resolved
indico/modules/categories/controllers/display.py Outdated Show resolved Hide resolved
indico/modules/categories/controllers/display.py Outdated Show resolved Hide resolved
indico/modules/categories/controllers/display.py Outdated Show resolved Hide resolved
indico/modules/events/models/events.py Show resolved Hide resolved
indico/modules/events/models/events.py Outdated Show resolved Hide resolved
@ThiefMaster
Copy link
Member

ThiefMaster commented Apr 21, 2020

What do you think about not checking for event management permissions but only category management permissions when determining whether to show hidden events anyway or not? This would allow making this much simpler as you could always hide events with visibility==0 unless you are a category manager (ie you can do this fully on the query level without and extra queries etc)!

I think this would cover the most common cases, and we could always show a notice on the protection page when setting the event's visibility to "Invisible" mentioning that only category managers can see the event, and others can only access it via a direct link or from their profile dashboard.

@openprojects
Copy link
Contributor Author

openprojects commented Apr 21, 2020

@ThiefMaster
Copy link
Member

ThiefMaster commented Apr 21, 2020

Yeah, depends on whether it's common to have managers that aren't category managers... I guess in your case this happens more often so having the event-level check probably indeed makes sense...

@openprojects
Copy link
Contributor Author

openprojects commented Apr 21, 2020

@openprojects openprojects requested a review from ThiefMaster Apr 21, 2020
indico/modules/categories/models/categories.py Outdated Show resolved Hide resolved
indico/modules/categories/models/categories.py Outdated Show resolved Hide resolved
indico/modules/events/models/events.py Outdated Show resolved Hide resolved
@openprojects openprojects requested a review from ThiefMaster Apr 21, 2020
@ThiefMaster ThiefMaster requested a review from plourenco Apr 21, 2020
@ThiefMaster
Copy link
Member

ThiefMaster commented Apr 22, 2020

You are not correctly checking for hidden events when loading past/future events (RHEventList).

@ThiefMaster
Copy link
Member

ThiefMaster commented Apr 22, 2020

Please update the field description of the event visibility field to mention that "Invisible" also hides the event from the category list

@openprojects
Copy link
Contributor Author

openprojects commented Apr 22, 2020

You are not correctly checking for hidden events when loading past/future events (RHEventList).

OK... from the category view I don't hit that class... where can I reproduce it?

@ThiefMaster
Copy link
Member

ThiefMaster commented Apr 22, 2020

Just move the event's date long enough in the past that it's part of the "past events" that are loaded with a separate request

@openprojects
Copy link
Contributor Author

openprojects commented Apr 22, 2020

Please update the field description of the event visibility field to mention that "Invisible" also hides the event from the category list

Sure. Should I also update the msgid in the .po files? and the relative translations too (BR,FR,ES)?

@ThiefMaster
Copy link
Member

ThiefMaster commented Apr 22, 2020

no, we do that later when coming closer to a release

@openprojects openprojects requested a review from ThiefMaster Apr 22, 2020
@openprojects
Copy link
Contributor Author

openprojects commented Apr 22, 2020

Is there anything else I have to change/fix in the PR?

@ThiefMaster
Copy link
Member

ThiefMaster commented Apr 22, 2020

no, looks good. i'll give it a last quick test tomorrow, add a changelog entry and then merge it

@ThiefMaster
Copy link
Member

ThiefMaster commented Apr 23, 2020

I pushed some small fixes/improvements; the most important one was including visibility in the list of queried columns when querying the events - without this there was one extra query for each event, making this whole check very slow for large categories.

@ThiefMaster ThiefMaster merged commit 9a36c3c into indico:master Apr 23, 2020
5 checks passed
@ThiefMaster ThiefMaster deleted the events-visibility branch Apr 23, 2020
openprojects added a commit to UNOG-Indico/indico-core that referenced this issue Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants