Skip to content

Consolidate discussion GID for user comments

Alexandru Croitor requested to merge fix-discussion-gid-with-one-note into master

What does this MR do and why?

Consolidate discussion GID for notes. Currently based on notes within a discussion or noteable on which the discussion happens the discussion GID can have a different representation.

That makes it difficult to use discussion GID as a unique identifier.

This MR consolidates discussion GID to always return the same value.

Screenshots or screen recordings

The only difference is the string representation of the GID. So for instance :

Before this code change

The String representation of the GID gid://gitlab/DiffDiscussion/b266efe29c8403f72294861a88042397f881cda0

[217] pry(main)> gid = Note.last.discussion.to_global_id
  Note Load (2.8ms)  SELECT "notes".* FROM "notes" ORDER BY "notes"."id" DESC LIMIT 1 /*application:console,db_config_name:main,console_hostname:GTLB-Alexandru.local,console_username:acroitor,line:(pry):221:in `__pry__'*/
  Project Load (5.6ms)  SELECT "projects".* FROM "projects" WHERE "projects"."id" = 2 LIMIT 1 /*application:console,db_config_name:main,console_hostname:GTLB-Alexandru.local,console_username:acroitor,line:/app/models/note.rb:367:in `commit'*/
  Route Load (0.8ms)  SELECT "routes".* FROM "routes" WHERE "routes"."source_id" = 2 AND "routes"."source_type" = 'Project' LIMIT 1 /*application:console,db_config_name:main,console_hostname:GTLB-Alexandru.local,console_username:acroitor,line:/app/models/concerns/routable.rb:121:in `full_path'*/
  Note Load (0.5ms)  SELECT "notes".* FROM "notes" WHERE "notes"."project_id" = 2 AND "notes"."noteable_type" = 'Commit' AND "notes"."commit_id" = '9ce57ef86f578ff01ebb75a58413dbde46aab683' AND "notes"."discussion_id" = 'b266efe29c8403f72294861a88042397f881cda0' ORDER BY "notes"."created_at" ASC, "notes"."id" ASC /*application:console,db_config_name:main,console_hostname:GTLB-Alexandru.local,console_username:acroitor,line:/app/models/note.rb:228:in `find_discussion'*/
=> #<GlobalID:0x00000002b571b148 @uri=#<URI::GID gid://gitlab/DiffDiscussion/b266efe29c8403f72294861a88042397f881cda0>>

[218] pry(main)> discussion = GitlabSchema.find_by_gid(gid)
=> #<DiffDiscussion:0x000000010a12c3d8
 @context_noteable=nil,
 @notes=
  [#<DiffNote:0x000000010a12e1b0
    id: 37532,
    note: "[FILTERED]",
    noteable_type: "Commit",
    author_id: 1,
    created_at: Wed, 25 Jan 2023 15:50:59.957342000 UTC +00:00,
    updated_at: Wed, 25 Jan 2023 15:50:59.957342000 UTC +00:00,
    project_id: 2,
    attachment: nil,
    line_code: "1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_5",
    commit_id: "9ce57ef86f578ff01ebb75a58413dbde46aab683",
    noteable_id: nil,
    system: false,
    st_diff: nil,
    updated_by_id: nil,
    type: "DiffNote",
    position:
     #<Gitlab::Diff::Position:1862760 {:base_sha=>"d52b519083123493d1692ec7b333890278d643f3", :start_sha=>"d52b519083123493d1692ec7b333890278d643f3", :head_sha=>"9ce57ef86f578ff01ebb75a58413dbde46aab683", :old_path=>"CHANGELOG", :new_path=>"CHANGELOG", :position_type=>"text", :old_line=>nil, :new_line=>5, :line_range=>nil}>,
    original_position:
     #<Gitlab::Diff::Position:1862780 {:base_sha=>"d52b519083123493d1692ec7b333890278d643f3", :start_sha=>"d52b519083123493d1692ec7b333890278d643f3", :head_sha=>"9ce57ef86f578ff01ebb75a58413dbde46aab683", :old_path=>"CHANGELOG", :new_path=>"CHANGELOG", :position_type=>"text", :old_line=>nil, :new_line=>5, :line_range=>nil}>,
    resolved_at: nil,
    resolved_by_id: nil,
    discussion_id: "b266efe29c8403f72294861a88042397f881cda0",
    note_html: "<p data-sourcepos=\"1:1-1:7\" dir=\"auto\">adsadsd</p>",
    cached_markdown_version: 2097152,
    change_position: #<Gitlab::Diff::Position:1862800 {:base_sha=>nil, :start_sha=>nil, :head_sha=>nil, :old_path=>nil, :new_path=>nil, :position_type=>"text", :old_line=>nil, :new_line=>nil, :line_range=>nil}>,
    resolved_by_push: nil,
    review_id: nil,
    confidential: nil,
    last_edited_at: nil,
    internal: false,
    first_discussion_note: true>,
   #<DiffNote:0x000000010a12df30
    id: 37533,
    note: "[FILTERED]",
    noteable_type: "Commit",
    author_id: 1,
    created_at: Wed, 25 Jan 2023 15:52:13.015456000 UTC +00:00,
    updated_at: Wed, 25 Jan 2023 15:52:13.015456000 UTC +00:00,
    project_id: 2,
    attachment: nil,
    line_code: "1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_5",
    commit_id: "9ce57ef86f578ff01ebb75a58413dbde46aab683",
    noteable_id: nil,
    system: false,
    st_diff: nil,
    updated_by_id: nil,
    type: "DiffNote",
    position:
     #<Gitlab::Diff::Position:1862820 {:base_sha=>"d52b519083123493d1692ec7b333890278d643f3", :start_sha=>"d52b519083123493d1692ec7b333890278d643f3", :head_sha=>"9ce57ef86f578ff01ebb75a58413dbde46aab683", :old_path=>"CHANGELOG", :new_path=>"CHANGELOG", :position_type=>"text", :old_line=>nil, :new_line=>5, :line_range=>nil}>,
    original_position:
     #<Gitlab::Diff::Position:1862840 {:base_sha=>"d52b519083123493d1692ec7b333890278d643f3", :start_sha=>"d52b519083123493d1692ec7b333890278d643f3", :head_sha=>"9ce57ef86f578ff01ebb75a58413dbde46aab683", :old_path=>"CHANGELOG", :new_path=>"CHANGELOG", :position_type=>"text", :old_line=>nil, :new_line=>5, :line_range=>nil}>,
    resolved_at: nil,
    resolved_by_id: nil,
    discussion_id: "b266efe29c8403f72294861a88042397f881cda0",
    note_html: "<p data-sourcepos=\"1:1-1:8\" dir=\"auto\">qeeweqwe</p>",
    cached_markdown_version: 2097152,
    change_position: #<Gitlab::Diff::Position:1862860 {:base_sha=>nil, :start_sha=>nil, :head_sha=>nil, :old_path=>nil, :new_path=>nil, :position_type=>"text", :old_line=>nil, :new_line=>nil, :line_range=>nil}>,
    resolved_by_push: nil,
    review_id: nil,
    confidential: false,
    last_edited_at: nil,
    internal: false,
    first_discussion_note: true>]>
After the code change

The String representation of the GID gid://gitlab/Discussion/b266efe29c8403f72294861a88042397f881cda0

[220] pry(main)> gid = Note.last.discussion.to_global_id
  Note Load (1.1ms)  SELECT "notes".* FROM "notes" ORDER BY "notes"."id" DESC LIMIT 1 /*application:console,db_config_name:main,console_hostname:GTLB-Alexandru.local,console_username:acroitor,line:(pry):224:in `__pry__'*/
  Project Load (2.2ms)  SELECT "projects".* FROM "projects" WHERE "projects"."id" = 2 LIMIT 1 /*application:console,db_config_name:main,console_hostname:GTLB-Alexandru.local,console_username:acroitor,line:/app/models/note.rb:367:in `commit'*/
  Route Load (0.8ms)  SELECT "routes".* FROM "routes" WHERE "routes"."source_id" = 2 AND "routes"."source_type" = 'Project' LIMIT 1 /*application:console,db_config_name:main,console_hostname:GTLB-Alexandru.local,console_username:acroitor,line:/app/models/concerns/routable.rb:121:in `full_path'*/
  Note Load (0.6ms)  SELECT "notes".* FROM "notes" WHERE "notes"."project_id" = 2 AND "notes"."noteable_type" = 'Commit' AND "notes"."commit_id" = '9ce57ef86f578ff01ebb75a58413dbde46aab683' AND "notes"."discussion_id" = 'b266efe29c8403f72294861a88042397f881cda0' ORDER BY "notes"."created_at" ASC, "notes"."id" ASC /*application:console,db_config_name:main,console_hostname:GTLB-Alexandru.local,console_username:acroitor,line:/app/models/note.rb:228:in `find_discussion'*/
=> #<GlobalID:0x00000002ad261d08 @uri=#<URI::GID gid://gitlab/Discussion/b266efe29c8403f72294861a88042397f881cda0>>

[221] pry(main)> discussion = GitlabSchema.find_by_gid(gid)
=> #<DiffDiscussion:0x000000010a12c3d8
 @context_noteable=nil,
 @notes=
  [#<DiffNote:0x000000010a12e1b0
    id: 37532,
    note: "[FILTERED]",
    noteable_type: "Commit",
    author_id: 1,
    created_at: Wed, 25 Jan 2023 15:50:59.957342000 UTC +00:00,
    updated_at: Wed, 25 Jan 2023 15:50:59.957342000 UTC +00:00,
    project_id: 2,
    attachment: nil,
    line_code: "1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_5",
    commit_id: "9ce57ef86f578ff01ebb75a58413dbde46aab683",
    noteable_id: nil,
    system: false,
    st_diff: nil,
    updated_by_id: nil,
    type: "DiffNote",
    position:
     #<Gitlab::Diff::Position:1862760 {:base_sha=>"d52b519083123493d1692ec7b333890278d643f3", :start_sha=>"d52b519083123493d1692ec7b333890278d643f3", :head_sha=>"9ce57ef86f578ff01ebb75a58413dbde46aab683", :old_path=>"CHANGELOG", :new_path=>"CHANGELOG", :position_type=>"text", :old_line=>nil, :new_line=>5, :line_range=>nil}>,
    original_position:
     #<Gitlab::Diff::Position:1862780 {:base_sha=>"d52b519083123493d1692ec7b333890278d643f3", :start_sha=>"d52b519083123493d1692ec7b333890278d643f3", :head_sha=>"9ce57ef86f578ff01ebb75a58413dbde46aab683", :old_path=>"CHANGELOG", :new_path=>"CHANGELOG", :position_type=>"text", :old_line=>nil, :new_line=>5, :line_range=>nil}>,
    resolved_at: nil,
    resolved_by_id: nil,
    discussion_id: "b266efe29c8403f72294861a88042397f881cda0",
    note_html: "<p data-sourcepos=\"1:1-1:7\" dir=\"auto\">adsadsd</p>",
    cached_markdown_version: 2097152,
    change_position: #<Gitlab::Diff::Position:1862800 {:base_sha=>nil, :start_sha=>nil, :head_sha=>nil, :old_path=>nil, :new_path=>nil, :position_type=>"text", :old_line=>nil, :new_line=>nil, :line_range=>nil}>,
    resolved_by_push: nil,
    review_id: nil,
    confidential: nil,
    last_edited_at: nil,
    internal: false,
    first_discussion_note: true>,
   #<DiffNote:0x000000010a12df30
    id: 37533,
    note: "[FILTERED]",
    noteable_type: "Commit",
    author_id: 1,
    created_at: Wed, 25 Jan 2023 15:52:13.015456000 UTC +00:00,
    updated_at: Wed, 25 Jan 2023 15:52:13.015456000 UTC +00:00,
    project_id: 2,
    attachment: nil,
    line_code: "1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_5",
    commit_id: "9ce57ef86f578ff01ebb75a58413dbde46aab683",
    noteable_id: nil,
    system: false,
    st_diff: nil,
    updated_by_id: nil,
    type: "DiffNote",
    position:
     #<Gitlab::Diff::Position:1862820 {:base_sha=>"d52b519083123493d1692ec7b333890278d643f3", :start_sha=>"d52b519083123493d1692ec7b333890278d643f3", :head_sha=>"9ce57ef86f578ff01ebb75a58413dbde46aab683", :old_path=>"CHANGELOG", :new_path=>"CHANGELOG", :position_type=>"text", :old_line=>nil, :new_line=>5, :line_range=>nil}>,
    original_position:
     #<Gitlab::Diff::Position:1862840 {:base_sha=>"d52b519083123493d1692ec7b333890278d643f3", :start_sha=>"d52b519083123493d1692ec7b333890278d643f3", :head_sha=>"9ce57ef86f578ff01ebb75a58413dbde46aab683", :old_path=>"CHANGELOG", :new_path=>"CHANGELOG", :position_type=>"text", :old_line=>nil, :new_line=>5, :line_range=>nil}>,
    resolved_at: nil,
    resolved_by_id: nil,
    discussion_id: "b266efe29c8403f72294861a88042397f881cda0",
    note_html: "<p data-sourcepos=\"1:1-1:8\" dir=\"auto\">qeeweqwe</p>",
    cached_markdown_version: 2097152,
    change_position: #<Gitlab::Diff::Position:1862860 {:base_sha=>nil, :start_sha=>nil, :head_sha=>nil, :old_path=>nil, :new_path=>nil, :position_type=>"text", :old_line=>nil, :new_line=>nil, :line_range=>nil}>,
    resolved_by_push: nil,
    review_id: nil,
    confidential: false,
    last_edited_at: nil,
    internal: false,
    first_discussion_note: true>]>

So in essence all that changes is how the GID is represented as a String which is what we want, but it will be different from what it used to look like before this change.

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

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

Edited by Alexandru Croitor

Merge request reports