Merge branch '45271-collpased-diff-loading' into 'master'

Resolve "Collapsed diff spacing wrong when failed to load"

Closes #45271 and #45317

See merge request gitlab-org/gitlab-ce!18351

app/assets/javascripts/notes.js

@@ -19,7 +19,6 @@ import AjaxCache from '~/lib/utils/ajax_cache';
 import Vue from 'vue';
 import syntaxHighlight from '~/syntax_highlight';
 import SkeletonLoadingContainer from '~/vue_shared/components/skeleton_loading_container.vue';
-import { __ } from '~/locale';
 import axios from './lib/utils/axios_utils';
 import { getLocationHash } from './lib/utils/url_utility';
 import Flash from './flash';
@@ -198,6 +197,8 @@ export default class Notes {
     );
 
     this.$wrapperEl.on('click', '.js-toggle-lazy-diff', this.loadLazyDiff);
+    this.$wrapperEl.on('click', '.js-toggle-lazy-diff-retry-button', this.onClickRetryLazyLoad.bind(this));
+
     // fetch notes when tab becomes visible
     this.$wrapperEl.on('visibilitychange', this.visibilityChange);
     // when issue status changes, we need to refresh data
@@ -244,6 +245,7 @@ export default class Notes {
     this.$wrapperEl.off('click', '.js-comment-resolve-button');
     this.$wrapperEl.off('click', '.system-note-commit-list-toggler');
     this.$wrapperEl.off('click', '.js-toggle-lazy-diff');
+    this.$wrapperEl.off('click', '.js-toggle-lazy-diff-retry-button');
     this.$wrapperEl.off('ajax:success', '.js-main-target-form');
     this.$wrapperEl.off('ajax:success', '.js-discussion-note-form');
     this.$wrapperEl.off('ajax:complete', '.js-main-target-form');
@@ -1431,16 +1433,15 @@ export default class Notes {
     syntaxHighlight(fileHolder);
   }
 
-  static renderDiffError($container) {
-    $container.find('.line_content').html(
-      $(`
-        <div class="nothing-here-block">
-          ${__(
-            'Unable to load the diff.',
-          )} <a class="js-toggle-lazy-diff" href="javascript:void(0)">Try again</a>?
-        </div>
-      `),
-    );
+  onClickRetryLazyLoad(e) {
+    const $retryButton = $(e.currentTarget);
+
+    $retryButton.prop('disabled', true);
+
+    return this.loadLazyDiff(e)
+    .then(() => {
+      $retryButton.prop('disabled', false);
+    });
   }
 
   loadLazyDiff(e) {
@@ -1449,21 +1450,36 @@ export default class Notes {
 
     $container.find('.js-toggle-lazy-diff').removeClass('js-toggle-lazy-diff');
 
-    const tableEl = $container.find('tbody');
-    if (tableEl.length === 0) return;
+    const $tableEl = $container.find('tbody');
+    if ($tableEl.length === 0) return;
 
     const fileHolder = $container.find('.file-holder');
     const url = fileHolder.data('linesPath');
 
-    axios
+    const $errorContainer = $container.find('.js-error-lazy-load-diff');
+    const $successContainer = $container.find('.js-success-lazy-load');
+
+    /**
+     * We only fetch resolved discussions.
+     * Unresolved discussions don't have an endpoint being provided.
+     */
+    if (url) {
+      return axios
       .get(url)
       .then(({ data }) => {
+        // Reset state in case last request returned error
+        $successContainer.removeClass('hidden');
+        $errorContainer.addClass('hidden');
+
         Notes.renderDiffContent($container, data);
       })
       .catch(() => {
-        Notes.renderDiffError($container);
+        $successContainer.addClass('hidden');
+        $errorContainer.removeClass('hidden');
       });
     }
+    return Promise.resolve();
+  }
 
   toggleCommitList(e) {
     const $element = $(e.currentTarget);

app/assets/stylesheets/framework/buttons.scss

@@ -503,3 +503,7 @@ fieldset[disabled] .btn,
     @extend %disabled;
   }
 }
+
+.btn-no-padding {
+  padding: 0;
+}

app/assets/stylesheets/pages/diff.scss

@@ -160,6 +160,11 @@
         }
       }
     }
+
+    .diff-loading-error-block {
+      padding: $gl-padding * 2 $gl-padding;
+      text-align: center;
+    }
   }
 
   .image {

app/assets/stylesheets/pages/merge_requests.scss

@@ -762,3 +762,20 @@
     max-width: 100%;
   }
 }
+
+// Hack alert: we've rewritten `btn` class in a way that
+// we've broken it and it is not possible to use with `btn-link`
+// which causes a blank button when it's disabled and hovering
+// The css in here is the boostrap one
+.btn-link-retry {
+  &[disabled] {
+    cursor: not-allowed;
+    box-shadow: none;
+    opacity: .65;
+
+    &:hover {
+      color: $file-mode-changed;
+      text-decoration: none;
+    }
+  }
+}

app/views/discussions/_diff_with_notes.html.haml

@@ -7,7 +7,7 @@
 - unless expanded
   - diff_data = { lines_path: project_merge_request_discussion_path(discussion.project, discussion.noteable, discussion) }
 
-.diff-file.file-holder{ class: diff_file_class, data: diff_data }
+.diff-file.file-holder.js-lazy-load-discussion{ class: diff_file_class, data: diff_data }
   .js-file-title.file-title.file-title-flex-parent
     .file-header-content
       = render "projects/diffs/file_header", diff_file: diff_file, url: discussion_path(discussion), show_toggle: false
@@ -28,8 +28,11 @@
           %tr.line_holder.line-holder-placeholder
             %td.old_line.diff-line-num
             %td.new_line.diff-line-num
-            %td.line_content
+            %td.line_content.js-success-lazy-load
               .js-code-placeholder
+            %td.js-error-lazy-load-diff.hidden.diff-loading-error-block
+              - button = button_tag(_("Try again"), class: "btn-link btn-link-retry btn-no-padding js-toggle-lazy-diff-retry-button")
+              = _("Unable to load the diff. %{button_try_again}").html_safe % { button_try_again: button}
           = render "discussions/diff_discussion", discussions: [discussion], expanded: true
   - else
     - partial = (diff_file.new_file? || diff_file.deleted_file?) ? 'single_image_diff' : 'replaced_image_diff'

changelogs/unreleased/45271-collpased-diff-loading.yml

+---
+title: Fixes unresolved discussions rendering the error state instead of the diff
+merge_request:
+author:
+type: fixed

spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb

@@ -27,6 +27,23 @@ describe 'Merge request > User scrolls to note on load', :js do
     expect(fragment_position_top).to be < (page_scroll_y + page_height)
   end
 
+  it 'renders un-collapsed notes with diff' do
+    page.current_window.resize_to(1000, 1000)
+
+    visit "#{project_merge_request_path(project, merge_request)}#{fragment_id}"
+
+    page.execute_script "window.scrollTo(0,0)"
+
+    note_element = find(fragment_id)
+    note_container = note_element.ancestor('.js-toggle-container')
+
+    expect(note_element.visible?).to eq true
+
+    page.within note_container do
+      expect(page).not_to have_selector('.js-error-lazy-load-diff')
+    end
+  end
+
   it 'expands collapsed notes' do
     visit "#{project_merge_request_path(project, merge_request)}#{collapsed_fragment_id}"
     note_element = find(collapsed_fragment_id)