Moved the members and groups to single option called members

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

Project members with pagination ON

Gear button gif

Does this MR meet the acceptance criteria?

• Changelog entry added
• Documentation created/updated
• API support added
• Tests
  • Added for this feature/bug
  • All builds are passing
• Conform by the merge request performance guides
• Conform by the style guides
• Branch has no merge conflicts with master (if it does - rebase it please)
• Squashed related commits together

What are the relevant issue numbers?

Closes #25985 (closed)

changed title from WIP: Moved the members (project_members)option to a single controller called members to WIP: Moved the members and groups to single option called members

@jivanvl : Thanks for starting this! Are you planning to do both FE + BE all together in this one merge request?

@victorwu Yes I'll try to get both ready in this one MR

added 1 commit

• 4d959814 - Fixed rspec tests for the project members also fixed the index

Compare with previous version

added 1 commit

• a24a2497 - Fixed more rspec tests as well as spinach features

Compare with previous version

added 1 commit

• bea3afa3 - Added groups to members section, added a members finder

Compare with previous version

Thanks @jivanvl

added 1 commit

• 89e4fb06 - Removed the "Groups" option from the settings gear

Compare with previous version

added 1 commit

• 3fba18ee - Removed the "Groups" option from the settings gear

Compare with previous version

added 1 commit

• 2726e097 - Added tests for the MembersController and corrected some more tests

Compare with previous version

added 1 commit

• 8cfcf7c7 - Fixed typo for rubocop

Compare with previous version

added 1 commit

• 8cfcf7c7 - Fixed typo for rubocop

Compare with previous version

added 1 commit

• cccdea88 - Added tests for the MembersController and corrected some more tests

Compare with previous version

unmarked as a Work In Progress

marked the task Added for this feature/bug as completed

assigned to @grzesiek

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?

@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

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

cc @tauriedavis :)

@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

@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.

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?

@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

@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

What used to be the groups links view

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 ?

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

@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

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"

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).

New issue for improvements: https://gitlab.com/gitlab-org/gitlab-ce/issues/26167

@tauriedavis Updated the placeholder text for the search box

added 1 commit

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

Compare with previous version

added 1 commit

• 4b30457a - Fixed tests

Compare with previous version

@grzesiek great! Do you mind creating that issue?

mentioned in issue #26255 (moved)

@smcgivern @jivanvl I created the issue about project settings presenters - https://gitlab.com/gitlab-org/gitlab-ce/issues/26255.

assigned to @alfredo1

@alfredo1 Can you review the frontend on this one please? Thanks!

changed milestone to %8.16

@jivanvl Please add a CHANGELOG file

added 1 commit

• 490509c1 - Added CHANGELOG

Compare with previous version

@alfredo1 Added!

Looks good so far. Just a few questions for you @jivanvl

assigned to @jivanvl

@jivanvl also can you please rebase with master? I couldn't test without doing rebase.

added 359 commits

• 490509c1...365612ce - 348 commits from branch master
• 4cd139e9 - Moved the members (project_members)option to a single controller called members
• 68c730bb - Fixed rspec tests for the project members also fixed the index
• def6c43d - Fixed more rspec tests as well as spinach features
• ad58dec2 - Added groups to members section, added a members finder
• 57b5612a - Removed the "Groups" option from the settings gear
• cfd8f635 - Added tests for the MembersController and corrected some more tests
• 99c263ad - Updated some tests descriptions to represent the correct settings path
• 075aae25 - Updated the "users" to "members" matches in the view
• 90f18d9e - Fixed tests
• f243f1b2 - Added CHANGELOG
• 8f4f04c2 - Fixed es6 lint errors, and reverted a change to a helper

Compare with previous version

@alfredo1 Just did a rebase and fixed the helper thingie

assigned to @alfredo1

Some design review:

Padding is strange in this view:

Button is cut off at certain size when side nav is pinned:

There isn't a lot of space for these five columns at certain sizes and it feels smashes together:

Let's match the styling of the section below for all screen sizes. This should fix the padding, button sizes, and the number of columns at any given time:

The search/sort fields don't always fit. Let's move them to their own row below the table header so it doesn't cause overlap.

@tauriedavis I adjusted the sizes for the different viewports, I kinda like those fields on that position, but what do you think, do we keep this change?

@tauriedavis How about this for the styling? I'm not so sure about the import button

@awhildy @tauriedavis I noticed that on the search component for the project members, doesn't have an x when there's something typed should we consider creating a new issue?

added 1 commit

• 7056e926 - Added UX improvements

Compare with previous version

That gif looks good to me.

I think we can put the Import button next to the Add to project button. What do you think @awhildy ?

Yes, lets make a new issue to make the search boxes consistent throughout the site. https://gitlab.com/gitlab-org/gitlab-ce/issues/26303

Thanks @jivanvl for wrapping this up so quickly. Let's ship it with the light HTML+CSS tweaks that @tauriedavis specified. We can make it more awesome-er later on in different issues. Thanks!

cc @lbennett this is the MR for the weird test issue

mentioned in merge request !7771 (closed)

Agreed with @tauriedavis that the Import button should go next to the Add to project button. Thanks!

added 1 commit

• 5ce4883d - Fixed a test and change the arrow functions to named functions

Compare with previous version

@tauriedavis @awhildy Moved the import button!

added 1 commit

• b9227ad0 - Moved the "import" button next to the "Add to project" button

Compare with previous version

Thanks @jivanvl .

@jivanvl can you fix paddings on mobile?

as admin

as logged in user

assigned to @jivanvl

@alfredo1 how is this margin amount for the dropdowns look?

assigned to @alfredo1

added 1 commit

• 23e6d8c3 - Moved the "import" button next to the "Add to project" button

Compare with previous version

@jivanvl looks good!

merged

mentioned in commit 9cdfcbb5

Thanks @jivanvl !

Thanks @jivanvl and @alfredo1 !

This MR was missing an EE counterpart and had some conflicts that I attempted to resolve myself. However, I still got some feature failures: https://gitlab.com/gitlab-org/gitlab-ee/builds/8396579

mentioned in issue #26532 (moved)

@jivanvl @alfredo1 Was this merge request reviewed by a backend merge request endboss? I also appears that this was not reviewed by any backend mini/endboss after first, initial review iteration.

@grzesiek I don't think so. I thought your comments above were because you have been reviewing this. Is there a problem with this MR?

mentioned in issue #26745 (closed)

@alfredo1 It looks good actually. An endboss probably still might be able to find some things to improve, but the point here is that it should be reviewed again. @DouweM @rymai Do we have plan to use approvals feature? I've seen some ongoing work meant to improve approvals feature, can we enable in it CE/EE after it is done?

mentioned in issue #24865 (closed)

mentioned in issue #26165 (closed)

@alfredo1 @grzesiek this MR introduces a new class app/finders/members_finder.rb but I don't see spec/finders/members_finder_spec.rb for it. Thats bad. Every new class introduced should have a spec file

@dzaporozhets I agree! I created a new technical debt issue at https://gitlab.com/gitlab-org/gitlab-ce/issues/27962. Thanks for noticing that

@grzesiek @smcgivern any reason why we moved members index to separate controller under show action? Seems weird to me that Projects::ProjectMembersController#index is now Projects::Settings::MembersController#show.

At same time members page for group is still Groups::GroupMembersController#index.

Good point. Everything under Projects::Settings uses show as the default action. I think index would be more appropriate for all of these.

Isn't index action meant to be used for lists? show describes a single resource. Are "settings" a single resource or a list of things? Since we have a single form I would vote for the former, but since there "settings" is a plural word, maybe index would fit better.

Because we are now grouping multiple "settings" per page, index seems to be better indeed. Semantically "Settings::Members" feels like a single resource describing a single settings page for members, but this is indeed tricky.