Hide tables in Users admin page if empty
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).
- related issue: Resolve "hidden" tabs visibility in Admin > Members list
Screenshots or screen recordings
Current view:
Proposed view:
Proposed view after an unused tab (blocked) is removed:
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.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
added Support Team Contributions label
requested review from @zcuddy
assigned to @calebcooper
- A deleted user
added frontend label
mentioned in merge request !77593 (closed)
Allure report
allure-report-publisher
generated test report for 3dac8cf7!review-qa-reliable:
test report
review-qa-smoke: test reportHey @calebcooper! Was this ready for review or you still working on this one?
Hello, @zcuddy. I believe this is ready for your review. Failing thing is a test, which I am not sure how to resolve.
I made the other two MRs based on the big one from before, but they are still in draft.
Great work @calebcooper! I had a question about the tabs as I think a tab was missed in the conditions. I also provided some details about specs that I hope can get you started with writing them. Let me know if you have any questions! Best of luck!
Glad to hear it @calebcooper! Thank you for sticking with this one, this is a great change
Taking a look now
Fantastic work here @calebcooper! frontend code looks good to me! I left a few other notes but I believe the other reviewers are better suited to address them. Approved
note: Danger has a few cleanup tasks to better align this issue that should be addressed: !78012 (comment 807647925)
Hey @blabuschagne! Do you have capacity for a frontend maintainer review here?
Thanks for the ping @zcuddy! @calebcooper the FE changes LGTM, there are however a good couple of great comments from @syasonik (and additional open threads) which need to be addressed before I can approve / merge.
In addition, this MR is quite far behind master. Perhaps rebasing your branch against master could help out with the pipeline failures.
- Resolved by Zack Cuddy
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
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:
removed review request for @zcuddy
added 2227 commits
-
dd2cdb3e...7d45e44f - 2224 commits from branch
master
- 9c39e7c4 - Merge branch 'master' into calebcooper-hide-empty-tables-admin-users
- 74a91e1f - Modified users_spec
- efc40e20 - Modified the users_spec file
Toggle commit list-
dd2cdb3e...7d45e44f - 2224 commits from branch
added pipeline:skip-undercoverage label
requested review from @zcuddy
requested review from @syasonik
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
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
Edited by Zack Cuddy@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?
I think the question here is what happens if there is no content for any tab @calebcooper ?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.
@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.
@tauriedavis I just got pointed to your comment, I didn't see it as I was not directly mentioned.
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 is0
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
.
added devopsmanage sectiondev + 1 deleted label
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?
@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.
@syasonik The undercoverage error seems to be the last remaining failure. I have updated the code to include most recent changes to master.
requested review from @blabuschagne and removed review request for @npost and @zcuddy
@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:
requested review from @npost
- Resolved by Caleb Cooper
- Resolved by Caleb Cooper
- Resolved by Caleb Cooper
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.
changed milestone to %14.8
added featureenhancement typefeature labels
removed review request for @syasonik
removed pipeline:skip-undercoverage label
removed review request for @blabuschagne
mentioned in commit 52178fd6
added 1 commit
- 52178fd6 - Fixed banned user tab appearing when feature is disabled
added 1203 commits
-
52178fd6...9cc0ed2a - 1202 commits from branch
master
- 3dac8cf7 - Hide tables in Users admin page if empty
-
52178fd6...9cc0ed2a - 1202 commits from branch
changed milestone to %14.9
added missed:14.8 label
mentioned in commit 9a686ddb