Fix incorrect Batched suggestions applications
What does this MR do?
This fixes the batch application issue when applying multiple suggestions to the same file. This happens only when the suggestion removes more than one line regardless of the length of the new content.
Please see more detail in #222780 (closed)
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
Related to #222780 (closed)
Merge request reports
Activity
changed milestone to %13.2
added 1 commit
- 5677eb55 - Fix batch apply suggestions issues with mutiple lines
2 Warnings ⚠ d4c772c5: 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. ⚠ 5677eb55: The commit subject length is acceptable, but please try to reduce it to 50 characters. For more information, take a look at our Commit message guidelines. 1 Message 📖 CHANGELOG missing: If you want to create a changelog entry for GitLab FOSS, run the following:
bin/changelog -m 34828 "Fix incorrect Batched suggestions applications"
If you want to create a changelog entry for GitLab EE, run the following instead:
bin/changelog --ee -m 34828 "Fix incorrect Batched suggestions applications"
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 backend Ethan Urie ( @eurie
)Robert Speicher ( @rspeicher
)If needed, you can retry the
danger-review
job that generated this comment.Generated by
🚫 DangerEdited by 🤖 GitLab Bot 🤖assigned to @dskim_gitlab
- Resolved by Nick Thomas
@reprazent Would you be able to review this fix please?
🙏
assigned to @reprazent
/cc @m_gill This seemed small enough issue so I took it on.
unassigned @reprazent
- Resolved by Bob Van Landuyt
Thanks a lot for jumping on this @dskim_gitlab! Left a thought.
removed [deprecated] Accepting merge requests label
assigned to @reprazent
- Resolved by Sincheol (David) Kim
- Resolved by Nick Thomas
- Resolved by Nick Thomas
Thanks @dskim_gitlab, I only had some minor nitpicks left, so feel free to pass it on for maintainer review after.
unassigned @reprazent
assigned to @nick.thomas
- Resolved by Nick Thomas
unassigned @nick.thomas
- Resolved by André Luís
@dskim_gitlab when this solution is merged, we need to remove the tooltip that is currently present in https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff_header.vue#L59
How would you like to proceed? You ping us when this is merged and we merge a separate MR to remove that tooltip? Or would you prefer a FE commit in this MR so that everything would go in at once?
Let me know so we can provide either a MR or a commit.
😉
- Resolved by Nick Thomas
Could we add one extra spec?
spec_for_file_suggestions.patch
The reason for this is that none of the current specs will fail if the suggestions are not sorted -- I've fetched this branch and confirmed this after removing the sorting. This extra spec will fail in that scenario.
It was tricky coming up with one as it was not enough just to change the order in which the suggestions were added to a file-suggestion.
Edited by Jesse Hall
assigned to @nick.thomas
- Resolved by Nick Thomas
Thanks @dskim_gitlab , I really like how the interface for
FileSuggestion
is simplified by this change!Thanks a lot for the review @nick.thomas
🙏
mentioned in commit 4d460c88
@dskim_gitlab can you please clarify whether this MR allows us to apply multiple suggestions in a batch even if some change the number of lines?
Or will it simply detect the collision and fail if that's the case?
The answer will impact the way we update the UI. I just ran some tests on latest master and it failed with:
But I might be missing something.
Thanks!
@andr3 It should allow you to apply multiple suggestions in a batch. Can you please check again if the underlying file hasn't changed after the suggestion? Please let me know if you still see any issues, I'll look into it as a separate issue.
🙏 @andr3 @dskim_gitlab I just tested it myself, and did various combinations:
- Increasing the number of lines in multiple suggestions
- Decreasing the number of lines in multiple suggestions
- A mix of increasing/decreasing the number of lines.
- Overlapping suggestions (which was detected as a mistake and blocked, nice!)
- Deleting lines completely.
All worked correctly, I could not break it!
Edited by Marcel Amirault@marcel.amirault Thanks a lot for your testing
🙇
added workflowstaging label
added workflowcanary label and removed workflowstaging label
added workflowproduction label and removed workflowcanary label
mentioned in merge request !35525 (merged)
added groupcode review label and removed groupsource code label