Commit 57995172 authored by Douwe Maan's avatar Douwe Maan
Browse files

Merge branch 'diff-line-comment-vuejs' into 'master'

Diff line comments resolve

## What does this MR do?

Diff line comments can be resolved.

Part of #10325 

To do:

- [x] Backend (@DouweM)
  - [x] Fix https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5022#note_13319326. Will be made easier by https://gitlab.com/gitlab-org/gitlab-ce/issues/17237#note_13370331
  - [x] System note when all discussions are resolved
  - [x] Notification when all discussions are resolved
  - [x] Write unit tests
  - [x] Look at resolve time https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5022#note_13912743 - Fixed by 4a13aa9f
- [x] Frontend (@iamphill)
  - [x] Fix bugs
  - [x] Write more feature tests 
- [x] Frontend (@connorshea)
  - [x] Address frontend feedback
  - [x] Feature specs for Jump feature
  - [x] Documentation
  - [x] Add Vue.js in a standard way

See merge request !5022
parents c5aa31c8 10c5ec3e
Loading
Loading
Loading
Loading
+1 −0
Original line number Original line Diff line number Diff line
@@ -30,6 +30,7 @@ v 8.11.0 (unreleased)
  - Expand commit message width in repo view (ClemMakesApps)
  - Expand commit message width in repo view (ClemMakesApps)
  - Cache highlighted diff lines for merge requests
  - Cache highlighted diff lines for merge requests
  - Pre-create all builds for a Pipeline when the new Pipeline is created !5295
  - Pre-create all builds for a Pipeline when the new Pipeline is created !5295
  - Allow merge request diff notes and discussions to be explicitly marked as resolved
  - API: Add deployment endpoints
  - API: Add deployment endpoints
  - API: Add Play endpoint on Builds
  - API: Add Play endpoint on Builds
  - Fix of 'Commits being passed to custom hooks are already reachable when using the UI'
  - Fix of 'Commits being passed to custom hooks are already reachable when using the UI'
+6 −3
Original line number Original line Diff line number Diff line
@@ -225,10 +225,13 @@
    });
    });
    $body.on("click", ".js-toggle-diff-comments", function(e) {
    $body.on("click", ".js-toggle-diff-comments", function(e) {
      var $this = $(this);
      var $this = $(this);
      var showComments = $this.hasClass('active');

      $this.toggleClass('active');
      $this.toggleClass('active');
      $this.closest(".diff-file").find(".notes_holder").toggle(showComments);
      var notesHolders = $this.closest('.diff-file').find('.notes_holder');
      if ($this.hasClass('active')) {
        notesHolders.show();
      } else {
        notesHolders.hide();
      }
      return e.preventDefault();
      return e.preventDefault();
    });
    });
    $document.off("click", '.js-confirm-danger');
    $document.off("click", '.js-confirm-danger');
+49 −0
Original line number Original line Diff line number Diff line
((w) => {
  w.CommentAndResolveBtn = Vue.extend({
    props: {
      discussionId: String,
      textareaIsEmpty: Boolean
    },
    computed: {
      discussion: function () {
        return CommentsStore.state[this.discussionId];
      },
      showButton: function () {
        if (this.discussion) {
          return this.discussion.isResolvable();
        } else {
          return false;
        }
      },
      isDiscussionResolved: function () {
        return this.discussion.isResolved();
      },
      buttonText: function () {
        if (this.isDiscussionResolved) {
          if (this.textareaIsEmpty) {
            return "Unresolve discussion";
          } else {
            return "Comment & unresolve discussion";
          }
        } else {
          if (this.textareaIsEmpty) {
            return "Resolve discussion";
          } else {
            return "Comment & resolve discussion";
          }
        }
      }
    },
    ready: function () {
      const $textarea = $(`#new-discussion-note-form-${this.discussionId} .note-textarea`);
      this.textareaIsEmpty = $textarea.val() === '';

      $textarea.on('input.comment-and-resolve-btn', () => {
        this.textareaIsEmpty = $textarea.val() === '';
      });
    },
    destroyed: function () {
      $(`#new-discussion-note-form-${this.discussionId} .note-textarea`).off('input.comment-and-resolve-btn');
    }
  });
})(window);
+188 −0
Original line number Original line Diff line number Diff line
(() => {
  JumpToDiscussion = Vue.extend({
    mixins: [DiscussionMixins],
    props: {
      discussionId: String
    },
    data: function () {
      return {
        discussions: CommentsStore.state,
      };
    },
    computed: {
      discussion: function () {
        return this.discussions[this.discussionId];
      },
      allResolved: function () {
        return this.unresolvedDiscussionCount === 0;
      },
      showButton: function () {
        if (this.discussionId) {
          if (this.unresolvedDiscussionCount > 1) {
            return true;
          } else {
            return this.discussionId !== this.lastResolvedId;
          }
        } else {
          return this.unresolvedDiscussionCount >= 1;
        }
      },
      lastResolvedId: function () {
        let lastId;
        for (const discussionId in this.discussions) {
          const discussion = this.discussions[discussionId];

          if (!discussion.isResolved()) {
            lastId = discussion.id;
          }
        }
        return lastId;
      }
    },
    methods: {
      jumpToNextUnresolvedDiscussion: function () {
        let discussionsSelector,
            discussionIdsInScope,
            firstUnresolvedDiscussionId,
            nextUnresolvedDiscussionId,
            activeTab = window.mrTabs.currentAction,
            hasDiscussionsToJumpTo = true,
            jumpToFirstDiscussion = !this.discussionId;

        const discussionIdsForElements = function(elements) {
          return elements.map(function() {
            return $(this).attr('data-discussion-id');
          }).toArray();
        };

        const discussions = this.discussions;

        if (activeTab === 'diffs') {
          discussionsSelector = '.diffs .notes[data-discussion-id]';
          discussionIdsInScope = discussionIdsForElements($(discussionsSelector));

          let unresolvedDiscussionCount = 0;

          for (let i = 0; i < discussionIdsInScope.length; i++) {
            const discussionId = discussionIdsInScope[i];
            const discussion = discussions[discussionId];
            if (discussion && !discussion.isResolved()) {
              unresolvedDiscussionCount++;
            }
          }

          if (this.discussionId && !this.discussion.isResolved()) {
            // If this is the last unresolved discussion on the diffs tab,
            // there are no discussions to jump to.
            if (unresolvedDiscussionCount === 1) {
              hasDiscussionsToJumpTo = false;
            }
          } else {
            // If there are no unresolved discussions on the diffs tab at all,
            // there are no discussions to jump to.
            if (unresolvedDiscussionCount === 0) {
              hasDiscussionsToJumpTo = false;
            }
          }
        } else if (activeTab !== 'notes') {
          // If we are on the commits or builds tabs,
          // there are no discussions to jump to.
          hasDiscussionsToJumpTo = false;
        }

        if (!hasDiscussionsToJumpTo) {
          // If there are no discussions to jump to on the current page,
          // switch to the notes tab and jump to the first disucssion there.
          window.mrTabs.activateTab('notes');
          activeTab = 'notes';
          jumpToFirstDiscussion = true;
        }

        if (activeTab === 'notes') {
          discussionsSelector = '.discussion[data-discussion-id]';
          discussionIdsInScope = discussionIdsForElements($(discussionsSelector));
        }

        let currentDiscussionFound = false;
        for (let i = 0; i < discussionIdsInScope.length; i++) {
          const discussionId = discussionIdsInScope[i];
          const discussion = discussions[discussionId];

          if (!discussion) {
            // Discussions for comments on commits in this MR don't have a resolved status.
            continue;
          }

          if (!firstUnresolvedDiscussionId && !discussion.isResolved()) {
            firstUnresolvedDiscussionId = discussionId;

            if (jumpToFirstDiscussion) {
              break;
            }
          }

          if (!jumpToFirstDiscussion) {
            if (currentDiscussionFound) {
              if (!discussion.isResolved()) {
                nextUnresolvedDiscussionId = discussionId;
                break;
              }
              else {
                continue;
              }
            }

            if (discussionId === this.discussionId) {
              currentDiscussionFound = true;
            }
          }
        }

        nextUnresolvedDiscussionId = nextUnresolvedDiscussionId || firstUnresolvedDiscussionId;

        if (!nextUnresolvedDiscussionId) {
          return;
        }

        let $target = $(`${discussionsSelector}[data-discussion-id="${nextUnresolvedDiscussionId}"]`);

        if (activeTab === 'notes') {
          $target = $target.closest('.note-discussion');

          // If the next discussion is closed, toggle it open.
          if ($target.find('.js-toggle-content').is(':hidden')) {
            $target.find('.js-toggle-button i').trigger('click')
          }
        } else if (activeTab === 'diffs') {
          // Resolved discussions are hidden in the diffs tab by default.
          // If they are marked unresolved on the notes tab, they will still be hidden on the diffs tab.
          // When jumping between unresolved discussions on the diffs tab, we show them.
          $target.closest(".content").show();

          $target = $target.closest("tr.notes_holder");
          $target.show();

          // If we are on the diffs tab, we don't scroll to the discussion itself, but to
          // 4 diff lines above it: the line the discussion was in response to + 3 context
          let prevEl;
          for (let i = 0; i < 4; i++) {
            prevEl = $target.prev();

            // If the discussion doesn't have 4 lines above it, we'll have to do with fewer.
            if (!prevEl.hasClass("line_holder")) {
              break;
            }

            $target = prevEl;
          }
        }

        $.scrollTo($target, {
          offset: -($('.navbar-gitlab').outerHeight() + $('.layout-nav').outerHeight())
        });
      }
    }
  });

  Vue.component('jump-to-discussion', JumpToDiscussion);
})();
+107 −0
Original line number Original line Diff line number Diff line
((w) => {
  w.ResolveBtn = Vue.extend({
    mixins: [
      ButtonMixins
    ],
    props: {
      noteId: Number,
      discussionId: String,
      resolved: Boolean,
      namespacePath: String,
      projectPath: String,
      canResolve: Boolean,
      resolvedBy: String
    },
    data: function () {
      return {
        discussions: CommentsStore.state,
        loading: false
      };
    },
    watch: {
      'discussions': {
        handler: 'updateTooltip',
        deep: true
      }
    },
    computed: {
      discussion: function () {
        return this.discussions[this.discussionId];
      },
      note: function () {
        if (this.discussion) {
          return this.discussion.getNote(this.noteId);
        } else {
          return undefined;
        }
      },
      buttonText: function () {
        if (this.isResolved) {
          return `Resolved by ${this.resolvedByName}`;
        } else if (this.canResolve) {
          return 'Mark as resolved';
        } else {
          return 'Unable to resolve';
        }
      },
      isResolved: function () {
        if (this.note) {
          return this.note.resolved;
        } else {
          return false;
        }
      },
      resolvedByName: function () {
        return this.note.resolved_by;
      },
    },
    methods: {
      updateTooltip: function () {
        $(this.$els.button)
          .tooltip('hide')
          .tooltip('fixTitle');
      },
      resolve: function () {
        if (!this.canResolve) return;

        let promise;
        this.loading = true;

        if (this.isResolved) {
          promise = ResolveService
            .unresolve(this.namespace, this.noteId);
        } else {
          promise = ResolveService
            .resolve(this.namespace, this.noteId);
        }

        promise.then((response) => {
          this.loading = false;

          if (response.status === 200) {
            const data = response.json();
            const resolved_by = data ? data.resolved_by : null;

            CommentsStore.update(this.discussionId, this.noteId, !this.isResolved, resolved_by);
            this.discussion.updateHeadline(data);
          } else {
            new Flash('An error occurred when trying to resolve a comment. Please try again.', 'alert');
          }

          this.$nextTick(this.updateTooltip);
        });
      }
    },
    compiled: function () {
      $(this.$els.button).tooltip({
        container: 'body'
      });
    },
    beforeDestroy: function () {
      CommentsStore.delete(this.discussionId, this.noteId);
    },
    created: function () {
      CommentsStore.create(this.discussionId, this.noteId, this.canResolve, this.resolved, this.resolvedBy);
    }
  });
})(window);
Loading