Put Batched Suggestions behind a Feature Flag default to false
What does this MR do?
In order to prevent unpredictable and incorrect application of suggestions when using the recent Add suggestion to Batch feature, this MR disables the Add to Batch button when the line count will be changed by applying the suggestion.
Update: we ended up wrapping the UI button behind a feature flag.
Screenshots
Feature flag disabled (default) | Feature flag enabled |
---|---|
![]() |
![]() |
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team
Addresses #222780 (closed)
Merge request reports
Activity
changed milestone to %13.1
added devopscreate frontend groupsource code merge requests labels
assigned to @andr3
added regression label
added regression:13.1 label
3 Warnings b3446031: Commits that change 30 or more lines across at least 3 files must describe these changes in the commit body. For more information, take a look at our Commit message guidelines. The merge request title length is acceptable, but please try to reduce it to 50 characters. For more information, take a look at our Commit message guidelines. Please add a throughput label to this merge request. 1 Message CHANGELOG missing: If you want to create a changelog entry for GitLab FOSS, run the following:
bin/changelog -m 34782 "Put Batched Suggestions behind a Feature Flag default to false"
If you want to create a changelog entry for GitLab EE, run the following instead:
bin/changelog --ee -m 34782 "Put Batched Suggestions behind a Feature Flag default to false"
If this merge request doesn’t need a CHANGELOG entry, feel free to ignore this message.
Reviewer roulette
Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.
To spread load more evenly across eligible reviewers, Danger has randomly picked a candidate for each review slot. Feel free to override these selections if you think someone else would be better-suited, or the chosen person is unavailable.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines.
Once you've decided who will review this merge request, mention them as you normally would! Danger does not (yet?) automatically notify them for you.
Category Reviewer Maintainer frontend Justin Ho ( @justin_ho
)Ezekiel Kigbo ( @ekigbo
)backend Robert May ( @robotmay
)Peter Leitzen ( @splattael
)If needed, you can retry the
danger-review
job that generated this comment.Generated by
DangerEdited by 🤖 GitLab Bot 🤖Bundle size analysis [beta]
This compares changes in bundle size for entry points between the commits ce62970d and b3446031
Special assetsEntrypoint / Name Size before Size after Diff Diff in percent average 4.05 MB 4.05 MB - 0.0 % mainChunk 3.19 MB 3.19 MB - 0.0 %
Note: We do not have exact data for ce62970d. So we have used data from: 83169b71.
The target commit was too new, so we used the latest commit from master we have info on.
It might help to rerun thebundle-size-review
job
This might mean that you have a few false positives in this report. If something unrelated to your code changes is reported, you can check this comparison in order to see if they caused this change.Please look at the full report for more details
Read more about how this report works.
Generated by
DangerEdited by 🤖 GitLab Bot 🤖added 201 commits
-
3d12378e...b7d9bfde - 200 commits from branch
master
- dc174393 - Disable Add to Batch on uneven suggestions
-
3d12378e...b7d9bfde - 200 commits from branch
mentioned in issue #222780 (closed)
@pslaughter could you please take a look at this and if it's ok, merge? Thanks.
assigned to @pslaughter and unassigned @andr3
marked the checklist item Changelog entry as completed
- Resolved by Jesse Hall
issue: Unfortunately, during my local testing, I found another issue with this feature
- Add a suggestion to batch
- Edit the suggestion (or even the comment)
- The original suggestion is never unapplied which either leaves the user in a doomed-to-fail state or with unintentional ghost suggestions
20200717_bug_with_batch_suggestions
It seems like this feature is not production ready yet even with this disable fix, so I'm suggestioning that we remove this button altogether. I will send a patch for this soon
Edited by Paul Slaughter
added 21 commits
-
96e7f8d4...ce62970d - 19 commits from branch
master
- d60d27f8 - Disable Add to Batch on uneven suggestions
- b3446031 - Apply patch to put button behind FF
-
96e7f8d4...ce62970d - 19 commits from branch
note: We'll want to revert the doc changes in !22439 (merged). We can do that in a separate MR
- Resolved by Paul Slaughter
Tested this locally. Everything LGTM! Thanks for hopping on this issue @andr3 and collaborating on the solution!
Approving and MWPS'ing...
enabled an automatic merge when the pipeline for 06311891 succeeds
added severity2 typebug labels and removed regression regression:13.1 labels
added Pick into auto-deploy + 1 deleted label
Added ~"Pick into 13.1" and Pick into auto-deploy labels.
Also set severity label and used the ~bug label instead of regression
mentioned in commit d72d6cb3
mentioned in commit 4aecfa69
picked the changes into the branch
13-1-auto-deploy-20200617
with commit 43f598f7removed Pick into auto-deploy label
mentioned in commit 43f598f7
According to @rspeicher, it looks like the ~Pick into 13.1 is not needed here
added workflowstaging label
added workflowcanary label and removed workflowstaging label
mentioned in issue gitlab-org/release/tasks#1370 (closed)
mentioned in merge request !34828 (merged)
mentioned in issue #223674 (closed)
mentioned in merge request !34994 (closed)
added Category:Code Review Workflow groupcode review labels and removed groupsource code label
added workflowproduction label and removed workflowcanary label