Prevent extra API request after searching for transfer location
What does this MR do and why?
Related to #384966 (comment 1200823662)
Currently, when transferring a group, if you search and then select a group it will clear the dropdown and perform an API request to get all of the groups again. In most cases this causes one extra API request because most users probably don't open the dropdown again. It is also causing a flakey test due to a race condition. See #384966 (comment 1200823662) for an explanation.
This MR updates the logic so the groups are not fetched again until the user opens the dropdown again
Screenshots or screen recordings
Before | After |
---|---|
Screen_Recording_2022-12-08_at_1.03.14_PM | Screen_Recording_2022-12-12_at_11.45.41_PM |
How to set up and validate locally
- Navigate to a group
- Navigate to
Settings
->Advanced
->Transfer group
- Open the dropdown and search
- Choose an item
- Open the dropdown again
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
changed milestone to %15.7
added devopsmanage + 1 deleted label
assigned to @peterhegman
added sectiondev label
added maintenancepipelines typemaintenance labels
removed maintenancepipelines label
removed typemaintenance label
added flaky-testtransient bug label
- A deleted user
added bugperformance frontend typebug labels
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer frontend Doug Stull (
@dstull
) (UTC-5, 3 hours ahead of@peterhegman
)Natalia Tepluhina (
@ntepluhina
) (UTC+1, 9 hours ahead of@peterhegman
)test for spec/features/*
Valerie Burton (
@vburton
) (UTC-6, 2 hours ahead of@peterhegman
)Maintainer review is optional for test for spec/features/*
UX Becka Lippert (
@beckalippert
) (UTC-6, 2 hours ahead of@peterhegman
)Maintainer review is optional for UX To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
DangerBundle size analysis [beta]
This compares changes in bundle size for entry points between the commits 0b949b08 and d70ad1ed
Special assetsEntrypoint / Name Size before Size after Diff Diff in percent average 3.53 MB 3.53 MB - 0.0 % mainChunk 1.95 MB 1.95 MB - 0.0 %
Note: We do not have exact data for 0b949b08. So we have used data from: cdaa1710.
The intended commit has no webpack pipeline, so we chose the last commit with one before it.Please look at the full report for more details
Read more about how this report works.
Generated by
Dangeradded UX label
Please wait for Reviewer Roulette to suggest a designer for UX review, and then assign them as Reviewer. This helps evenly distribute reviews across UX.
This message was generated automatically. You're welcome to improve it.
@afracazo can you please review the UX and let me know what you think of this change? It is the simplest way to fix the flaky test and reduce the number of API calls. Do we have any guidelines around the search resetting or not when choosing an item?
requested review from @afracazo
mentioned in issue #384966 (closed)
Allure report
allure-report-publisher
generated test report!e2e-review-qa:
test report for d70ad1edexpand test summary
+-----------------------------------------------------------------------------------------+ | suites summary | +------------------------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------------------------+--------+--------+---------+-------+-------+--------+ | Plan | 49 | 0 | 1 | 0 | 50 | ✅ | | Verify | 12 | 0 | 1 | 0 | 13 | ✅ | | Manage | 39 | 0 | 4 | 6 | 43 | ❗ | | Create | 28 | 0 | 1 | 0 | 29 | ✅ | | Govern | 15 | 0 | 5 | 3 | 20 | ❗ | | Version sanity check | 0 | 0 | 1 | 0 | 1 | ➖ | | Configure | 0 | 0 | 1 | 0 | 1 | ➖ | | Feature flag handler sanity checks | 9 | 0 | 0 | 0 | 9 | ✅ | | Package | 0 | 0 | 1 | 0 | 1 | ➖ | +------------------------------------+--------+--------+---------+-------+-------+--------+ | Total | 152 | 0 | 15 | 9 | 167 | ❗ | +------------------------------------+--------+--------+---------+-------+-------+--------+
- Resolved by Miguel Rincon
Hi @peterhegman, thanks for tagging me on this issue! I haven't found anything in Pajamas that would make this behaviour non-compliant, and also asked other designers about this since I'm pretty new at GitLab in case I wasn't aware. A question raised by one of the designers in the group was if you "considered clearing the dropdown at the point of clicking to open? Then there would be no visible change to the user, but we save our API call when not required."
added 598 commits
-
77c3428d...3a330abd - 597 commits from branch
master
- d70ad1ed - Don't clear transfer locations dropdown search when selecting an item
-
77c3428d...3a330abd - 597 commits from branch
@afracazo
, thanks for approving this merge request.This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.
For more info, please refer to the following links:
added pipeline:mr-approved label
removed review request for @afracazo
@mfluharty can you please review the frontend?
@vburton can you please review the test for
spec/features/*
?Thanks @peterhegman for making this change! test change LGTM
I also pulled down your branch locally to do some exploratory testing and ran the following specs several times in a row to observe if any flakiness occurred:
-
spec/features/groups/group_settings_spec.rb
-transfer group
block qa/qa/specs/features/browser_ui/1_manage/group/transfer_group_spec.rb
qa/qa/specs/features/browser_ui/1_manage/group/transfer_project_spec.rb
Specs had passed each time for me
One odd issue I noticed during manual testing was if the
transfer_locations
call returned a blank response twice in a row (ex: entering a group name that doesn't exist, and then start hitting backspace), the dropdown disappears entirely and the user has to refresh the page in order for it to come back again. This also happens onmaster
though and is outside the scope of these changes, so it is something I can make a separate issue for.-
@vburton thanks for the review and finding that bug. It does appear to be a pre-existing bug so I created #385786 (closed) to take care of this.
Looks good to me @peterhegman - approved!
@mrincon Could you do the maintainer review for this please?
Thanks all! However, maybe this actually didn't fix the flaky spec. I was surprised to see that the spec in question failed in the latest pipeline.
Based on my tests locally the extra API request we thought was causing the flaky spec is no longer firing
Screen_Recording_2022-12-21_at_9.55.33_AM
@stanhu since you looked into this previously is there anything you can think of that I might be missing?
cc @vburton
@peterhegman I haven't been able to reproduce the failure on this branch yet. Previously I was able to reproduce it consistently by precompiling assets and disabling the Webpack development server. With a lot of debugging I could see that the extra API request was causing issues. Now it seems to be passing consistently.
Unfortunately the failing job doesn't appear to have the backend logs for this, so I can't tell if yet another extra request disrupted the flash message. But maybe that's the case, and we need to prioritize #385232.
@peterhegman I am going to be on OOO from tomorrow, let me know if you think the frontend-only change deserves a merge on its own. Feel free to find another maintainer to merge later as well.
@peterhegman I'll unassign myself here. Please feel free to ping again when this is ready.
requested review from @mfluharty and @vburton
removed review request for @vburton
requested review from @mrincon and removed review request for @mfluharty
- Resolved by Miguel Rincon
@peterhegman I am wondering why we need to clear the results of the dropdown when the user selects a result in the first place. On
master
I naively did:diff --git a/app/assets/javascripts/groups_projects/components/transfer_locations.vue b/app/assets/javascripts/groups_projects/components/transfer_locations.vue index e0c8ce36e3c9..6bc77036ebad 100644 --- a/app/assets/javascripts/groups_projects/components/transfer_locations.vue +++ b/app/assets/javascripts/groups_projects/components/transfer_locations.vue @@ -109,7 +109,7 @@ export default { }, methods: { handleSelect(item) { - this.searchTerm = ''; + // this.searchTerm = ''; this.$emit('input', item); }, async handleShow() {
And it seemed to behave as I would expect: with API requests only running when needed. I am not sure that addresses all the problems you fix here, but it may be a more boring solution. wdyt?
Edited by Miguel Rincon
changed milestone to %15.10
enabled an automatic merge when the pipeline for 50506882 succeeds
removed review request for @mrincon
mentioned in issue gitlab-org/quality/quality-engineering/team-tasks#1546 (closed)
added devopsdata stores grouptenant scale sectioncore platform labels and removed devopsmanage sectiondev + 1 deleted label
Closing this because this change did not actually fix the flaky spec. I opened !114498 (merged) to fix the flaky spec
reset approvals from @mrincon and @mfluharty by pushing to the branch