Skip to content
Snippets Groups Projects

WIP: Fix imported from GitHub empty discussion threads on MRs

Closed George Koltsov requested to merge georgekoltsov/26952-fix-github-discussion-diff into master
2 unresolved threads

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:

  1. Github Importer uses LegacyDiffNote instead of DiffNote
  2. 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 image

After image

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

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
Edited by 🤖 GitLab Bot 🤖

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • George Koltsov changed milestone to %12.9

    changed milestone to %12.9

  • added devopsmanage typebug + 1 deleted label

  • George Koltsov changed the description

    changed the description

  • George Koltsov added 1 commit

    added 1 commit

    • 4d4e3a41 - Fix imported from GitHub empty discussion threads on MRs

    Compare with previous version

  • 2 Warnings
    :warning: 4d4e3a41: The commit subject length is acceptable, but please try to reduce it to 50 characters.
    :warning: 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 :no_entry_sign: Danger

  • George Koltsov changed the description

    changed the description

  • Hey @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 :pray:

  • 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.

      1. discussions to lines of code in a diff are showed as two download buttons pointing to before/after file
      2. 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.

    • Please register or sign in to reply
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:

    1. It's present for the Changes tab because we pass it when building the collection. It uses the persisted, MergeRequestDiff record to get the diff_refs
    2. 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:

    1. Are we expecting to present the diff here or just the discussions when expanding? (I believe the latter, right?)
    2. 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
  • Thanks for the review @oswaldo you confirmed my suspicions of this solution being invalid :)

    1. 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.

    1. 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 a line_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 the Overview tab given it doesn't have some information such as position or original_position (normally found at the DiffNote - 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! :smile:

  • @oswaldo @georgekoltsov I'm not a domain expert in this area either, but I'm working on to become one though. :sweat_smile: 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! :pray:

  • I don't have a great deal of context on this, but having the FE more-gracefully handle this scenario seems like a better option than returning synthetic data from the backend, at least to me :shrug:

  • Thanks for your comment @nick.thomas :pray:

    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? :pray:

  • I'm not really sure I follow what needs to be done on the frontend side of things. However, if we are handling this correctly on the changes tab then I would expect it to be easy enough to handle it correctly on the overview tab. :shrug:

  • 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.

  • Please register or sign in to reply
  • assigned to @iamphill and unassigned @dskim_gitlab

  • mentioned in issue #26952 (closed)

  • changed milestone to %12.10

  • 🤖 GitLab Bot 🤖 changed the description

    changed the description

  • 🤖 GitLab Bot 🤖 added groupimport and integrate label and removed 1 deleted label

    added groupimport and integrate label and removed 1 deleted label

  • Please register or sign in to reply
    Loading