-
Notifications
You must be signed in to change notification settings - Fork 529
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
Add banner for project managers, give priority to roles within locale #3422
base: main
Are you sure you want to change the base?
Conversation
Deleted the previous comment because I realize I was testing the same code 🤦🏼 Follow-up on conversation, I tested with
|
The second commit fixes #3425 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Added a few suggestions for minor improvements, and suggested a couple of ways forward for the PM status.
self.translation.entity.resource.project | ||
if self.translation | ||
else self.entity.resource.project | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not very pretty and not very fast. Similarly, the path to get to project
in pontoon/base/views.py, while a bit prettier, is still slow.
There are ways to get to the project object in a more scalable way, e.g. by passing the project from frontend all the way to here or by passing contact = entity.resource.project.contact
instead.
My personal preference would be to just use user.groups.filter(name="project_managers").exists()
in user_status()
, which would show all PMs, regardless of the project, as PMs.
I think that's even better, because when PMs of other projects add translations or comments, they are effectively acting PMs (e.g. because they step in temporarily or because they have managed the project in the past).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My personal preference would be to just use user.groups.filter(name="project_managers").exists() in user_status(), which would show all PMs, regardless of the project, as PMs.
That kind of defies the main point of having the PM banner.
I want the PM banner to show up only within the project for which that user is set as PM, not in all of them.
An admin that is also a PM should show up as an Admin. A person that is not an Admin, but is set as a PM for a specific project, shouldn't be displayed as a PM for any other projects (sure, they're unlikely to do anything outside of their project, but we should still avoid presenting them as something they're not).
Regarding this being slow: can we live with it, considering it's only used when there are actually comments (so, not in the majority of cases)? We have 12k comments for almost 8M translations, and I bet most comments are in group of at least 2 (so, ballpark of 4–5k translations affected? I'm sure actual data can be extracted if we think it's important).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are ways to get to the project object in a more scalable way, e.g. by passing the project from frontend all the way to here or by passing contact = entity.resource.project.contact instead.
I'd be happy to have someone else pick up this PR and pursue this path, because I'm not sure that's something I can do with my current understanding of the codebase 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(investigating the passing contact = entity.resource.project.contact instead
in the meantime)
EDIT: (solved) The suggest code returns a
EDIT: removed the diff, because after putting some logging, I realize I was completely off. |
pontoon/base/views.py
Outdated
@@ -886,6 +886,7 @@ def user_data(request): | |||
"manager_for_locales": list( | |||
user.managed_locales.values_list("code", flat=True) | |||
), | |||
"pm_for_projects": user.contact_for.all(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This returns a ProjectQuerySet. Not sure if there's a better way to get a list of slugs besides list comprehension.
This adds a banner for users defined as "Project Manager" within a project. To reduce the space for confusion, I changed the tooltip of MNGR from "Manager" to "Team Manager". This also makes the priority of roles consistent between front-end and back-end: - If a user as a role within the locale (translator, manager), we use that for the banner. - If a user is set as PM, we use that even if the user is an Admin. Finally, this adds CSS variables for users, instead of reusing the ones for translation status.
This adds a banner for users defined as "Project Manager" within a project. To reduce the space for confusion, I changed the tooltip of MNGR from "Manager" to "Team Manager".
This also makes the priority of roles consistent between front-end and back-end:
Finally, this adds CSS variables for users, instead of reusing the ones for translation status.
Fixes #3418