Capture diff note position on mergeability check
What does this MR do?
When a merge request is checked whether it's mergeable merge_ref_head
is updated and we need to update diff note positions within it.
Related issue: #198457 (closed)
Related MRs:
-
!28113 (merged) - Introduces
diff_note_positions
table - !28003 (closed) - draft MR for demonstrating the general idea
Merge request reports
Activity
changed milestone to %12.10
added backend devopscreate groupsource code typefeature labels
added database databasereview pending labels
2 Messages 📖 This merge request adds or changes files that require a review from the Database team. 📖 CHANGELOG missing: If this merge request doesn’t need a CHANGELOG entry, feel free to ignore this message. If you want to create a changelog entry for GitLab FOSS, run the following:
bin/changelog -m 28623 "Capture diff note position on mergeability check"
If you want to create a changelog entry for GitLab EE, run the following instead:
bin/changelog --ee -m 28623 "Capture diff note position on mergeability check"
Note: Merge requests with ~backstage, ci-build, meta do not trigger this check.
This merge request requires a database review. To make sure these changes are reviewed, take the following steps:
- Ensure the merge request has database and databasereview pending labels. If the merge request modifies database files, Danger will do this for you.
- Prepare your MR for database review according to the docs.
- Assign and mention the database reviewer suggested by Reviewer Roulette.
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 this selection 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
)Dylan Griffith ( @DylanGriffith
)database Yannis Roussos ( @iroussos
)Tiger Watson ( @tigerwnz
)Generated by
🚫 DangerEdited by 🤖 GitLab Bot 🤖added 1 commit
- 97058d2a - Capture diff note position on mergeability check
added 1 commit
- ad4f604a - Capture diff note position on mergeability check
- Resolved by Igor Drozdov
The idea is to store the position which a note has on the
merge_ref_head
version and send it to the frontend along with theposition
andoriginal_position
. I was inspired by this idea #27008 (comment 215656247), but in the first implementation of this !28003 (closed) I've been creating adiff_note_position
(a table of which is being added here) for every change ofmerge_ref_head
because I was thinking that this data will be useful for displaying notes in those cases that we're not covering now (#36093)After @oswaldo has noticed that we may create an unnecessarily big amount of records using this approach, I gave this idea a second thought:
- storing all the positions for
merge_ref_head
won't be useful, it's enough to store only the latest one - we may want to store all the position updates instead of updating a column: https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/services/discussions/update_diff_position_service.rb#L21
I've also noticed that we updating the positions for all notes in a discussion, which actually duplicates the data, only to delegate to the first note when we're fetching this data: https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/models/diff_discussion.rb#L14.
As a result, I have the following idea:
-
diff_note_positions
table will havetype
column, which is enum for:merge_ref_head
,original
,regular
,change
(merge_ref_head
position,original_position
,position
andchange_position
correspondingly) - we will store records for
merge_ref_head
andregular
positions as an association for the first note of a discussion (need to investigate whether we can do the same fororiginal
andchange
without complicating the fetch of these positions for an arbitrary note)
This idea is implemented for
merge_ref_head
in this MR in order to tackle #198457 (closed), for the rest columns we can think about during #33271Answering the questions from !28003 (comment 313034173):
Are we going to support both implementations of discussion positioning at the same time?
Yes, until we gradually migrate the data from the position columns into
diff_note_positions
Should we have a clear strategy there on deprecating / removing the old behavior?
I'll update the #33271 with keynotes from these discussions after we're good with the discussed approach. The current idea is to apply
diff_note_positions
tomerge_ref_head
case and gradually move the data fromnotes
into this table@nick.thomas can I ask you to have a look at these ideas? After reviewing with @oswaldo I'll ask you for a maintainer review if you don't mind
- storing all the positions for
- Resolved by Igor Drozdov
@oswaldo could you please review this MR?
🙏 I'm sorry to ping you on this topic, will try to do it rarer😅
assigned to @oswaldo
mentioned in merge request !28326 (closed)
- Resolved by Igor Drozdov
- Resolved by Igor Drozdov
- Resolved by Igor Drozdov
added 1 commit
- 09186c19 - Capture diff note position on mergeability check
- Resolved by Igor Drozdov
added 1 commit
- a34df395 - Capture diff note position on mergeability check
added 1 commit
- 1baf8631 - Capture diff note position on mergeability check
added 1 commit
- 0aa2ffcb - Capture diff note position on mergeability check