Merge branch '27614-improve-instant-comments-exp' into 'master'

Improve user experience around slash commands in instant comments

Closes #32464

See merge request !11612

app/assets/javascripts/gfm_auto_complete.js

@@ -2,6 +2,7 @@ import emojiMap from 'emojis/digests.json';
 import emojiAliases from 'emojis/aliases.json';
 import { glEmojiTag } from '~/behaviors/gl_emoji';
 import glRegexp from '~/lib/utils/regexp';
+import AjaxCache from '~/lib/utils/ajax_cache';
 
 function sanitize(str) {
   return str.replace(/<(?:.|\n)*?>/gm, '');
@@ -35,6 +36,7 @@ class GfmAutoComplete {
       // This triggers at.js again
       // Needed for slash commands with suffixes (ex: /label ~)
       $input.on('inserted-commands.atwho', $input.trigger.bind($input, 'keyup'));
+      $input.on('clear-commands-cache.atwho', () => this.clearCache());
     });
   }
 
@@ -375,11 +377,14 @@ class GfmAutoComplete {
     } else if (GfmAutoComplete.atTypeMap[at] === 'emojis') {
       this.loadData($input, at, Object.keys(emojiMap).concat(Object.keys(emojiAliases)));
     } else {
-      $.getJSON(this.dataSources[GfmAutoComplete.atTypeMap[at]], (data) => {
-        this.loadData($input, at, data);
-      }).fail(() => { this.isLoadingData[at] = false; });
+      AjaxCache.retrieve(this.dataSources[GfmAutoComplete.atTypeMap[at]], true)
+        .then((data) => {
+          this.loadData($input, at, data);
+        })
+        .catch(() => { this.isLoadingData[at] = false; });
     }
   }
+
   loadData($input, at, data) {
     this.isLoadingData[at] = false;
     this.cachedData[at] = data;
@@ -389,6 +394,10 @@ class GfmAutoComplete {
     return $input.trigger('keyup');
   }
 
+  clearCache() {
+    this.cachedData = {};
+  }
+
   static isLoading(data) {
     let dataToInspect = data;
     if (data && data.length > 0) {

app/assets/javascripts/lib/utils/ajax_cache.js

@@ -6,8 +6,8 @@ class AjaxCache extends Cache {
     this.pendingRequests = { };
   }
 
-  retrieve(endpoint) {
-    if (this.hasData(endpoint)) {
+  retrieve(endpoint, forceRetrieve) {
+    if (this.hasData(endpoint) && !forceRetrieve) {
       return Promise.resolve(this.get(endpoint));
     }
 

app/assets/javascripts/notes.js

@@ -16,6 +16,7 @@ import autosize from 'vendor/autosize';
 import Dropzone from 'dropzone';
 import 'vendor/jquery.caret'; // required by jquery.atwho
 import 'vendor/jquery.atwho';
+import AjaxCache from '~/lib/utils/ajax_cache';
 import CommentTypeToggle from './comment_type_toggle';
 import './autosave';
 import './dropzone_input';
@@ -66,7 +67,6 @@ const normalizeNewlines = function(str) {
       this.notesCountBadge || (this.notesCountBadge = $('.issuable-details').find('.notes-tab .badge'));
       this.basePollingInterval = 15000;
       this.maxPollingSteps = 4;
-      this.flashErrors = [];
 
       this.cleanBinding();
       this.addBinding();
@@ -325,6 +325,9 @@ const normalizeNewlines = function(str) {
       if (Notes.isNewNote(noteEntity, this.note_ids)) {
         this.note_ids.push(noteEntity.id);
 
+        if ($notesList.length) {
+          $notesList.find('.system-note.being-posted').remove();
+        }
         const $newNote = Notes.animateAppendNote(noteEntity.html, $notesList);
 
         this.setupNewNote($newNote);
@@ -1118,12 +1121,14 @@ const normalizeNewlines = function(str) {
     };
 
     Notes.prototype.addFlash = function(...flashParams) {
-      this.flashErrors.push(new Flash(...flashParams));
+      this.flashInstance = new Flash(...flashParams);
     };
 
     Notes.prototype.clearFlash = function() {
-      this.flashErrors.forEach(flash => flash.flashContainer.remove());
-      this.flashErrors = [];
+      if (this.flashInstance && this.flashInstance.flashContainer) {
+        this.flashInstance.flashContainer.hide();
+        this.flashInstance = null;
+      }
     };
 
     Notes.prototype.cleanForm = function($form) {
@@ -1187,7 +1192,7 @@ const normalizeNewlines = function(str) {
     Notes.prototype.getFormData = function($form) {
       return {
         formData: $form.serialize(),
-        formContent: $form.find('.js-note-text').val(),
+        formContent: _.escape($form.find('.js-note-text').val()),
         formAction: $form.attr('action'),
       };
     };
@@ -1206,20 +1211,47 @@ const normalizeNewlines = function(str) {
       return formContent.replace(REGEX_SLASH_COMMANDS, '').trim();
     };
 
+    /**
+     * Gets appropriate description from slash commands found in provided `formContent`
+     */
+    Notes.prototype.getSlashCommandDescription = function (formContent, availableSlashCommands = []) {
+      let tempFormContent;
+
+      // Identify executed slash commands from `formContent`
+      const executedCommands = availableSlashCommands.filter((command, index) => {
+        const commandRegex = new RegExp(`/${command.name}`);
+        return commandRegex.test(formContent);
+      });
+
+      if (executedCommands && executedCommands.length) {
+        if (executedCommands.length > 1) {
+          tempFormContent = 'Applying multiple commands';
+        } else {
+          const commandDescription = executedCommands[0].description.toLowerCase();
+          tempFormContent = `Applying command to ${commandDescription}`;
+        }
+      } else {
+        tempFormContent = 'Applying command';
+      }
+
+      return tempFormContent;
+    };
+
     /**
      * Create placeholder note DOM element populated with comment body
      * that we will show while comment is being posted.
      * Once comment is _actually_ posted on server, we will have final element
      * in response that we will show in place of this temporary element.
      */
-    Notes.prototype.createPlaceholderNote = function({ formContent, uniqueId, isDiscussionNote, currentUsername, currentUserFullname }) {
+    Notes.prototype.createPlaceholderNote = function ({ formContent, uniqueId, isDiscussionNote, currentUsername, currentUserFullname, currentUserAvatar }) {
       const discussionClass = isDiscussionNote ? 'discussion' : '';
-      const escapedFormContent = _.escape(formContent);
       const $tempNote = $(
         `<li id="${uniqueId}" class="note being-posted fade-in-half timeline-entry">
            <div class="timeline-entry-inner">
               <div class="timeline-icon">
-                 <a href="/${currentUsername}"><span class="avatar dummy-avatar"></span></a>
+                 <a href="/${currentUsername}">
+                   <img class="avatar s40" src="${currentUserAvatar}">
+                 </a>
               </div>
               <div class="timeline-content ${discussionClass}">
                  <div class="note-header">
@@ -1232,7 +1264,7 @@ const normalizeNewlines = function(str) {
                  </div>
                  <div class="note-body">
                    <div class="note-text">
-                     <p>${escapedFormContent}</p>
+                     <p>${formContent}</p>
                    </div>
                  </div>
               </div>
@@ -1243,6 +1275,23 @@ const normalizeNewlines = function(str) {
       return $tempNote;
     };
 
+    /**
+     * Create Placeholder System Note DOM element populated with slash command description
+     */
+    Notes.prototype.createPlaceholderSystemNote = function ({ formContent, uniqueId }) {
+      const $tempNote = $(
+        `<li id="${uniqueId}" class="note system-note timeline-entry being-posted fade-in-half">
+           <div class="timeline-entry-inner">
+             <div class="timeline-content">
+               <i>${formContent}</i>
+             </div>
+           </div>
+         </li>`
+      );
+
+      return $tempNote;
+    };
+
     /**
      * This method does following tasks step-by-step whenever a new comment
      * is submitted by user (both main thread comments as well as discussion comments).
@@ -1274,7 +1323,9 @@ const normalizeNewlines = function(str) {
       const isDiscussionForm = $form.hasClass('js-discussion-note-form');
       const isDiscussionResolve = $submitBtn.hasClass('js-comment-resolve-button');
       const { formData, formContent, formAction } = this.getFormData($form);
-      const uniqueId = _.uniqueId('tempNote_');
+      let noteUniqueId;
+      let systemNoteUniqueId;
+      let hasSlashCommands = false;
       let $notesContainer;
       let tempFormContent;
 
@@ -1295,16 +1346,28 @@ const normalizeNewlines = function(str) {
       tempFormContent = formContent;
       if (this.hasSlashCommands(formContent)) {
         tempFormContent = this.stripSlashCommands(formContent);
+        hasSlashCommands = true;
       }
 
+      // Show placeholder note
       if (tempFormContent) {
-        // Show placeholder note
+        noteUniqueId = _.uniqueId('tempNote_');
         $notesContainer.append(this.createPlaceholderNote({
           formContent: tempFormContent,
-          uniqueId,
+          uniqueId: noteUniqueId,
           isDiscussionNote,
           currentUsername: gon.current_username,
           currentUserFullname: gon.current_user_fullname,
+          currentUserAvatar: gon.current_user_avatar_url,
+        }));
+      }
+
+      // Show placeholder system note
+      if (hasSlashCommands) {
+        systemNoteUniqueId = _.uniqueId('tempSystemNote_');
+        $notesContainer.append(this.createPlaceholderSystemNote({
+          formContent: this.getSlashCommandDescription(formContent, AjaxCache.get(gl.GfmAutoComplete.dataSources.commands)),
+          uniqueId: systemNoteUniqueId,
         }));
       }
 
@@ -1322,7 +1385,13 @@ const normalizeNewlines = function(str) {
       gl.utils.ajaxPost(formAction, formData)
         .then((note) => {
           // Submission successful! remove placeholder
-          $notesContainer.find(`#${uniqueId}`).remove();
+          $notesContainer.find(`#${noteUniqueId}`).remove();
+
+          // Reset cached commands list when command is applied
+          if (hasSlashCommands) {
+            $form.find('textarea.js-note-text').trigger('clear-commands-cache.atwho');
+          }
+
           // Clear previous form errors
           this.clearFlashWrapper();
 
@@ -1359,7 +1428,11 @@ const normalizeNewlines = function(str) {
           $form.trigger('ajax:success', [note]);
         }).fail(() => {
           // Submission failed, remove placeholder note and show Flash error message
-          $notesContainer.find(`#${uniqueId}`).remove();
+          $notesContainer.find(`#${noteUniqueId}`).remove();
+
+          if (hasSlashCommands) {
+            $notesContainer.find(`#${systemNoteUniqueId}`).remove();
+          }
 
           // Show form again on UI on failure
           if (isDiscussionForm && $notesContainer.length) {

changelogs/unreleased/27614-improve-instant-comments-exp.yml

+---
+title: Improve user experience around slash commands in instant comments
+merge_request: 11612
+author:

spec/javascripts/lib/utils/ajax_cache_spec.js

@@ -154,5 +154,36 @@ describe('AjaxCache', () => {
       .then(done)
       .catch(fail);
     });
+
+    it('makes Ajax call even if matching data exists when forceRequest parameter is provided', (done) => {
+      const oldDummyResponse = {
+        important: 'old dummy data',
+      };
+
+      AjaxCache.internalStorage[dummyEndpoint] = oldDummyResponse;
+
+      ajaxSpy = (url) => {
+        expect(url).toBe(dummyEndpoint);
+        const deferred = $.Deferred();
+        deferred.resolve(dummyResponse);
+        return deferred.promise();
+      };
+
+      // Call without forceRetrieve param
+      AjaxCache.retrieve(dummyEndpoint)
+        .then((data) => {
+          expect(data).toBe(oldDummyResponse);
+        })
+        .then(done)
+        .catch(fail);
+
+      // Call with forceRetrieve param
+      AjaxCache.retrieve(dummyEndpoint, true)
+        .then((data) => {
+          expect(data).toBe(dummyResponse);
+        })
+        .then(done)
+        .catch(fail);
+    });
   });
 });

spec/javascripts/notes_spec.js

@@ -13,6 +13,23 @@ import '~/notes';
   window.gl = window.gl || {};
   gl.utils = gl.utils || {};
 
+  const htmlEscape = (comment) => {
+    const escapedString = comment.replace(/["&'<>]/g, (a) => {
+      const escapedToken = {
+        '&': '&amp;',
+        '<': '&lt;',
+        '>': '&gt;',
+        '"': '&quot;',
+        "'": '&#x27;',
+        '`': '&#x60;'
+      }[a];
+
+      return escapedToken;
+    });
+
+    return escapedString;
+  };
+
   describe('Notes', function() {
     const FLASH_TYPE_ALERT = 'alert';
     var commentsTemplate = 'issues/issue_with_comment.html.raw';
@@ -445,11 +462,17 @@ import '~/notes';
     });
 
     describe('getFormData', () => {
-      it('should return form metadata object from form reference', () => {
+      let $form;
+      let sampleComment;
+
+      beforeEach(() => {
         this.notes = new Notes('', []);
 
-        const $form = $('form');
-        const sampleComment = 'foobar';
+        $form = $('form');
+        sampleComment = 'foobar';
+      });
+
+      it('should return form metadata object from form reference', () => {
         $form.find('textarea.js-note-text').val(sampleComment);
         const { formData, formContent, formAction } = this.notes.getFormData($form);
 
@@ -457,6 +480,18 @@ import '~/notes';
         expect(formContent).toEqual(sampleComment);
         expect(formAction).toEqual($form.attr('action'));
       });
+
+      it('should return form metadata with sanitized formContent from form reference', () => {
+        spyOn(_, 'escape').and.callFake(htmlEscape);
+
+        sampleComment = '<script>alert("Boom!");</script>';
+        $form.find('textarea.js-note-text').val(sampleComment);
+
+        const { formContent } = this.notes.getFormData($form);
+
+        expect(_.escape).toHaveBeenCalledWith(sampleComment);
+        expect(formContent).toEqual('&lt;script&gt;alert(&quot;Boom!&quot;);&lt;/script&gt;');
+      });
     });
 
     describe('hasSlashCommands', () => {
@@ -512,30 +547,42 @@ import '~/notes';
       });
     });
 
+    describe('getSlashCommandDescription', () => {
+      const availableSlashCommands = [
+        { name: 'close', description: 'Close this issue', params: [] },
+        { name: 'title', description: 'Change title', params: [{}] },
+        { name: 'estimate', description: 'Set time estimate', params: [{}] }
+      ];
+
+      beforeEach(() => {
+        this.notes = new Notes();
+      });
+
+      it('should return executing slash command description when note has single slash command', () => {
+        const sampleComment = '/close';
+        expect(this.notes.getSlashCommandDescription(sampleComment, availableSlashCommands)).toBe('Applying command to close this issue');
+      });
+
+      it('should return generic multiple slash command description when note has multiple slash commands', () => {
+        const sampleComment = '/close\n/title [Duplicate] Issue foobar';
+        expect(this.notes.getSlashCommandDescription(sampleComment, availableSlashCommands)).toBe('Applying multiple commands');
+      });
+
+      it('should return generic slash command description when available slash commands list is not populated', () => {
+        const sampleComment = '/close\n/title [Duplicate] Issue foobar';
+        expect(this.notes.getSlashCommandDescription(sampleComment)).toBe('Applying command');
+      });
+    });
+
     describe('createPlaceholderNote', () => {
       const sampleComment = 'foobar';
       const uniqueId = 'b1234-a4567';
       const currentUsername = 'root';
       const currentUserFullname = 'Administrator';
+      const currentUserAvatar = 'avatar_url';
 
       beforeEach(() => {
         this.notes = new Notes('', []);
-        spyOn(_, 'escape').and.callFake((comment) => {
-          const escapedString = comment.replace(/["&'<>]/g, (a) => {
-            const escapedToken = {
-              '&': '&amp;',
-              '<': '&lt;',
-              '>': '&gt;',
-              '"': '&quot;',
-              "'": '&#x27;',
-              '`': '&#x60;'
-            }[a];
-
-            return escapedToken;
-          });
-
-          return escapedString;
-        });
       });
 
       it('should return constructed placeholder element for regular note based on form contents', () => {
@@ -544,46 +591,59 @@ import '~/notes';
           uniqueId,
           isDiscussionNote: false,
           currentUsername,
-          currentUserFullname
+          currentUserFullname,
+          currentUserAvatar,
         });
         const $tempNoteHeader = $tempNote.find('.note-header');
 
         expect($tempNote.prop('nodeName')).toEqual('LI');
         expect($tempNote.attr('id')).toEqual(uniqueId);
+        expect($tempNote.hasClass('being-posted')).toBeTruthy();
+        expect($tempNote.hasClass('fade-in-half')).toBeTruthy();
         $tempNote.find('.timeline-icon > a, .note-header-info > a').each(function() {
           expect($(this).attr('href')).toEqual(`/${currentUsername}`);
         });
+        expect($tempNote.find('.timeline-icon .avatar').attr('src')).toEqual(currentUserAvatar);
         expect($tempNote.find('.timeline-content').hasClass('discussion')).toBeFalsy();
         expect($tempNoteHeader.find('.hidden-xs').text().trim()).toEqual(currentUserFullname);
         expect($tempNoteHeader.find('.note-headline-light').text().trim()).toEqual(`@${currentUsername}`);
         expect($tempNote.find('.note-body .note-text p').text().trim()).toEqual(sampleComment);
       });
 
-      it('should escape HTML characters from note based on form contents', () => {
-        const commentWithHtml = '<script>alert("Boom!");</script>';
+      it('should return constructed placeholder element for discussion note based on form contents', () => {
         const $tempNote = this.notes.createPlaceholderNote({
-          formContent: commentWithHtml,
+          formContent: sampleComment,
           uniqueId,
-          isDiscussionNote: false,
+          isDiscussionNote: true,
           currentUsername,
           currentUserFullname
         });
 
-        expect(_.escape).toHaveBeenCalledWith(commentWithHtml);
-        expect($tempNote.find('.note-body .note-text p').html()).toEqual('&lt;script&gt;alert("Boom!");&lt;/script&gt;');
+        expect($tempNote.prop('nodeName')).toEqual('LI');
+        expect($tempNote.find('.timeline-content').hasClass('discussion')).toBeTruthy();
       });
+    });
 
-      it('should return constructed placeholder element for discussion note based on form contents', () => {
-        const $tempNote = this.notes.createPlaceholderNote({
-          formContent: sampleComment,
+    describe('createPlaceholderSystemNote', () => {
+      const sampleCommandDescription = 'Applying command to close this issue';
+      const uniqueId = 'b1234-a4567';
+
+      beforeEach(() => {
+        this.notes = new Notes('', []);
+        spyOn(_, 'escape').and.callFake(htmlEscape);
+      });
+
+      it('should return constructed placeholder element for system note based on form contents', () => {
+        const $tempNote = this.notes.createPlaceholderSystemNote({
+          formContent: sampleCommandDescription,
           uniqueId,
-          isDiscussionNote: true,
-          currentUsername,
-          currentUserFullname
         });
 
         expect($tempNote.prop('nodeName')).toEqual('LI');
-        expect($tempNote.find('.timeline-content').hasClass('discussion')).toBeTruthy();
+        expect($tempNote.attr('id')).toEqual(uniqueId);
+        expect($tempNote.hasClass('being-posted')).toBeTruthy();
+        expect($tempNote.hasClass('fade-in-half')).toBeTruthy();
+        expect($tempNote.find('.timeline-content i').text().trim()).toEqual(sampleCommandDescription);
       });
     });
 
@@ -595,7 +655,7 @@ import '~/notes';
       it('shows a flash message', () => {
         this.notes.addFlash('Error message', FLASH_TYPE_ALERT, this.notes.parentTimeline);
 
-        expect(document.querySelectorAll('.flash-alert').length).toBe(1);
+        expect($('.flash-alert').is(':visible')).toBeTruthy();
       });
     });
 
@@ -605,13 +665,12 @@ import '~/notes';
         this.notes = new Notes();
       });
 
-      it('removes all the associated flash messages', () => {
+      it('hides visible flash message', () => {
         this.notes.addFlash('Error message 1', FLASH_TYPE_ALERT, this.notes.parentTimeline);
-        this.notes.addFlash('Error message 2', FLASH_TYPE_ALERT, this.notes.parentTimeline);
 
         this.notes.clearFlash();
 
-        expect(document.querySelectorAll('.flash-alert').length).toBe(0);
+        expect($('.flash-alert').is(':visible')).toBeFalsy();
       });
     });
   });