Linking to a comment in a resolved non-diff discussion in an MR doesn't expand the discussion

When opening a direct link to a comment of a resolved non-diff discussion in an MR, the discussion isn't expanded to show the comment being linked to.

This may be due to our redesigned non-diff discussions where the first comment is shown and the rest is collapsed.

Findings

In NoteableDiscussion, we have isRepliesCollapsed which determines whether a discussion is expanded or not. This is a local property of the component with a default value based on whether the discussion is resolved or not.

This means it doesn't get affected when checkLocationHash() mutates discussion.expanded when the discussion is being linked to.

I also noticed that this component also has shouldShowDiscussions which depends on discussion.expanded but an OR condition was added to make it true all of the time.

In the template, this is also used like so: <div v-if="shouldShowDiscussions" class="discussion-body">. We don't ever want this to be false because the discussion would just look like a gray line.

Screen_Shot_2019-03-22_at_8.45.46_PM

Possible solution

  1. Remove v-if="shouldShowDiscussions" in div.discussion-body.

  2. Change shouldShowDiscussions to something like:

    diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue 
    b/app/assets/javascripts/notes/components/noteable_discussion.vue
    index fc51998935d..44b0fe4d772 100644
    --- a/app/assets/javascripts/notes/components/noteable_discussion.vue
    +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue
    @@ -176,10 +176,7 @@ export default {
           return '';
         },
         shouldShowDiscussions() {
    -      const { expanded, resolved } = this.discussion;
    -      const isResolvedNonDiffDiscussion = !this.discussion.diff_discussion && resolved;
    -
    -      return expanded || this.alwaysExpanded || isResolvedNonDiffDiscussion;
    +      return this.discussion.expanded || this.alwaysExpanded;
         },
         actionText() {
           const linkStart = `<a href="${_.escape(this.discussion.discussion_path)}">`;
  3. Remove isRepliesCollapsed and change all references to it in the template to !shouldShowDiscussions

  4. Remove toggleReplies and use the already existing toggleDiscussionHandler instead. This actually triggers the TOGGLE_DISCUSSION mutation instead of just toggling the internal isRepliesCollapsed boolean.

It looks like this.discussion.expanded is being set properly by the backend from DiscussionEntity so we should just base the initial state on that instead of having special logic here in the view.

Edited by Heinrich Lee Yu