Role Management #2705
Role Management #2705
Conversation
di
force-pushed
the
role-management
branch
2 times, most recently
from
c12177f to
1d9704e
Compare
|
Hi @di. I've just pushed some (fairly large) changes to the styles and the way that the manager login works. If it's ok with you, I'll keep working on this branch to continue this work, as it's not there yet. The main thing I currently need your help with is changing the way that the roles form works. My new design looks like this:
The added feature is that a user can change the role for an existing collaborator, rather than having to remove and then re-add them. Is it possible to add some new form code to enable this functionality? It would also be really awesome if we could make the 'username' text input searchable - the way GitHub does it:
I realise this second request is fairly large, so I'm happy to open a separate issue for this if you prefer :) |
|
@nlhkabu The reason the form was like that originally is because it's technically possible (right now, on legacy-pypi) for a user to have both the "Maintainer" and "Owner" role -- they are not mutually exclusive. I don't see any reason why a user would need to have both roles (since all the privileges a Maintainer has, an Owner also has) but since it's possible that we have users right now with both, I wanted this to be able to render correctly when that's the case. I think we could possibly add some logic in here that hides the "Maintainer" role if the "Owner" role also exists for that user. Presumably when we shut down legacy-pypi, we could run a migration that removes any "duplicate" roles so we won't need that logic forever.
I agree, that would be awesome, and I think it's doable. But we should probably save that for post-launch work since it's not critical. |
| <p class="package-description">{{ release.summary }}</p> | ||
| {% endif %} | ||
| <!-- todo: if logged in user can edit package --> | ||
| <a href="{{ request.route_path('manage.project.settings', name=project.normalized_name) }}" class="button button--highlight">Edit Project</a> |
There was a problem hiding this comment.
If we really want an edit link on this page, we'll need to do a client-side include so the page doesn't require the use of a session (mostly adding this as a note to myself)
I think doing this makes a lot of sense, I don't think it makes much sense at all for someone to be both an owner and a maintainer. |
This would be great. I absolutely agree - it makes no sense that someone should be able to have both roles... so if we can find a way of hiding this in the UI, that would be ideal.
Great! :D I'll open a ticket. |
|
@nlhkabu OK, I've made the select boxes functional now, and hidden any "duplicate" roles. I just realized that I still need to add |
|
hi @di - the role form is working well - thank you :) One small request - are we able to order the collaborators by when they were added to the project? In this example, I used Ernest's login - and then added Donald to the project. What was weird was that Donald wasn't added to the bottom of the list - but at the top above Ernest.
What do you think? Also, FYI, I have added quite a lot of UI stuff to this branch that needs more work: Overall UI
Project Options
Deleting a Project
Release options
Deleting a release
Overall the TODO list is:
Are we happy to merge this PR with dummy UI and make separate PRs for each of these issues? |
There was a problem hiding this comment.
Overall Looks sound.
Couple things I'd like to see:
TODO:strings in comments for temporary workarounds/etc. I see GitHub issues are filed, but it can be nice to just grep the codebase for a quick glance as well.- Route namespacing for
/manageviews, basically anything that is user editable should probably fall under that top level path.
Looking really good and I think we can get this out to keep making progress. Some functionality I note that it is missing (but obviously does not strictly fall into role management):
- Change Password. We have Password Resets in flight on another PR, but it seems prudent to allow a user to change their password without rolling over into their inbox
- Change Email Address. We currently allow this, and it's important to keep this self service.
warehouse/routes.py
Outdated
| factory="warehouse.accounts.models:UserFactory", | ||
| traverse="/{username}", | ||
| "manage.project.settings", | ||
| "/project/{name}/settings/", |
There was a problem hiding this comment.
should this and the following three routes be namespaces under /manage?
it feels valid to me to keep all "edit" activity under a single top level path.
perceived advantages:
- easier to spot/search in logs
- easier to manage routing/caching/etc in the layers above warehouse
|
@nlhkabu @ewdurbin responses to both of you below:
Currently we don't have the "date created" for any existing roles. We can certainly add it, but I'm not really sure what the ordering would look like when some roles have a timestamp and others don't, and anything we choose would probably be pretty confusing to the user without indicating why the sorting is the way it is. Currently it's alphabetical based on username, which I think is ideal for finding a given user in a long list of collaborators.
I'd like to keep this PR just focused on role management as much as we can. Perhaps we can feature-flag or comment out anything in the UI that is not actually working and re-enable it when we go to ship those issues?
I don't want to merge this with UI components that don't work, I think that there's a strong chance that they'll be around for a significant amount of time and confuse users.
Added
These are captured in #423 and should probably be a part of a separate PR which fully addresses that issue. |
There was a problem hiding this comment.
Assuming a decision on UI components is fleshed out, I'm good with the state here.
One note on sorting of collaborators, at the very least sorting by Owner, Maintainer and then alphabetical by name might make it a bit easier to consume a larger list of collaborators.
|
I've opened tickets for each of the missing UI components and commented them out (adding TODOs linking to the tickets) - so on my side, we're good to go. I agree with Ernest - maybe listing the owners first is better? I don't see it as a blocker though - let's merge this PR and open a separate ticket for this? |
Before, these were always UUID objects, but since #1329 this is stored as a string for session-based authentication only. To keep everything consistent, always use strings over UUID objects.
This gives the logged in user a place to manage their profile, and a place to manage their projects. Mostly stubbed out for now.
This no longer needs to be a client-side include because we can just edit it via profile management when the user is logged in.
Adding and deleting roles
This will always link to a project/release that is live, so it's never really a "Preview" per se. Also, this allows us to actually have a "Preview" some day when we allow for staged releases.
Fixes #2654, fixes #956.
This adds the ability for a project Owner to add/remove roles for their projects.
This adds two new logged-in views:
For individual projects, there are two logged-in views per project:
@nlhkabu Please feel free to reorganize/restyle as much as you want -- I just did the bare minimum to get this feature functional (especially, please revert 1d9704e).