Skip to content

Fix an incorrect parameter with the Draft Notes API

Gary Holtz requested to merge 433402-clearing-the-diff-highlighting-cache-is-confusing-a-merge-request-for-a-project-in-some-cases into master

What does this MR do and why?

A sentry error revealed that the Draft Notes API was incorrectly passing a Project to DraftNotes::DestroyService:

  def delete_draft_note(draft_note)
    ::DraftNotes::DestroyService.new(user_project, current_user).execute(draft_note)
  end

This would result in DestroyService occasionally throwing an error here:

  def clear_highlight_diffs_cache(drafts)
      if drafts.any? { |draft| draft.diff_file&.unfolded? }
        **merge_request.diffs.clear_cache**
      end
    end

Since a Project was being passed in the initializer, merge_request was referring to a project and thus didn't have a diffs method.

This MR changes the Project to a MergeRequest and updates the original spec to stub draft.diff_file&.unfolded? so it will fail if something else passes the wrong parameter.

How to set up and validate locally

To set up and validate the change locally, follow these steps:

  1. Clone the GitLab repository and checkout the branch associated with this MR.
  2. Run bundle install to install the necessary dependencies.
  3. Run the tests to verify that the change works as expected.
  4. In an MR in the changes tab, add a draft note with any content
  5. Delete that draft note via the API with a curl command
curl --request DELETE \
  --header "PRIVATE-TOKEN: <your_access_token>" \
    --url "https://gitlab.example.com/api/v4/projects/14/merge_requests/11/draft_notes/5"

(You can find the necessary IDs by doing DraftNote.last in the rails console. Remember the MR id should be the iid!)

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #433402 (closed)

Edited by Gary Holtz

Merge request reports