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

Allow Owners to Create new Owners for teams #237

Closed
ehazlett opened this Issue Jul 11, 2013 · 32 comments

Comments

Projects
None yet
5 participants
@ehazlett
Contributor

ehazlett commented Jul 11, 2013

NF related: Owners currently have abilities other users don't have such as deleting videos not just from the team but from Amara itself. Currently only Amara staff can create owners for teams, there should be a way for team owners to create more owners for their teams.

@ehazlett

This comment has been minimized.

Contributor

ehazlett commented Jul 11, 2013

From: Juliana Rincón Parra
NF related: Owners currently have abilities other users don't have such as deleting videos not just from the team but from Amara itself. Currently only Amara staff can create owners for teams, there should be a way for team owners to create more owners for their teams.

@ehazlett

This comment has been minimized.

Contributor

ehazlett commented Jul 11, 2013

From: Craig Zheng

@ehazlett

This comment has been minimized.

Contributor

ehazlett commented Jul 11, 2013

@bendk bendk added the known issue label Feb 19, 2016

@bendk bendk modified the milestone: Known Issues Feb 19, 2016

@asabine asabine closed this Feb 28, 2018

@asabine asabine reopened this Feb 28, 2018

@asabine asabine self-assigned this Feb 28, 2018

@asabine

This comment has been minimized.

Contributor

asabine commented Mar 1, 2018

team owners can now add new owners through the member page change role modal

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Mar 2, 2018

  • The API method now acts inconsistently with the UI - it does not allow owners to promote other users to owners. On attempt to do so, the response is 403 Forbidden, "detail": "Permission denied."

  • Regular admins (not owners) can demote owners. This does not make sense because owners are meant to be a special kind of admins with higher permissions, usually the users who are in charge of their teams and have a partner status or a paid AE account. How about not allowing admins to change the owners' role?

@asabine

This comment has been minimized.

Contributor

asabine commented Mar 2, 2018

added the permission check to the form and changed the permission itself which disallowed owners from adding owners, so the api should allow this now too and the form should prevent admins from changing owner roles

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Mar 3, 2018

With the latest changes, it is no longer possible for team owners to promote other users to owners or demote owners to junior members. The error message says "Member role not changed because you cannot change roles higher than your own".

@asabine

This comment has been minimized.

Contributor

asabine commented Mar 6, 2018

issue should be fixed, there was a typo in the permission check call so it returned the error if the check passed

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Mar 7, 2018

The current behaviour is that admins can promote / demote users below the admin level and owners can make any changes at any level (except their own account). I think this makes sense.

2 things about the UI:

  • If admins are not permitted to make other users admins, why do they have "Admin" as an option in the Change Role dialogue? Shouldn't it be hidden?

  • The error message when an admin attempts to demote another admin is not quite accurate. It says: "Member role not changed because you cannot change roles higher than your own". How about "Member role not changed because you cannot change roles that are equal to or higher than your own"?

@asabine

This comment has been minimized.

Contributor

asabine commented Mar 7, 2018

oh this is because of the role permission function; before this admins could promote/demote other admins through the modal. should we keep that behavior, or just change the above? in this case it'd make more sense to change the error message to "Member role not changed because you do not have permission to change this role" or something else generic like this, or should we have the extra specificity for this use case?

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Mar 7, 2018

"Member role not changed because you do not have permission to change this role" sounds great to me.

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Mar 8, 2018

Looks good now.

One last thing that is not exactly specific for this issue / ticket, but does not feel right and gets more exposure with the changes from this ticket: in new teams, after selecting single or multiple users and changing their roles, sometimes some members remain selected after the action. Sometimes the selected members are different from those whose roles the user was trying to change. This is a bit confusing and can result in accidentally changing a member's role in an unexpected way.

To reproduce, try to sign in as a team admin (not owner), bulk select several admins, owners, and junior team members, and change their role.

Would it be too hard to fix by making all the users on the page deselected after the action? Thanks!

@asabine asabine added this to the Sprint 35 milestone Mar 22, 2018

@asabine

This comment has been minimized.

Contributor

asabine commented Mar 22, 2018

might be fixed by #1842, if not we should make a separate issue for this probably

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Mar 23, 2018

gh-237 does not yet have the changes made in #1842, the latest code needs to be merged into the branch.

@kslottow kslottow modified the milestones: Sprint 35, Sprint 36 Apr 4, 2018

@asabine

This comment has been minimized.

Contributor

asabine commented Apr 5, 2018

fixed the merge conflicts, should be good now

@asabine asabine modified the milestones: Sprint 36, Sprint 37 Apr 17, 2018

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Apr 18, 2018

Sorry, but this one has been blocked by gh-enterprise-1624, which was dealing with the same functionality. Once staging (which is currently in a messy condition) gets sorted out, it should be merged into this branch so that it can be tested.

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Apr 23, 2018

I think it should be OK to merge staging into the branch now.

@asabine asabine modified the milestones: Sprint 37, Sprint 38 May 2, 2018

@asabine asabine modified the milestones: Sprint 38, Sprint 39 May 16, 2018

@asabine asabine modified the milestones: Sprint 39, Sprint 40 May 30, 2018

@asabine

This comment has been minimized.

Contributor

asabine commented Jun 12, 2018

just merged dev into the branch, should be good to go

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Jun 14, 2018

"Error submitting form" on attempt to change the team role for a user:

app_1 | ERROR 2018-06-14 10:16:41,849 teams.views global name 'is_owner' is not defined
app_1 | Traceback (most recent call last):
app_1 | File "/opt/apps/amara/apps/teams/new_views.py", line 286, in manage_members_form
app_1 | is_owner=is_owner, team=team)
app_1 | NameError: global name 'is_owner' is not defined
app_1 | ERROR 2018-06-14 10:16:41,851 unhandled_error Error in AJAX request
app_1 | Traceback (most recent call last):
app_1 | File "/usr/local/lib/python2.7/dist-packages/django/core/handlers/base.py", line 111, in get_response
app_1 | response = wrapped_callback(request, *callback_args, **callback_kwargs)
app_1 | File "/opt/apps/amara/apps/teams/new_views.py", line 113, in wrapper
app_1 | return view_func(request, team, *args, **kwargs)
app_1 | File "/opt/apps/amara/apps/teams/new_views.py", line 100, in wrapper
app_1 | return view_func(request, team, *args, **kwargs)
app_1 | File "/opt/apps/amara/apps/teams/new_views.py", line 232, in members
app_1 | return manage_members_form(request, team, form_name, members, page)
app_1 | File "/opt/apps/amara/apps/teams/new_views.py", line 292, in manage_members_form
app_1 | 'form': form,
app_1 | UnboundLocalError: local variable 'form' referenced before assignment
app_1 | INFO 2018-06-14 10:16:41,853 access GET /teams/collab-test/members/ 500 (0.029s)

@asabine

This comment has been minimized.

Contributor

asabine commented Jun 14, 2018

it should be good to go now; for some reason the is_owner assignment from a couple months ago before i stopped working on this for GDPR stuff was removed during one of the merges

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Jun 15, 2018

  • "Owners" should go in the list of available roles after "Admins" because the list is sorted by permissions level, from least (Contributors) to most.

  • Changing the role still explodes:
    app_1 | ERROR 2018-06-15 04:40:30,793 unhandled_error Error in AJAX request
    app_1 | Traceback (most recent call last):
    app_1 | File "/usr/local/lib/python2.7/dist-packages/django/core/handlers/base.py", line 111, in get_response
    app_1 | response = wrapped_callback(request, *callback_args, **callback_kwargs)
    app_1 | File "/opt/apps/amara/apps/teams/new_views.py", line 113, in wrapper
    app_1 | return view_func(request, team, *args, **kwargs)
    app_1 | File "/opt/apps/amara/apps/teams/new_views.py", line 100, in wrapper
    app_1 | return view_func(request, team, *args, **kwargs)
    app_1 | File "/opt/apps/amara/apps/teams/new_views.py", line 232, in members
    app_1 | return manage_members_form(request, team, form_name, members, page)
    app_1 | File "/opt/apps/amara/apps/teams/new_views.py", line 279, in manage_members_form
    app_1 | return render_management_form_submit(request, form)
    app_1 | File "/opt/apps/amara/apps/ui/views.py", line 78, in render_management_form_submit
    app_1 | form.submit()
    app_1 | File "/opt/apps/amara/apps/ui/forms/forms.py", line 134, in submit
    app_1 | self.perform_submit(self.iter_objects(progress_callback))
    app_1 | File "/opt/apps/amara/apps/teams/forms.py", line 1808, in perform_submit
    app_1 | elif not can_assign_role(member.team, self.user, role, member.user):
    app_1 | NameError: global name 'can_assign_role' is not defined
    app_1 | INFO 2018-06-15 04:40:30,815 access POST /teams/collab-test/members/ 500 (0.074s)

@asabine

This comment has been minimized.

Contributor

asabine commented Jun 21, 2018

forgot the permissions. before can_assign_role() 😅; should work now though

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Jun 23, 2018

  • "Owners" should go in the list of available roles after "Admins" because the list is sorted by permissions level, from least (Contributors) to most.

  • An owner cannot make another user a project or language manager - this causes the following error message "Member not changed because you do not have permission to change this role".

@asabine

This comment has been minimized.

Contributor

asabine commented Jun 25, 2018

both issues should be fixed; i changed the permission itself to let owners assign project and language managers, and just switched the place of owner and admin when adding to the list in the field setup

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Jun 26, 2018

  • Still a problem with the permissions - an admin cannot change the user role to Project / Language Manager, the error message says: "members not changed because you do not have permission to change these roles ".
@asabine

This comment has been minimized.

Contributor

asabine commented Jun 26, 2018

ah, i thought it was only owners who were supposed to be allowed to do this, not admins too.

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Jun 28, 2018

An owner can demote herself through API. She shouldn't.

@asabine

This comment has been minimized.

Contributor

asabine commented Jun 28, 2018

do we have these requirements written down anywhere? also, should admins be able to demote themselves? and if not, should any user be able to change their own role, and should this be enforced site-wide (via permissions), or only in the API (via serializer view)?

@asabine

This comment has been minimized.

Contributor

asabine commented Jun 28, 2018

it'd be helpful to know what we should or shouldn't allow in cases like this where we're altering permissions, especially when it's not enforced application-wide

@asabine asabine modified the milestones: Sprint 40, Sprint 42 Jun 28, 2018

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Jun 28, 2018

A team member (owner, admin, or any other role) should not be able to change their role either through the UI or through the API. Admins can appoint or demote managers and below. Owners can also appoint or demote admins and owners, except their own role.

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Jun 29, 2018

Verified.

danantonioreyes added a commit that referenced this issue Jul 3, 2018

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Jul 5, 2018

Deployed.

@PCF-Testing PCF-Testing closed this Jul 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment