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

User can leave group from group page. #6274

Merged
merged 1 commit into from Feb 13, 2014

Conversation

4 participants
@cirosantilli
Contributor

cirosantilli commented Feb 8, 2014

Fixes ACCEPTING MERGE/PULL REQUESTS at http://feedback.gitlab.com/forums/176466-general/suggestions/5323575-allow-people-to-leave-a-group

Screenshot:

screenshot from 2014-02-08 14 36 15

Implementation notes

Moved some of the "who can remove who from a group" logic from templates into new functions of users_groups model.

Tests:

  • named the user and the groups. This led to mass refactoring of the group tests, but what was tested in all the existing tests was kept.
  • merged create group, which had a single test, into the group tests.
  • moved group test out of group directory since there is a single file now.
@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Feb 10, 2014

Sorry for silliness in opening the already implemented idea of last user not being able to leave group.....

I have corrected it and squashed old commit: last owner cannot leave the group from the group members page either.

To make UI and logic uniform, on the Profile > Groups page, the leave button simply does not show if the user is the last owner:

screenshot from 2014-02-10 11 58 57

The profiles controller was modified to contain the exact same "who can remove whom from a group" logic as the group members page, now encapsulated in the can_be_removed_by method of the UsersGroup model.

Add one documentation line explaining it that the last owner cannot leave the group:

screenshot from 2014-02-10 12 01 15

Add tests to check the "user leaves group from the profile page" case.

@users_group.destroy
redirect_to(profile_groups_path, info: "You left #{group.name} group.")
else
return render_404

This comment has been minimized.

@dosire

dosire Feb 10, 2014

Member

Not sure, but don't we want to raise an error in this case?

This comment has been minimized.

@cirosantilli

cirosantilli Feb 10, 2014

Contributor

@dosire Sorry I don't quite understand what you mean. Isn't 404 an error?

The button does not show on the UI anymore if the user is the last owner, for uniformity with the Group > Members page, so it is never possible to press something and get a popup saying "you cant do that" anymore.

I have documented it in the docs.

This comment has been minimized.

@dosire

dosire Feb 10, 2014

Member

I thought 404 is for Not Found and this should be 403 Forbidden

This comment has been minimized.

@cirosantilli

cirosantilli Feb 10, 2014

Contributor

You are right, 403 is better.

@dosire

This comment has been minimized.

Member

dosire commented Feb 10, 2014

@randx Can you review this?

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Feb 10, 2014

Updated to return 403 instead of 404 on error.

@dosire

This comment has been minimized.

Member

dosire commented Feb 10, 2014

Thanks!

# Returns true iff this user / group pair can be destroyed by the remover user.
def can_be_removed_by(remover)
!group.last_owner?(remover) && (abilities.allowed?(remover, :manage_group, group) || (remover == user))

This comment has been minimized.

@dzaporozhets

dzaporozhets Feb 10, 2014

Member

too much conditions in one line

This comment has been minimized.

@cirosantilli

cirosantilli Feb 10, 2014

Contributor

ok, I'll split it when we decide on the other issue.

@@ -2,7 +2,7 @@ class UsersGroupsController < ApplicationController
before_filter :group
# Authorize
before_filter :authorize_admin_group!
before_filter :authorize_admin_group!, except: [:destroy]

This comment has been minimized.

@dzaporozhets

dzaporozhets Feb 10, 2014

Member

why not just check for admin group permissions at first and only then check for ability to remove owner.
Separating authorization from other logic will prevent security issues in future.
So keep this before filter. And check for removing owner inside user_group without abilities logic

This comment has been minimized.

@cirosantilli

cirosantilli Feb 10, 2014

Contributor

@randx The problem is that now you don't need to be the admin anymore to DELETE the destroy URL and remove users: you can remove yourself at that URL, even if you are not the admin.
If we keep the before filter, it will never enter destroy action, and you can't destroy yourself.

This comment has been minimized.

@cirosantilli

cirosantilli Feb 10, 2014

Contributor

You got me thinking, we now have 2 URLs to remove users from groups: one under /profile to remove self only, and one under group/members (currently on this commit to remove anyone). How about I make the removal from the profile DELETE the group URL instead? Then all group removal would be done at a single URL, which feels like a better REST API.

This comment has been minimized.

@dzaporozhets

dzaporozhets Feb 11, 2014

Member

I prefer having them separated. Then authorization looks prettier.
profile/leave
-> I can leave any group if im not the last owner
user_groups/destroy
-> I can manage group membership only if I have manager access

This comment has been minimized.

@cirosantilli

cirosantilli Feb 11, 2014

Contributor

OK. So should I leave this as it is, or should I modify the view so that when the user want to remove himself from group, the DELETE gets sent to profile instead of groups? I think this is not very pretty, since we'd have side by side buttons with same style that send requests to different URLs, but beauty is subjective.
If we redirect DELETE to profile, what should we do about the "who can remove whom" logic added under users_group model:

  • keep it because it is used on the view, to decide what to show, and on the controller. I prefer this option since it encapsulates logic on the model. Fat models, lean controllers.
  • leave it in the controllers, always show the buttons on the view, and open the error popup if someone clicks.

This comment has been minimized.

@cirosantilli

cirosantilli Feb 11, 2014

Contributor

@randx checking if I understand what you suggest:

  • make a helper for - unless member.owner && group.last_owner?(member), put it under application_helper and use it in the groups and profiles views to hide the button in that case.

    I feel this brings ability logic into views. I could move can_be_removed_by logic into the Ability model as an alternative, and use it in the views.

  • point the remove user button on the group view to the profile leave URL in case it is the current user being removed. Remove the except authentication from the group controller.

This comment has been minimized.

@dzaporozhets

dzaporozhets Feb 11, 2014

Member

@cirosantilli yes you understood what I said

This comment has been minimized.

@dzaporozhets

dzaporozhets Feb 11, 2014

Member

I could move can_be_removed_by logic into the Ability model

makes sense. So we can do like can?(current_user, :remove_member, member) anywhere

This comment has been minimized.

@cirosantilli

cirosantilli Feb 11, 2014

Contributor

OK! Its clear now. Sorry for so many questions! =) I'll start updating.

This comment has been minimized.

@dzaporozhets

dzaporozhets Feb 11, 2014

Member

@cirosantilli great. thank you

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Feb 12, 2014

@randx I tried to implement it how I understood we had agreed on:

  • the UI is unchanged from last commit (except for the confirmation popup messages)

  • moved can logic from UsersGroup into Abilities and used it consistently everywhere.

    New abilities were added for UsersGroup: :destroy and :modify, since now non admins can also destroy an user group pair.

  • on the group view, the leave button for self points to the profile path, other buttons point to users_group path.

    If an user leaves a group from the group page, he is redirected to profile_groups.

    The except: [:destroy] was removed from the groups_user controller.

In addition I have:

  • corrected the render_403 method to return a head only 403.

    Previously it was rendering an inexistent public/403 template, which would generate a server 5XX error instead.

    I did not write a template because currently it is not possible for users to reach any 403 via their browser address bar: only via non GET methods.

  • rebased. Killed John Smith. Long live John Doe!

Lets see how Travis feels about this one =)

@MrKeiKun

This comment has been minimized.

Contributor

MrKeiKun commented Feb 12, 2014

@cirosantilli

CI felt great XD

dzaporozhets added a commit that referenced this pull request Feb 13, 2014

@dzaporozhets dzaporozhets merged commit d41e404 into gitlabhq:master Feb 13, 2014

1 check passed

default The Travis CI build passed
Details
@dzaporozhets

This comment has been minimized.

Member

dzaporozhets commented Feb 13, 2014

@cirosantilli great! Thanks

@cirosantilli cirosantilli deleted the booktree:leave-group-from-members-page branch Feb 13, 2014

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