WIP: Fix imported from GitHub empty discussion threads on MRs
What does this MR do?
This MR fixes diff notes on Imported from GitHub merge requests. Currently diff notes are shown on diff page, however they are not shown on Overview
Tab. This is due to the fact that discussion.diff_file.diff_refs
is nil, which frontend relies on.
Please note that:
- Github Importer uses
LegacyDiffNote
instead ofDiffNote
- Each diff note is inserted using bulk_insert https://gitlab.com/gitlab-org/gitlab/blob/master/lib/gitlab/github_import/importer/diff_note_importer.rb#L50-50 and skips all rails validations and callbacks for performance reasons
I am not 100% sure of the solution yet, as I am not sure if there are going to be any side effects by initializing Diff File with diff_refs: noteable.diff_refs
due to my lack of domain knowledge on Diffs and DiffNotes. I watched diff notes deep dive video and spent 3 days investigating but it's still not clear to me.
Screenshots
Before (old screenshot from #26952 (closed)) -- discussion is not expandable
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
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
Merge request reports
Activity
changed milestone to %12.9
added devopsmanage typebug + 1 deleted label
added 1 commit
- 4d4e3a41 - Fix imported from GitHub empty discussion threads on MRs
2 Warnings 4d4e3a41: The commit subject length is acceptable, but please try to reduce it to 50 characters. You’ve made some app changes, but didn’t add any tests.
That’s OK as long as you’re refactoring existing code,
but please consider adding any of the ~backstage, documentation, QA labels.Commit message standards
One or more commit messages do not meet our Git commit message standards.
For more information on how to write a good commit message, take a look at How to Write a Git Commit Message.
Here is an example of a good commit message:
Reject ruby interpolation in externalized strings When using ruby interpolation in externalized strings, they can't be detected. Which means they will never be presented to be translated. To mix variables into translations we need to use `sprintf` instead. Instead of: _("Hello #{subject}") Use: _("Hello %{subject}") % { subject: 'world' }
This is an example of a bad commit message:
updated README.md
This commit message is bad because although it tells us that README.md is updated, it doesn't tell us why or how it was updated.
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 Vitali Tatarintev ( @ck3g
)James Fargher ( @proglottis
)Generated by
DangerHey @oswaldo may I ask you to have a look at this change? Wanted to double check if this solution is acceptable as I am not sure if there are going to be any unexpected side effects. This is still WIP due to this reason and I haven't added a spec just yet. CI is green so it looks like a good sign. Thanks
assigned to @oswaldo
@georgekoltsov Thank you for your time spent on that issue.
I've tried to manually apply your patch to 12.6.4-ee and despite the fact it actually starts to download diff notes, it messes up diff-viewers.
- discussions to lines of code in a diff are showed as two download buttons pointing to before/after file
- I've created a commit with some text added to README.md (via Web IDE), created a merge request. And I see a diff-viewer for Images.
I believe that could be due to version mismatch (I applied it to 12.6.4-ee) but I decided to point your attention to it just in case.
Thanks for testing it out @tuxx.sp looks like the current approach is not going to work.
32 32 end 33 33 34 34 def diff_file 35 @diff_file ||= Gitlab::Diff::File.new(diff, repository: project_repository) if diff 35 @diff_file ||= Gitlab::Diff::File.new(diff, diff_refs: noteable.diff_refs, repository: project_repository) if diff @georgekoltsov Right, after some investigation here's where I'm at:
- It's present for the Changes tab because we pass it when building the collection. It uses the persisted,
MergeRequestDiff
record to get thediff_refs
-
MergeRequestDiff#diff_refs
holds the revisions of when a push / update occurred
In general I believe we can't just pass
noteable.diff_refs
because it'll always fetch the same revisions for every legacy comment. With that I have two questions:- Are we expecting to present the diff here or just the discussions when expanding? (I believe the latter, right?)
- Wouldn't the fix be a matter of changing how FE handles the lack of
diff_refs
, in the Legacy diff scenario?
Edited by Oswaldo Ferreira- It's present for the Changes tab because we pass it when building the collection. It uses the persisted,
Thanks for the review @oswaldo you confirmed my suspicions of this solution being invalid :)
- Are we expecting to present the diff here or just the discussions when expanding? (I believe the latter, right?)
I believe we need to present the same as for DiffNote, which is a small diff (pardon if I am not using correct terminology here. Not a full file diff but a small part of it where the discussion started) as well as the diff notes.
- Wouldn't the fix be a matter of changing how FE handles the lack of
diff_refs
, in the Legacy diff scenario?
That's a good question. Is there anything else that can be used when
diff_refs
are missing? Is it aline_code
that needs to be used?I was also exploring a possibility of using DiffNote, instead of LegacyDiffNote, but I wasn't able to get it to work due to use of bulk_insert, which skips all callbacks and validations. And for DiffNote, the creation of diff file is happening in the after_commit callback https://gitlab.com/gitlab-org/gitlab/blob/georgekoltsov/26952-fix-github-discussion-diff/app/models/diff_note.rb#L32-32
I believe we need to present the same as for DiffNote, which is a small diff (pardon if I am not using correct terminology here. Not a full file diff but a small part of it where the discussion started) as well as the diff notes. I believe it's expected that a
LegacyDiffNote
won't present the diffs in theOverview
tab given it doesn't have some information such asposition
ororiginal_position
(normally found at theDiffNote
- the positions hold refs and other valuable information to present diffs).I'm curious given FE expects
diff_refs
to always exist, so it raises this error when trying to load discussions. It wasn't always the case, as we used to present a limited discussion in the case of legacy, back in the day.I'd like to involve @iamphill and @dskim_gitlab to look further into that and help if possible!
@oswaldo @georgekoltsov I'm not a domain expert in this area either, but I'm working on to become one though.
I don't have much time left today, but I can do some investigation and get back to you tomorrow if that's ok.@nick.thomas Would you be able to have a look at this? I did look around for a bit, but got distracted last couple of days with on-call and etc. I feel I have taken up too much time already. I'll get back to it again tomorrow if you are busy as well, but would be great to hear from you if you have any suggestions.
Sorry about that @georgekoltsov!
Thanks for your comment @nick.thomas
After looking at it again today, it does seem working around this in FE code makes more sense. Given Changes tab is already working, I assume the same can be applied to Overview section.
@iamphill Would you be able to comment on this from FE point of view?
@iamphill did you have a chance to look at the above?
Ok, thanks for your input @iamphill it sounds to me that before doing any changes on the backend side there needs to be some frontend investigation whether it's possible to fix it with what we currently have.
I am going to close this MR and update the issue. Thanks all.
assigned to @dskim_gitlab
assigned to @iamphill and unassigned @dskim_gitlab
mentioned in issue #26952 (closed)
changed milestone to %12.10
added missed:12.9 label
added groupimport and integrate label and removed 1 deleted label