Merge branch 'ce-to-ee-2018-04-05' into 'master'

CE upstream - 2018-04-05 18:26 UTC

Closes #4788

See merge request gitlab-org/gitlab-ee!5257

GITLAB_PAGES_VERSION

-0.7.1
+0.8.0

app/assets/javascripts/ide/components/repo_editor.vue

@@ -19,7 +19,7 @@ export default {
     },
   },
   computed: {
-    ...mapState(['leftPanelCollapsed', 'rightPanelCollapsed', 'viewer', 'delayViewerUpdated']),
+    ...mapState(['rightPanelCollapsed', 'viewer', 'delayViewerUpdated', 'panelResizing']),
     ...mapGetters(['currentMergeRequest']),
     shouldHideEditor() {
       return this.file && this.file.binary && !this.file.raw;
@@ -42,15 +42,17 @@ export default {
         this.initMonaco();
       }
     },
-    leftPanelCollapsed() {
-      this.editor.updateDimensions();
-    },
     rightPanelCollapsed() {
       this.editor.updateDimensions();
     },
     viewer() {
       this.createEditorInstance();
     },
+    panelResizing() {
+      if (!this.panelResizing) {
+        this.editor.updateDimensions();
+      }
+    },
   },
   beforeDestroy() {
     this.editor.dispose();

app/assets/javascripts/ide/lib/editor.js

@@ -69,6 +69,7 @@ export default class Editor {
           occurrencesHighlight: false,
           renderLineHighlight: 'none',
           hideCursorInOverviewRuler: true,
+          renderSideBySide: Editor.renderSideBySide(domElement),
         })),
       );
 
@@ -81,7 +82,7 @@ export default class Editor {
   }
 
   attachModel(model) {
-    if (this.instance.getEditorType() === 'vs.editor.IDiffEditor') {
+    if (this.isDiffEditorType) {
       this.instance.setModel({
         original: model.getOriginalModel(),
         modified: model.getModel(),
@@ -153,6 +154,7 @@ export default class Editor {
 
   updateDimensions() {
     this.instance.layout();
+    this.updateDiffView();
   }
 
   setPosition({ lineNumber, column }) {
@@ -171,4 +173,20 @@ export default class Editor {
 
     this.disposable.add(this.instance.onDidChangeCursorPosition(e => cb(this.instance, e)));
   }
+
+  updateDiffView() {
+    if (!this.isDiffEditorType) return;
+
+    this.instance.updateOptions({
+      renderSideBySide: Editor.renderSideBySide(this.instance.getDomNode()),
+    });
+  }
+
+  get isDiffEditorType() {
+    return this.instance.getEditorType() === 'vs.editor.IDiffEditor';
+  }
+
+  static renderSideBySide(domElement) {
+    return domElement.offsetWidth >= 700;
+  }
 }

app/assets/javascripts/ide/lib/editor_options.js

@@ -6,7 +6,7 @@ export const defaultEditorOptions = {
   minimap: {
     enabled: false,
   },
-  wordWrap: 'bounded',
+  wordWrap: 'on',
 };
 
 export default [

app/assets/javascripts/notes.js

@@ -1190,12 +1190,12 @@ export default class Notes {
     addForm = false;
     let lineTypeSelector = '';
     rowCssToAdd =
-      '<tr class="notes_holder js-temp-notes-holder"><td class="notes_line" colspan="2"></td><td class="notes_content"><div class="content"></div></td></tr>';
+      '<tr class="notes_holder js-temp-notes-holder"><td class="notes_line" colspan="2"></td><td class="notes_content"><div class="content discussion-notes"></div></td></tr>';
     // In parallel view, look inside the correct left/right pane
     if (this.isParallelView()) {
       lineTypeSelector = `.${lineType}`;
       rowCssToAdd =
-        '<tr class="notes_holder js-temp-notes-holder"><td class="notes_line old"></td><td class="notes_content parallel old"><div class="content"></div></td><td class="notes_line new"></td><td class="notes_content parallel new"><div class="content"></div></td></tr>';
+        '<tr class="notes_holder js-temp-notes-holder"><td class="notes_line old"></td><td class="notes_content parallel old"><div class="content discussion-notes"></div></td><td class="notes_line new"></td><td class="notes_content parallel new"><div class="content discussion-notes"></div></td></tr>';
     }
     const notesContentSelector = `.notes_content${lineTypeSelector} .content`;
     let notesContent = targetRow.find(notesContentSelector);

app/assets/javascripts/notes/components/noteable_discussion.vue

@@ -258,9 +258,7 @@ Please check your network connection and try again.`;
                     :key="note.id"
                   />
                 </ul>
-                <div
-                  :class="{ 'is-replying': isReplying }"
-                  class="discussion-reply-holder">
+                <div class="discussion-reply-holder">
                   <template v-if="!isReplying && canReply">
                     <div
                       class="btn-group-justified discussion-with-resolve-btn"

app/assets/stylesheets/pages/diff.scss

@@ -813,6 +813,7 @@
 }
 
 .discussion-notes {
+  padding: 0 $gl-padding $gl-padding;
   min-height: 35px;
 
   &:first-child {

app/assets/stylesheets/pages/note_form.scss

@@ -173,11 +173,7 @@
 }
 
 .discussion-form {
-  background-color: $white-light;
-}
-
-.discussion-form-container {
-  padding: $gl-padding-top $gl-padding $gl-padding;
+  padding-top: $gl-padding-top;
 }
 
 .discussion-notes .disabled-comment {
@@ -237,12 +233,7 @@
 .discussion-body,
 .diff-file {
   .discussion-reply-holder {
-    background-color: $white-light;
-    padding: 10px 16px;
-
-    &.is-replying {
-      padding-bottom: $gl-padding;
-    }
+    padding-top: $gl-padding;
   }
 }
 

app/assets/stylesheets/pages/notes.scss

@@ -47,7 +47,7 @@ ul.notes {
     }
 
     .timeline-entry-inner {
-      padding: $gl-padding $gl-btn-padding;
+      padding: $gl-padding 0;
       border-bottom: 1px solid $white-normal;
     }
 
@@ -94,12 +94,6 @@ ul.notes {
       }
     }
 
-    &.note-discussion {
-      .timeline-entry-inner {
-        padding: $gl-padding 10px;
-      }
-    }
-
     .editing-spinner {
       display: none;
     }
@@ -352,6 +346,8 @@ ul.notes {
         }
 
         .discussion-notes {
+          background-color: $white-light;
+
           &:not(:first-child) {
             border-top: 1px solid $white-normal;
             margin-top: 20px;
@@ -363,10 +359,6 @@ ul.notes {
           }
         }
 
-        .notes {
-          background-color: $white-light;
-        }
-
         a code {
           top: 0;
           margin-right: 0;
@@ -647,8 +639,6 @@ ul.notes {
     border-bottom: 1px solid $white-normal;
 
     .timeline-entry-inner {
-      padding-left: $gl-padding;
-      padding-right: $gl-padding;
       border-bottom: 0;
     }
   }

app/controllers/projects/discussions_controller.rb

@@ -4,8 +4,8 @@ class Projects::DiscussionsController < Projects::ApplicationController
 
   before_action :check_merge_requests_available!
   before_action :merge_request
-  before_action :discussion
-  before_action :authorize_resolve_discussion!
+  before_action :discussion, only: [:resolve, :unresolve]
+  before_action :authorize_resolve_discussion!, only: [:resolve, :unresolve]
 
   def resolve
     Discussions::ResolveService.new(project, current_user, merge_request: merge_request).execute(discussion)

app/views/shared/notes/_form.html.haml

@@ -24,21 +24,20 @@
   -# DiffNote
   = f.hidden_field :position
 
-  .discussion-form-container
-    = render layout: 'projects/md_preview', locals: { url: preview_url, referenced_users: true } do
-      = render 'projects/zen', f: f,
-        attr: :note,
-        classes: 'note-textarea js-note-text',
-        placeholder: "Write a comment or drag your files here...",
-        supports_quick_actions: supports_quick_actions,
-        supports_autocomplete: supports_autocomplete
-      = render 'shared/notes/hints', supports_quick_actions: supports_quick_actions
-      .error-alert
-
-    .note-form-actions.clearfix
-      = render partial: 'shared/notes/comment_button'
-
-      = yield(:note_actions)
-
-      %a.btn.btn-cancel.js-note-discard{ role: "button", data: {cancel_text: "Cancel" } }
-        Discard draft
+  = render layout: 'projects/md_preview', locals: { url: preview_url, referenced_users: true } do
+    = render 'projects/zen', f: f,
+      attr: :note,
+      classes: 'note-textarea js-note-text',
+      placeholder: "Write a comment or drag your files here...",
+      supports_quick_actions: supports_quick_actions,
+      supports_autocomplete: supports_autocomplete
+    = render 'shared/notes/hints', supports_quick_actions: supports_quick_actions
+    .error-alert
+
+  .note-form-actions.clearfix
+    = render partial: 'shared/notes/comment_button'
+
+    = yield(:note_actions)
+
+    %a.btn.btn-cancel.js-note-discard{ role: "button", data: {cancel_text: "Cancel" } }
+      Discard draft

changelogs/unreleased/44870-remove-extra-space-around-comment-form-on-merge-requests.yml

+---
+title: Refactor and tweak margin for note forms on Issuable
+merge_request: 18120
+author: Takuya Noguchi
+type: fixed

changelogs/unreleased/osw-44295-adjust-authorization-for-discussions-show.yml

+---
+title: Adjust 404's for LegacyDiffNote discussion rendering
+merge_request: 18201
+author:
+type: fixed

doc/api/commits.md

@@ -539,6 +539,8 @@ Example response:
 
 ## List Merge Requests associated with a commit
 
+> [Introduced][ce-18004] in GitLab 10.7.
+
 Get a list of Merge Requests related to the specified commit.
 
 ```
@@ -608,3 +610,4 @@ Example response:
 [ce-6096]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6096 "Multi-file commit"
 [ce-8047]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8047
 [ce-15026]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/15026
+[ce-18004]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/18004

spec/controllers/projects/discussions_controller_spec.rb

@@ -16,6 +16,53 @@ describe Projects::DiscussionsController do
     }
   end
 
+  describe 'GET show' do
+    before do
+      sign_in user
+    end
+
+    context 'when user is not authorized to read the MR' do
+      it 'returns 404' do
+        get :show, request_params, format: :json
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+
+    context 'when user is authorized to read the MR' do
+      before do
+        project.add_reporter(user)
+      end
+
+      it 'returns status 200' do
+        get :show, request_params, format: :json
+
+        expect(response).to have_gitlab_http_status(200)
+      end
+
+      it 'returns status 404 if MR does not exists' do
+        merge_request.destroy!
+
+        get :show, request_params, format: :json
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+
+    context 'when user is authorized but note is LegacyDiffNote' do
+      before do
+        project.add_developer(user)
+        note.update!(type: 'LegacyDiffNote')
+      end
+
+      it 'returns status 200' do
+        get :show, request_params, format: :json
+
+        expect(response).to have_gitlab_http_status(200)
+      end
+    end
+  end
+
   describe 'POST resolve' do
     before do
       sign_in user

spec/javascripts/ide/components/repo_editor_spec.js

@@ -199,47 +199,49 @@ describe('RepoEditor', () => {
     });
   });
 
-  describe('setup editor for merge request viewing', () => {
-    beforeEach(done => {
-      // Resetting as the main test setup has already done it
-      vm.$destroy();
-      resetStore(vm.$store);
-      Editor.editorInstance.modelManager.dispose();
-
-      const f = {
-        ...file(),
-        active: true,
-        tempFile: true,
-        html: 'testing',
-        mrChange: { diff: 'ABC' },
-        baseRaw: 'testing',
-        content: 'test',
-      };
-      const RepoEditor = Vue.extend(repoEditor);
-      vm = createComponentWithStore(RepoEditor, store, {
-        file: f,
-      });
+  describe('editor updateDimensions', () => {
+    beforeEach(() => {
+      spyOn(vm.editor, 'updateDimensions').and.callThrough();
+      spyOn(vm.editor, 'updateDiffView');
+    });
 
-      vm.$store.state.openFiles.push(f);
-      vm.$store.state.entries[f.path] = f;
+    it('calls updateDimensions when rightPanelCollapsed is changed', done => {
+      vm.$store.state.rightPanelCollapsed = true;
 
-      vm.$store.state.viewer = 'mrdiff';
+      vm.$nextTick(() => {
+        expect(vm.editor.updateDimensions).toHaveBeenCalled();
+        expect(vm.editor.updateDiffView).toHaveBeenCalled();
 
-      vm.monaco = true;
+        done();
+      });
+    });
 
-      vm.$mount();
+    it('calls updateDimensions when panelResizing is false', done => {
+      vm.$store.state.panelResizing = true;
 
-      monacoLoader(['vs/editor/editor.main'], () => {
-        setTimeout(done, 0);
-      });
+      vm
+        .$nextTick()
+        .then(() => {
+          vm.$store.state.panelResizing = false;
+        })
+        .then(vm.$nextTick)
+        .then(() => {
+          expect(vm.editor.updateDimensions).toHaveBeenCalled();
+          expect(vm.editor.updateDiffView).toHaveBeenCalled();
+        })
+        .then(done)
+        .catch(done.fail);
     });
 
-    it('attaches merge request model to editor when merge request diff', () => {
-      spyOn(vm.editor, 'attachMergeRequestModel').and.callThrough();
+    it('does not call updateDimensions when panelResizing is true', done => {
+      vm.$store.state.panelResizing = true;
 
-      vm.setupEditor();
+      vm.$nextTick(() => {
+        expect(vm.editor.updateDimensions).not.toHaveBeenCalled();
+        expect(vm.editor.updateDiffView).not.toHaveBeenCalled();
 
-      expect(vm.editor.attachMergeRequestModel).toHaveBeenCalledWith(vm.model);
+        done();
+      });
     });
   });
 });

spec/javascripts/ide/lib/editor_spec.js

@@ -76,7 +76,8 @@ describe('Multi-file editor library', () => {
         occurrencesHighlight: false,
         renderLineHighlight: 'none',
         hideCursorInOverviewRuler: true,
-        wordWrap: 'bounded',
+        wordWrap: 'on',
+        renderSideBySide: true,
       });
     });
   });
@@ -215,4 +216,56 @@ describe('Multi-file editor library', () => {
       expect(instance.decorationsController.dispose).not.toHaveBeenCalled();
     });
   });
+
+  describe('updateDiffView', () => {
+    describe('edit mode', () => {
+      it('does not update options', () => {
+        instance.createInstance(holder);
+
+        spyOn(instance.instance, 'updateOptions');
+
+        instance.updateDiffView();
+
+        expect(instance.instance.updateOptions).not.toHaveBeenCalled();
+      });
+    });
+
+    describe('diff mode', () => {
+      beforeEach(() => {
+        instance.createDiffInstance(holder);
+
+        spyOn(instance.instance, 'updateOptions').and.callThrough();
+      });
+
+      it('sets renderSideBySide to false if el is less than 700 pixels', () => {
+        spyOnProperty(instance.instance.getDomNode(), 'offsetWidth').and.returnValue(600);
+
+        expect(instance.instance.updateOptions).not.toHaveBeenCalledWith({
+          renderSideBySide: false,
+        });
+      });
+
+      it('sets renderSideBySide to false if el is more than 700 pixels', () => {
+        spyOnProperty(instance.instance.getDomNode(), 'offsetWidth').and.returnValue(800);
+
+        expect(instance.instance.updateOptions).not.toHaveBeenCalledWith({
+          renderSideBySide: true,
+        });
+      });
+    });
+  });
+
+  describe('isDiffEditorType', () => {
+    it('returns true when diff editor', () => {
+      instance.createDiffInstance(holder);
+
+      expect(instance.isDiffEditorType).toBe(true);
+    });
+
+    it('returns false when not diff editor', () => {
+      instance.createInstance(holder);
+
+      expect(instance.isDiffEditorType).toBe(false);
+    });
+  });
 });