Skip to content
Snippets Groups Projects

Hide tables in Users admin page if empty

Closed Caleb Cooper requested to merge calebcooper-hide-empty-tables-admin-users into master

What does this MR do and why?

This MR hides tabs that include no users to avoid requiring a scrollbar to see all tabs in cases where not all categories are used.

Comes from a large MR: !77593 (closed).

Screenshots or screen recordings

Current view:

Screenshot_2022-01-11_17-57-27

Proposed view:

Screenshot_2022-01-11_17-29-09

Proposed view after an unused tab (blocked) is removed:

Screenshot_2022-01-11_17-29-51

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Daniel Mora

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
  • notes: When changing existing functionality it is expected that tests will fail. You will almost always want to also look into adding additional specs to ensure your newly changed functionality is covered.

    To discover which tests are failing you can take a look at the Pipelines tab on your MR and note the Red :x: in the list of jobs. Clicking that will open a dropdown and put the failing specs at the top. You then can check the logs to see what specs are failing and where in the code they live.

    This pipeline shows we have two failing specs:

    Finished in 24 minutes 50 seconds (files took 45.3 seconds to load)
    219 examples, 2 failures, 5 pending
    Failed examples:
    rspec ./spec/features/admin/users/users_spec.rb:78 # Admin::Users GET /admin/users tabs has multiple tabs to filter users
    rspec ./spec/features/admin/users/users_spec.rb:94 # Admin::Users GET /admin/users tabs `Pending approval` tab shows the `Pending approval` tab

    Both of which are in the spec/features/admin/users/users_spec.rb.

    Typically you will run the specs locally to get an idea of what is going wrong. This can be done with the following command from your GitLab directory: bin/rspec spec/features/admin/users/users_spec.rb

    Naturally the failing specs are with the Tabs as currently they are expecting all of them to be there. You will want to modify the specs to add some expectations of when tabs are there and when they are not (ie. when a user exists).

    I would take a look at how other tests are written in that file. You will note something like:

      before do
        create(:user, name: 'Dmitriy')
      end

    This would be the basis of how you mock a user, you could then add some specifics for a particular tab and expect it to be there.

    I would suggest along with looking at similar specs in that file, also taking a look at:

    1. The Capybara Documentation
    2. Our GitLab Testing Documentation
  • Zack Cuddy removed review request for @zcuddy

    removed review request for @zcuddy

  • Caleb Cooper added 2227 commits

    added 2227 commits

    Compare with previous version

  • Caleb Cooper added 1 commit

    added 1 commit

    • 49cfb944 - Hide tables in Users admin page if empty

    Compare with previous version

  • Caleb Cooper added 1 commit

    added 1 commit

    • ab43d23e - Hide tables in Users admin page if empty

    Compare with previous version

  • Caleb Cooper requested review from @zcuddy

    requested review from @zcuddy

  • Caleb Cooper requested review from @syasonik

    requested review from @syasonik

  • Caleb Cooper resolved all threads

    resolved all threads

    • issue (UX Minor): There is a minor issue when "moving" the last user in a particular tab. When doing so the filter remains and you end up in odd state of the UI where you have no active tabs and no users found. This is expected as the tabs are simply hidden and there isn't any guards against valid tabs with 0 users.

      Video of issue

      Ux_Issue

      Since the UI does handle it gracefully enough I am leaning towards being okay with addressing this in a follow up.

      However, this a significant change to the Admin/Users UI anyways so we could do with a UX review and get their thoughts.

      Adding ~"group::workspace", devopsmanage, and sectiondev to properly align this change.

      Hey @npost! I found you via the Workspace Group Page and was wondering if you'd be available for a UX review on this change to the Admin/Users view. Noting the minor issue I have mentioned at the top of this thread! Please let me know if you are the incorrect team member for this :bow:

      Edited by Zack Cuddy
    • Contributor

      @zcuddy - thanks for the ping.

      @dmoraBerlin - you've also worked on this area. It would be great to get your insight too.

      My thinking... If a tab is empty, can we refresh the page and navigate to the furthest left tab (active) by default?

    • @npost I believe there was a rejection of hiding tabs by @gitlab-com/gitlab-ux/ux-foundations on the idea of not hiding content?

      cc: @peterhegman do you remember this?

    • @peterhegman yes, that was one issue, but for some reason it doesn't have the discussion I was thinking about?  :thinking: I think the question here is what happens if there is no content for any tab @calebcooper ?

    • @dmoraBerlin

      what happens if there is no content for any tab

      I don't think that would be possible since every instance will have at least one user. So there should always be at least one tab.

    • Author Contributor

      Yes, it is impossible for there to be no content in any tab since in order to view the page, one would need to be both active and an admin. That is why neither of those tabs get logic to check for their existence.

    • @npost I'm not seeing any reason not to proceed. Plus we've brought up the issue in the past, I think it makes sense to hide them. :thumbsup:

    • How will an admin know how many blocked users they have if they in fact have zero? It seems we would be obscuring this information from the admin.

    • @tauriedavis I just got pointed to your comment, I didn't see it as I was not directly mentioned. :grimacing: Apologies. I'm confused on the premise, if someone has nothing do we need to explicitly show them they have nothing in every case as mentioned above, "don't hide content, even if its empty/zero?" In this context, I'm not seeing the value of showing something that is 0 to take up space in the table. Which leads to the real point of the problem, how do we minimize tabs in tables to prevent overflow of data?

      cc: @badnewsblair

      Edited by Daniel Mora
    • @dmoraBerlin I can see value in knowing that there are 0 blocked users. Hiding the information could mean that an admin who thinks there is a blocked user cannot find where that blocked user should be in the UI.

      I think decreasing tabs means re-evaluating where we are using tabs vs. filters here.

    • @tauriedavis based on that, do you feel we should reject this MR in favor of a different solution?

    • @tauriedavis based on this would you reject this MR? What about a toggle to hide or show tabs that are 0?

    • @dmoraBerlin My recommendation would be to close the MR. We could introduce a toggle, but it introduces a UI element that feels like a patch rather than solving the initial issue of too many tabs. What do you think about exploring solutions to that issue outside of zero sums? The tabs would still be extensive when there are non-zero amounts.

    • FYI @mnichols1 this is an ongoing problem we have been investigating. I think at this point it is reasonable to hand over as we will not progress on hiding tabs and Taurie had another solution to investigate.

    • @dmoraBerlin Thank you for the heads up and your work on this.

      I agree hiding tabs is not something we should proceed with.

      @tauriedavis did you have an issue you wanted to continue investigating an alternative solution in, or shall I create one?

      Is there any further discussion needed or are we ok to close?

      cc @calebcooper

    • @dmoraBerlin Was there an issue to look at which tabs should be filters on the members page?

      @mnichols1 As for this MR, I think we are good to close.

      Edited by Taurie Davis
    • @tauriedavis @mnichols1 This was the original issue where we were thinking of moving tabs to filter queries.

    • @calebcooper Based on the above conversation, we will close this MR and solve this another way in the linked issue(s). Never the outcome someone wants we they create a merge request, but this has sparked a great conversation and will lead us to a solution so thank you for your work on this :pray: .

    • Please register or sign in to reply
  • added devopsmanage sectiondev + 1 deleted label

  • Zack Cuddy requested review from @npost

    requested review from @npost

    • question: I am unfamiliar with the pipeline:skip-undercoverage tag. Do we need this? If there is related undercovered code from this change I believe it should be addressed. I'll lean on @syasonik test review to give the final word here.

    • @zcuddy Hmmm, I'm not super sure on the pipeline:skip-undercoverage tag either...

      Looking at the failure in past pipelines, the undercoverage report errors during startup, so it hasn't been running overall. Still, it seems to me that the changes introduced in this MR have complete test coverage, so I'd be inclined to check with a maintainer on how to approach the scenario.

      I also wouldn't be surprised if it would work to remove the label & retry the job if it fails @calebcooper. Might be worth a shot to see if it can return meaningful output?

    • Author Contributor

      @syasonik Sure. Doing that now. I added that label because it was failing in a way which resembled this: #347269 (closed).

      Thank you for taking a look at this.

    • Author Contributor

      @syasonik The undercoverage error seems to be the last remaining failure. I have updated the code to include most recent changes to master.

    • Please register or sign in to reply
  • Zack Cuddy approved this merge request

    approved this merge request

  • Zack Cuddy requested review from @blabuschagne and removed review request for @npost and @zcuddy

    requested review from @blabuschagne and removed review request for @npost and @zcuddy

  • :wave: @zcuddy, thanks for approving this merge request.

    This is the first time the merge request is approved. To ensure full test coverage, a new pipeline has been started.

    For more info, please refer to the following links:

  • Zack Cuddy requested review from @npost

    requested review from @npost

  • Sarah Yasonik
  • Nice work @calebcooper! I think we'll want adjust the SQL queries a tiny bit, but this is a great change!

    We'll also need some changelog trailers on a commit before merging, as this is user-facing.

  • Sarah Yasonik changed milestone to %14.8

    changed milestone to %14.8

  • Sarah Yasonik removed review request for @syasonik

    removed review request for @syasonik

  • Brandon Labuschagne removed review request for @blabuschagne

    removed review request for @blabuschagne

  • Daniel Mora changed the description

    changed the description

  • Caleb Cooper mentioned in commit 52178fd6

    mentioned in commit 52178fd6

  • Caleb Cooper added 1 commit

    added 1 commit

    • 52178fd6 - Fixed banned user tab appearing when feature is disabled

    Compare with previous version

  • Caleb Cooper added 1203 commits

    added 1203 commits

    Compare with previous version

  • 🤖 GitLab Bot 🤖 changed milestone to %14.9

    changed milestone to %14.9

  • Caleb Cooper mentioned in commit 9a686ddb

    mentioned in commit 9a686ddb

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