From 7495140de2b4412240a09d7ee72800a84f57433e Mon Sep 17 00:00:00 2001
From: Heinrich Lee Yu <heinrich@gitlab.com>
Date: Fri, 22 Mar 2019 22:33:01 +0800
Subject: [PATCH] Expand discussion when opening link to comment

Makes discussion expansion depend on `discussion.expanded`
---
 .../notes/components/noteable_discussion.vue  | 26 +++++--------
 .../59352-fix-mr-discussion-expansion.yml     |  5 +++
 .../user_scrolls_to_note_on_load_spec.rb      | 38 ++++++++++++++-----
 3 files changed, 44 insertions(+), 25 deletions(-)
 create mode 100644 changelogs/unreleased/59352-fix-mr-discussion-expansion.yml

diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue
index 0fabbfb06b5..a3d664a738f 100644
--- a/app/assets/javascripts/notes/components/noteable_discussion.vue
+++ b/app/assets/javascripts/notes/components/noteable_discussion.vue
@@ -87,13 +87,10 @@ export default {
     },
   },
   data() {
-    const { diff_discussion: isDiffDiscussion, resolved } = this.discussion;
-
     return {
       isReplying: false,
       isResolving: false,
       resolveAsThread: true,
-      isRepliesCollapsed: Boolean(!isDiffDiscussion && resolved),
     };
   },
   computed: {
@@ -178,11 +175,11 @@ export default {
 
       return '';
     },
-    shouldShowDiscussions() {
-      const { expanded, resolved } = this.discussion;
-      const isResolvedNonDiffDiscussion = !this.discussion.diff_discussion && resolved;
-
-      return expanded || this.alwaysExpanded || isResolvedNonDiffDiscussion;
+    isExpanded() {
+      return this.discussion.expanded || this.alwaysExpanded;
+    },
+    shouldHideDiscussionBody() {
+      return this.shouldRenderDiffs && !this.isExpanded;
     },
     actionText() {
       const linkStart = `<a href="${_.escape(this.discussion.discussion_path)}">`;
@@ -282,9 +279,6 @@ export default {
     toggleDiscussionHandler() {
       this.toggleDiscussion({ discussionId: this.discussion.id });
     },
-    toggleReplies() {
-      this.isRepliesCollapsed = !this.isRepliesCollapsed;
-    },
     showReplyForm() {
       this.isReplying = true;
     },
@@ -405,7 +399,7 @@ Please check your network connection and try again.`;
             />
           </div>
         </div>
-        <div v-if="shouldShowDiscussions" class="discussion-body">
+        <div v-if="!shouldHideDiscussionBody" class="discussion-body">
           <component
             :is="wrapperComponent"
             v-bind="wrapperComponentProps"
@@ -436,11 +430,11 @@ Please check your network connection and try again.`;
                   </component>
                   <toggle-replies-widget
                     v-if="hasReplies"
-                    :collapsed="isRepliesCollapsed"
+                    :collapsed="!isExpanded"
                     :replies="replies"
-                    @toggle="toggleReplies"
+                    @toggle="toggleDiscussionHandler"
                   />
-                  <template v-if="!isRepliesCollapsed">
+                  <template v-if="isExpanded">
                     <component
                       :is="componentName(note)"
                       v-for="note in replies"
@@ -467,7 +461,7 @@ Please check your network connection and try again.`;
                 </template>
               </ul>
               <div
-                v-if="!isRepliesCollapsed || !hasReplies"
+                v-if="isExpanded || !hasReplies"
                 :class="{ 'is-replying': isReplying }"
                 class="discussion-reply-holder"
               >
diff --git a/changelogs/unreleased/59352-fix-mr-discussion-expansion.yml b/changelogs/unreleased/59352-fix-mr-discussion-expansion.yml
new file mode 100644
index 00000000000..ab9ad53835c
--- /dev/null
+++ b/changelogs/unreleased/59352-fix-mr-discussion-expansion.yml
@@ -0,0 +1,5 @@
+---
+title: Expand resolved discussion when linking to a comment in the discussion
+merge_request: 26483
+author:
+type: fixed
diff --git a/spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb b/spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb
index 8c2599615cb..2f7d359575e 100644
--- a/spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb
+++ b/spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb
@@ -5,9 +5,7 @@ describe 'Merge request > User scrolls to note on load', :js do
   let(:user) { project.creator }
   let(:merge_request) { create(:merge_request, source_project: project, author: user) }
   let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
-  let(:resolved_note) { create(:diff_note_on_merge_request, :resolved, noteable: merge_request, project: project) }
   let(:fragment_id) { "#note_#{note.id}" }
-  let(:collapsed_fragment_id) { "#note_#{resolved_note.id}" }
 
   before do
     sign_in(user)
@@ -45,13 +43,35 @@ describe 'Merge request > User scrolls to note on load', :js do
     end
   end
 
-  # TODO: https://gitlab.com/gitlab-org/gitlab-ce/issues/48034
-  xit 'expands collapsed notes' do
-    visit "#{project_merge_request_path(project, merge_request)}#{collapsed_fragment_id}"
-    note_element = find(collapsed_fragment_id)
-    note_container = note_element.ancestor('.timeline-content')
+  context 'resolved notes' do
+    let(:collapsed_fragment_id) { "#note_#{resolved_note.id}" }
 
-    expect(note_element.visible?).to eq true
-    expect(note_container.find('.line_content.noteable_line.old', match: :first).visible?).to eq true
+    context 'when diff note' do
+      let(:resolved_note) { create(:diff_note_on_merge_request, :resolved, noteable: merge_request, project: project) }
+
+      it 'expands collapsed notes' do
+        visit "#{project_merge_request_path(project, merge_request)}#{collapsed_fragment_id}"
+
+        note_element = find(collapsed_fragment_id)
+        diff_container = note_element.ancestor('.diff-content')
+
+        expect(note_element.visible?).to eq(true)
+        expect(diff_container.visible?).to eq(true)
+      end
+    end
+
+    context 'when non-diff note' do
+      let(:non_diff_discussion) { create(:discussion_note_on_merge_request, :resolved, noteable: merge_request, project: project) }
+      let(:resolved_note) { create(:discussion_note_on_merge_request, :resolved, noteable: merge_request, project: project, in_reply_to: non_diff_discussion) }
+
+      it 'expands collapsed replies' do
+        visit "#{project_merge_request_path(project, merge_request)}#{collapsed_fragment_id}"
+
+        note_element = find(collapsed_fragment_id)
+
+        expect(note_element.visible?).to eq(true)
+        expect(note_element.sibling('.replies-toggle')[:class]).to include('expanded')
+      end
+    end
   end
 end