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

Replace usage of is_admin:project filter in object lists #49

Closed
agjohnson opened this issue Nov 17, 2021 · 5 comments · Fixed by #404
Closed

Replace usage of is_admin:project filter in object lists #49

agjohnson opened this issue Nov 17, 2021 · 5 comments · Fixed by #404
Assignees
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code

Comments

@agjohnson
Copy link
Contributor

agjohnson commented Nov 17, 2021

I've updated this issue to be more specific. Originally, this was meant as a note from an previous change:

The original change was to update use of request.user|is_admin:project with a context variable is_project_admin -- which is the same call and return value, but cached instead of repeated in the template.

However, neither approach is usable when listing multiple projects or similar, where we need the original logic (request.user|is_admin:project). We can't use the context variable approach, as this still incurs the same performance issue as doing this in template.

We need another option for iterating over a list of objects and using is_admin.

@agjohnson agjohnson added Accepted Accepted issue on our roadmap Improvement Minor improvement to code labels Nov 19, 2021
@agjohnson agjohnson added this to the Public beta - community milestone Nov 19, 2021
@agjohnson
Copy link
Contributor Author

Note this is not usable in a listing view yet, where I have used is_admin the most so far. Noted in #128 but this code is responsible for most of the loading lag on some listing UIs.

@agjohnson agjohnson self-assigned this Dec 11, 2023
@humitos
Copy link
Member

humitos commented Dec 12, 2023

I think the is_project_admin is the wrong approach. It needs to be defined on the context for each view. Besides, it implies rewriting all the templates that are currently using is_admin:project to use the new variable from the context.

I'd say we should implement a ~5s cache on is_admin filter tag instead (https://github.com/readthedocs/readthedocs.org/blob/5a5bf41db92cca4d14c9657da2e2a6067a8b9b53/readthedocs/core/templatetags/privacy_tags.py#L13-L15). That doesn't need to rewrite anything and all the templates will be benefit immediately without any other change.

I mentioned something similar on the PR linked at (readthedocs/readthedocs.org#7277 (comment))

@agjohnson
Copy link
Contributor Author

Mentioned on #128, this approach doesn't quite solve the problem there. I'm not sure a cached admin check is the final solution we want either, but I do agree we should do something other than replace these checks with is_project_admin -- which also doesn't work to solve #128.

Ideally:

  • Templates and views all use the same approach
  • This approach performs well when iterating over multiple objects (project listing view), not just a single object (project detail view)

I'll repurpose this issue to be more clear that perhaps is_project_admin is not the best target, but we probably want to do something more than just caching.

@agjohnson agjohnson changed the title Use is_project_admin instead of is_admin:project filter Replace usage of is_admin:project filter in object lists Dec 12, 2023
@agjohnson agjohnson removed their assignment Dec 12, 2023
@agjohnson
Copy link
Contributor Author

Removed assignment for now, it's looking like this needs to be backend change.

agjohnson added a commit that referenced this issue Dec 12, 2023
This consolidates all of the build list checks for `is_admin:project`
into a single template variable for now. This replicates what we're
doing at the view level in other views.

Ongoing discussion for a long term approach is at #49

- Refs #128
agjohnson added a commit that referenced this issue Dec 13, 2023
This consolidates all of the build list checks for `is_admin:project`
into a single template variable for now. This replicates what we're
doing at the view level in other views.

Ongoing discussion for a long term approach is at #49

- Refs #128
@agjohnson
Copy link
Contributor Author

agjohnson commented Mar 1, 2024

Just noting that this is mostly to resolve:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants