With #3071755: Show merge request activity on www.drupal.org issues, we will be showing all comments on merge requests associated with an issue in that issue. Currently, these are just collapsed into a linear list of activity. We should either:

  • Add a note about which thread a comment is in reply to, keeping the ability to catch up in a linear way.
  • Thread the comments in the issue, keeping the structure of the conversation.
  • Thread the comments in the issue, and order them by the last reply time, keeping the structure, and some of the linear flattening.
CommentFileSizeAuthor
#4 Screen Shot 2020-09-17 at 6.02.34 PM.png69.97 KBdrumm
#2 Screen Shot 2020-07-28 at 4.42.35 PM.png110.05 KBmglaman

Issue fork drupalorg-3158852

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drumm created an issue. See original summary.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
FileSize
Screen Shot 2020-07-28 at 4.42.35 PM.png110.05 KB

I was doing some poking and saw the GitLab MR loads this URL: https://git.drupalcode.org/project/commerce_api/-/merge_requests/2/discu...

Each discussions contains the Notes and truncated_diff_lines. This contains the files which were commented on!

       "truncated_diff_lines": [
            {
                "line_code": "a626dfee914fb8b259cf5beeccfa19fdd37d2225_1_29",
                "type": "new",
                "old_line": null,
                "new_line": 29,
                "text": "+      'entity' =\u003e $payment_method,",
                "meta_data": null,
                "rich_text": "+\u003cspan id=\"LC29\" class=\"line\" lang=\"php\"\u003e      \u003cspan class=\"s1\"\u003e'entity'\u003c/span\u003e \u003cspan class=\"o\"\u003e=\u0026gt;\u003c/span\u003e \u003cspan class=\"nv\"\u003e$payment_method\u003c/span\u003e\u003cspan class=\"p\"\u003e,\u003c/span\u003e\u003c/span\u003e\n",
                "can_receive_suggestion": true
            },
            {
                "line_code": "a626dfee914fb8b259cf5beeccfa19fdd37d2225_1_30",
                "type": "new",
                "old_line": null,
                "new_line": 30,
                "text": "+    ]);",
                "meta_data": null,
                "rich_text": "+\u003cspan id=\"LC30\" class=\"line\" lang=\"php\"\u003e    \u003cspan class=\"p\"\u003e]);\u003c/span\u003e\u003c/span\u003e\n",
                "can_receive_suggestion": true
            },
            {
                "line_code": "a626dfee914fb8b259cf5beeccfa19fdd37d2225_1_31",
                "type": "new",
                "old_line": null,
                "new_line": 31,
                "text": "+  }",
                "meta_data": null,
                "rich_text": "+\u003cspan id=\"LC31\" class=\"line\" lang=\"php\"\u003e  \u003cspan class=\"p\"\u003e}\u003c/span\u003e\u003c/span\u003e\n",
                "can_receive_suggestion": true
            },

This is the first three lines from the following discussion and note

  • drumm committed d1e913d on 7.x-3.x 
    Issue #3158852: Add reply to note for merge request replies
drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm at Drupal Association commented
Assigned: Unassigned » drumm
FileSize
Screen Shot 2020-09-17 at 6.02.34 PM.png69.97 KB

The initial deployment here adds some context for notes that are replies.

Screenshot

This could use some additional styling, and potentially highlighting the notes in the thread on hover.

  • drumm committed 8496359 on 7.x-3.x 
    Issue #3158852: Add discussion marker

  • drumm committed a2799e4 on 7.x-3.x 
    Issue #3158852: Highlight unresolved discussions
drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm at Drupal Association commented

https://git.drupalcode.org/project/commerce_api/-/merge_requests/2/discu... isn’t a documented API, so I’m hesitant to rely on it. I did find the position property of notes. Combined with https://docs.gitlab.com/ee/api/repositories.html#compare-branches-tags-o..., we should be able to build our own truncated diff. However, I discovered a bug where getting the position 500s the whole query after a note is edited, https://gitlab.com/gitlab-org/gitlab/-/issues/255934. I expect editing notes will happen often enough, so we have to backlog this until the GitLab issue is resolved.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm at Drupal Association commented
Status: Active » Fixed

I split off showing the diff context into #3173909: Show diff snippets for merge request notes.

With previous deployments from this issue, notes now have:

  • A link to the start of their discussion
  • Highlighting of all the notes in a discussion
  • A more-visible unresolved note marker

That accomplishes what I wanted for this issue. Followups can be more-targeted issues.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.