Skip to content
Snippets Groups Projects

Fix incorrect Batched suggestions applications

2 unresolved threads

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

Availability and Testing

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)

Edited by 🤖 GitLab Bot 🤖

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
  • Thanks a lot for jumping on this @dskim_gitlab! Left a thought.

  • added 1 commit

    • e06582e8 - Sort suggestions before applying them

    Compare with previous version

  • Bob Van Landuyt approved this merge request

    approved this merge request

  • added 1 commit

    • 8a80618f - Apply 1 suggestion(s) to 1 file(s)

    Compare with previous version

  • added 1 commit

    • d5a8d3f7 - Make sorted_suggestions method private

    Compare with previous version

  • added 1 commit

    • d4c772c5 - Refactor a little to simplify the logic

    Compare with previous version

  • Sincheol (David) Kim
  • Nick Thomas resolved all threads

    resolved all threads

  • Nick Thomas approved this merge request

    approved this merge request

  • merged

  • Nick Thomas mentioned in commit 4d460c88

    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:

      Screenshot_2020-06-22_at_16.00.59

      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. 🙏

    • FYI, I've enabled it on staging for verification. It seems to be working fine according to my tests though.

    • @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 🙇

    • Please register or sign in to reply
  • added workflowcanary label and removed workflowstaging label

  • added workflowproduction label and removed workflowcanary label

  • André Luís mentioned in merge request !35525 (merged)

    mentioned in merge request !35525 (merged)

  • 🤖 GitLab Bot 🤖 changed the description

    changed the description

  • added groupcode review label and removed groupsource code label

  • Please register or sign in to reply
    Loading