Skip to content
Snippets Groups Projects

Moved the members and groups to single option called members

Merged Jose Ivan Vargas requested to merge 25985-combine-members-and-groups-settings-pages into master

What does this MR do?

Are there points in the code the reviewer needs to double check?

Why was this MR needed?

Screenshots (if relevant)

Project members and groups

Groups_and_Project_Members

Project members with pagination ONProject_Members_with_pagination

Gear button gif

2016-12-28_09.56.22

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Closes #25985 (closed)

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Grzegorz Bizon
  • Grzegorz Bizon
  • 11 @project_members = @project_members.non_invite unless can?(current_user, :admin_project, @project)
    12
    13 group = @project.group
    14
    15 # group links
    16 @group_links = @project.project_group_links.all
    17
    18 @skip_groups = @group_links.pluck(:group_id)
    19 @skip_groups << @project.namespace_id unless @project.personal?
    20
    21 if group
    22 # We need `.where.not(user_id: nil)` here otherwise when a group has an
    23 # invitee, it would make the following query return 0 rows since a NULL
    24 # user_id would be present in the subquery
    25 # See http://stackoverflow.com/questions/129077/not-in-clause-and-null-values
    26 group_members = MembersFinder.new(@project_members, group).execute(current_user)
    • I wonder if extracting code from this conditional is in scope for this merge request. Is there a reason why we should do that now?

    • The reason I did it was because of the Rubocop rule that prevents the branch assignments count from going to high. So I changed it to a finder to have some of those extra "assignment slots" if you will. But I think you mentioned this in the previous diff about copy-pasting the instance variables. What would be a good way to solve this?

    • Also should we resolve this discussion for the time being in light of the recent decision towards the instance variables or is there another way to perhaps solve this?

    • Please register or sign in to reply
  • @jivanvl Nice work! It looks exactly how we decided about doing that!

    I wonder if we can solve problem with copying & pasting instance variables from old controllers to the new one, which becomes an aggregate for settings. This is a little cumbersome and error prone to do it that way. This was the most boring solution we could think about when talking with @smcgivern, but maybe there is a better way!

    I was thinking about introducing app/presenters/settings/ with settings presenter for each old controller/index action. Let me explain that using an example.

    In this merge request, we strive for creating a settings aggregate which combines settings we previously had in ProjectMembersController and GroupLinksController. Each controller we had, defined some instance variables in index action, which were then passed to index.html.haml view to generate forms. Now we want to decouple legacy views and instance variables from controller these elements were associated with.

    I wonder if having settings presenters would solve this problem. With this approach we can easily move settings forms around, because forms used to edit settings would be decoupled from controllers. Consider having something like below.

    app/presenters/settings/project_members_presenter.rb

    class Settings::ProjectMembersPresenter
      include Gitlab::View::Presenter
    
      def initialize(project, user, params = {})
         # ...
      end
    
      def group_links
        @project.project_group_links
      end
    
      def project_members
        if  can?(current_user, :admin_project, @project)
          @project.project_members
        else
          @project.project_members.non_invite
        end
      end
    
      # etc, method for each instance variable we had in old controller
      # and finally
     
      def to_partial_path
        'projects/project_members/index'
      end
    end

    With this approach we can have a can easily move settings around and render partials using:

    projects/settings/members_controller.rb

    def show
      @project_members_settings = Settings::ProjectMembersPresenter
        .new(project, current_user, params)
    end 

    projects/settings/members/show.html.haml

    .some_css_etc
      = render partial: @project_members_settings
    
    .some_other_selectors_for_subsequent_subsettings_group
      = render partial: @group_links_settings

    How do you like this idea? This actually would make it a little more difficult to finish this MR, from the backend perspective. We still do not have app/presenters, but I think that we already decided we need presenters, and this is yet another example why presenters can be useful.

    What do you think about this solution @smcgivern? /cc @DouweM @rymai

    Edited by Grzegorz Bizon
  • assigned to @jivanvl

  • added 1 commit

    • 268d613f - Updated some tests descriptions to represent the correct settings path

    Compare with previous version

  • @grzesiek I think I get how the presenter logic would work, but something that I'm a little bit of a loss here is. Are we going to add tests, if so how? By doing something similar on how you would test a controller?

  • @jivanvl : Could you add a screenshot/gif showing the updated cog navigation item and also a screenshot/gif of the new combined page? Thanks!

    cc @awhildy

  • @jivanvl Yes, we would need to add tests for both, controllers and presenters. Since presenters are a normal Ruby objects in this case, adding tests should not be too difficult. I think that we should hear feedback from @smcgivern or @rymai before we proceed with presenters here.

  • @jivanvl Another solution would be continue with copying & pasting instance variables, and submitting a ~"technical debt" issue about moving this to presenters and scheduling this issue for the next release. This may help to deliver this on time.

  • @grzesiek is anyone helping @jivanvl on the backend for this full-time? If not, I think your solution in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8281#note_20604329 is best. It would be nice to not implement presenters for the first time in the case of an MR like this, but I agree that it would make this simpler!

  • @awhildy @tauriedavis @victorwu Updated the description with screenshots and the gear button gif

    Edited by Jose Ivan Vargas
  • @smcgivern There is no backend developer assigned for a full time to help @jivanvl so I agree that this may be a little too much work for now. Let's continue without presenters, but let's make sure we have a proper tech debt issue for that. :thumbsup:

  • There are multiple instances where we use user when I think we mean to say member. Would we be able to update the copy in this MR and change user to member on this page to keep consistency?

    Edited by Taurie Davis
  • @tauriedavis There's only one place that I'm confused wether we should member or user. Personally I'm leaning towards user

    The part that I'm referring is this one do_we_use_member_here_

    Edited by Jose Ivan Vargas
  • @tauriedavis @awhildy I have a question, I noticed that there's two ways to delete groups from this single view

    What used to be the project members view

    Multiple_Groups_List

    What used to be the groups links view

    Multiple_Groups_List_2

    Do we create a separate issue for this or what do you think?

  • Do you mean in the search box? I think that should also be members, since members is used everywhere else. Why do you lean towards user?

    Do we need that bottom section (Groups you share with) of the groups view at all since it is above? I have ideas on how to improve the ux for this page. I think at this point we should make a separate issue so we have the space and time to discuss improvements. What are your thoughts @awhildy ?

    Edited by Taurie Davis
  • @tauriedavis I changed most of the users to members this is how it looks (didn't change the user select box) changed_from_users_to_members

  • @tauriedavis I feel because user was because you could invite any GitLab user to be a part of the project. But if member is already being used, I agree we should use member for consistency

  • Changed every match of user to member @tauriedavis

    All_users_to_members

  • Agreed that we should make a separate issue about cleaning up the UX for this page (to help keep this work scoped). It's great that there is some redundancy on the page (like the 'delete'). Means we can probably simplify it!

  • Thats a good point @jivanvl - And brings up a weird thing that is happening. It is strange that you can select current members in this dropdown and the button still says "Add to project" even when they are already in the project. There is some UX that can be improved here as well. I will include some improvement to this flow in the new issue as well.

    For now I think the search input should say "Search for members to update or invite"

    Edited by Taurie Davis
  • Thanks @jivanvl + @tauriedavis!

    Looking great to me! Yeah, like @awhildy says, let's make any simple copy tweaks and get this merged in, since the goal is navigation migration. I'm sure there's plenty of improvements needed in each settings page.

    Thanks @tauriedavis for creating the new issue(s).

  • @tauriedavis Updated the placeholder text for the search box Updated_members_search_box

  • added 1 commit

    • e7335a7a - Updated the "users" to "members" matches in the view

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • @grzesiek great! Do you mind creating that issue? :wink:

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading