Merge branch 'fix/award-emoji-conflict-in-notes' into 'master'

Fix problems with award-emoji-only comment

This fixes a conflict between note with only a single emoji in content
and award-emojis mechanisms.

Closes #3734 

cc @vsizov

See merge request !1936

11 files changed, 129 insertions, 43 deletions

diff --git a/app/assets/javascripts/awards_handler.coffee b/app/assets/javascripts/awards_handler.coffee
index 09b48fe5572..96fd8f8773e 100644
--- a/app/assets/javascripts/awards_handler.coffee
+++ b/app/assets/javascripts/awards_handler.coffee
@@ -88,4 +88,9 @@ class @AwardsHandler
         callback.call()
 
   findEmojiIcon: (emoji) ->
-    $(".icon[data-emoji='" + emoji + "']")
\ No newline at end of file
+    $(".icon[data-emoji='" + emoji + "']")
+
+  scrollToAwards: ->
+    $('body, html').animate({
+      scrollTop: $('.awards').offset().top - 80
+    }, 200)
diff --git a/app/assets/javascripts/flash.js.coffee b/app/assets/javascripts/flash.js.coffee
index b39ab0c4475..9b59d4e57f7 100644
--- a/app/assets/javascripts/flash.js.coffee
+++ b/app/assets/javascripts/flash.js.coffee
@@ -1,12 +1,16 @@
 class @Flash
   constructor: (message, type)->
-    flash = $(".flash-container")
-    flash.html("")
+    @flash = $(".flash-container")
+    @flash.html("")
 
-    $('<div/>',
+    innerDiv = $('<div/>',
       class: "flash-#{type}",
       text: message
-    ).appendTo(".flash-container")
+    )
+    innerDiv.appendTo(".flash-container")
 
-    flash.click -> $(@).fadeOut()
-    flash.show()
+    @flash.click -> $(@).fadeOut()
+    @flash.show()
+
+  pin: ->
+    @flash.addClass('flash-pinned flash-raised')
diff --git a/app/assets/javascripts/notes.js.coffee b/app/assets/javascripts/notes.js.coffee
index 7de7632201d..dd6cbcfc70b 100644
--- a/app/assets/javascripts/notes.js.coffee
+++ b/app/assets/javascripts/notes.js.coffee
@@ -111,6 +111,12 @@ class @Notes
   Note: for rendering inline notes use renderDiscussionNote
   ###
   renderNote: (note) ->
+    unless note.valid
+      if note.award
+        flash = new Flash('You have already used this award emoji!', 'alert')
+        flash.pin()
+      return
+
     # render note if it not present in loaded list
     # or skip if rendered
     if @isNewNote(note) && !note.award
@@ -122,6 +128,7 @@ class @Notes
 
     if note.award
       awards_handler.addAwardToEmojiBar(note.note, note.emoji_path)
+      awards_handler.scrollToAwards()
 
   ###
   Check if note does not exists on page
diff --git a/app/assets/stylesheets/framework/flash.scss b/app/assets/stylesheets/framework/flash.scss
index 82eb50ad4be..1b723021d76 100644
--- a/app/assets/stylesheets/framework/flash.scss
+++ b/app/assets/stylesheets/framework/flash.scss
@@ -15,3 +15,13 @@
     @extend .alert-danger;
   }
 }
+
+.flash-pinned {
+  position: fixed;
+  top: 80px;
+  width: 80%;
+}
+
+.flash-raised {
+  z-index: 1000;
+}
diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb
index 5ac18446aa7..88b949a27ab 100644
--- a/app/controllers/projects/notes_controller.rb
+++ b/app/controllers/projects/notes_controller.rb
@@ -131,16 +131,25 @@ class Projects::NotesController < Projects::ApplicationController
   end
 
   def render_note_json(note)
-    render json: {
-      id: note.id,
-      discussion_id: note.discussion_id,
-      html: note_to_html(note),
-      award: note.is_award,
-      emoji_path: note.is_award ? view_context.image_url(::AwardEmoji.path_to_emoji_image(note.note)) : "",
-      note: note.note,
-      discussion_html: note_to_discussion_html(note),
-      discussion_with_diff_html: note_to_discussion_with_diff_html(note)
-    }
+    if note.valid?
+      render json: {
+        valid: true,
+        id: note.id,
+        discussion_id: note.discussion_id,
+        html: note_to_html(note),
+        award: note.is_award,
+        emoji_path: note.is_award ? view_context.image_url(::AwardEmoji.path_to_emoji_image(note.note)) : "",
+        note: note.note,
+        discussion_html: note_to_discussion_html(note),
+        discussion_with_diff_html: note_to_discussion_with_diff_html(note)
+      }
+    else
+      render json: {
+        valid: false,
+        award: note.is_award,
+        errors: note.errors
+      }
+    end
   end
 
   def authorize_admin_note!
diff --git a/app/models/note.rb b/app/models/note.rb
index 1c6345e735c..239a0f77f8e 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -39,8 +39,11 @@ class Note < ActiveRecord::Base
   delegate :name, to: :project, prefix: true
   delegate :name, :email, to: :author, prefix: true
 
+  before_validation :set_award!
+
   validates :note, :project, presence: true
   validates :note, uniqueness: { scope: [:author, :noteable_type, :noteable_id] }, if: ->(n) { n.is_award }
+  validates :note, inclusion: { in: Emoji.emojis_names }, if: ->(n) { n.is_award }
   validates :line_code, format: { with: /\A[a-z0-9]+_\d+_\d+\Z/ }, allow_blank: true
   # Attachments are deprecated and are handled by Markdown uploader
   validates :attachment, file_size: { maximum: :max_attachment_size }
@@ -348,4 +351,31 @@ class Note < ActiveRecord::Base
   def editable?
     !system?
   end
+
+  # Checks if note is an award added as a comment
+  #
+  # If note is an award, this method sets is_award to true
+  #   and changes content of the note to award name.
+  #
+  # Method is executed as a before_validation callback.
+  #
+  def set_award!
+    return unless awards_supported? && contains_emoji_only?
+    self.is_award = true
+    self.note = award_emoji_name
+  end
+
+  private
+
+  def awards_supported?
+    noteable.kind_of?(Issue) || noteable.is_a?(MergeRequest)
+  end
+
+  def contains_emoji_only?
+    note =~ /\A#{Gitlab::Markdown::EmojiFilter.emoji_pattern}\s?\Z/
+  end
+
+  def award_emoji_name
+    note.match(Gitlab::Markdown::EmojiFilter.emoji_pattern)[1]
+  end
 end
diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb
index dbff58dfb9c..a8486e6a5a1 100644
--- a/app/services/notes/create_service.rb
+++ b/app/services/notes/create_service.rb
@@ -5,11 +5,6 @@ module Notes
       note.author = current_user
       note.system = false
 
-      if contains_emoji_only?(params[:note])
-        note.is_award = true
-        note.note = emoji_name(params[:note])
-      end
-
       if note.save
         notification_service.new_note(note)
 
@@ -33,13 +28,5 @@ module Notes
       note.project.execute_hooks(note_data, :note_hooks)
       note.project.execute_services(note_data, :note_hooks)
     end
-
-    def contains_emoji_only?(note)
-      note =~ /\A:[-_+[:alnum:]]*:\s?\z/
-    end
-
-    def emoji_name(note)
-      note.match(/\A:([-_+[:alnum:]]*):\s?/)[1]
-    end
   end
 end
diff --git a/features/project/commits/diff_comments.feature b/features/project/commits/diff_comments.feature
index 4a2b870e082..d6e0c84537e 100644
--- a/features/project/commits/diff_comments.feature
+++ b/features/project/commits/diff_comments.feature
@@ -14,6 +14,12 @@ Feature: Project Commits Diff Comments
     Then I should see a diff comment saying "Typo, please fix"
 
   @javascript
+  Scenario: I can add a diff comment with a single emoji
+    Given I open a diff comment form
+    And I write a diff comment like ":smile:"
+    Then I should see a diff comment with an emoji image
+
+  @javascript
   Scenario: I get a temporary form for the first comment on a diff line
     Given I open a diff comment form
     Then I should see a temporary diff comment form
diff --git a/features/project/issues/award_emoji.feature b/features/project/issues/award_emoji.feature
index a9bc8ffb9bb..2609f129d07 100644
--- a/features/project/issues/award_emoji.feature
+++ b/features/project/issues/award_emoji.feature
@@ -11,4 +11,8 @@ Feature: Award Emoji
     And I click to emoji in the picker
     Then I have award added
     And I can remove it by clicking to icon
-    
\ No newline at end of file
+
+  @javascript
+  Scenario: I add award emoji using regular comment
+  Given I leave comment with a single emoji
+  Then I have award added
diff --git a/features/steps/project/issues/award_emoji.rb b/features/steps/project/issues/award_emoji.rb
index 8f7a45dec0e..325eaf2ea6a 100644
--- a/features/steps/project/issues/award_emoji.rb
+++ b/features/steps/project/issues/award_emoji.rb
@@ -9,33 +9,40 @@ class Spinach::Features::AwardEmoji < Spinach::FeatureSteps
   end
 
   step 'I click to emoji-picker' do
-    page.within ".awards-controls" do
-      page.find(".add-award").click
+    page.within '.awards-controls' do
+      page.find('.add-award').click
     end
   end
 
   step 'I click to emoji in the picker' do
-    page.within ".awards-menu" do
-      page.first("img").click
+    page.within '.awards-menu' do
+      page.first('img').click
     end
   end
 
   step 'I can remove it by clicking to icon' do
-    page.within ".awards" do
-      page.first(".award").click
-      expect(page).to_not have_selector ".award"
+    page.within '.awards' do
+      page.first('.award').click
+      expect(page).to_not have_selector '.award'
     end
   end
 
   step 'I have award added' do
-    page.within ".awards" do
-      expect(page).to have_selector ".award"
-      expect(page.find(".award .counter")).to have_content "1"
+    page.within '.awards' do
+      expect(page).to have_selector '.award'
+      expect(page.find('.award .counter')).to have_content '1'
     end
   end
 
   step 'project "Shop" has issue "Bugfix"' do
-    @project = Project.find_by(name: "Shop")
-    @issue = create(:issue, title: "Bugfix", project: project)
+    @project = Project.find_by(name: 'Shop')
+    @issue = create(:issue, title: 'Bugfix', project: project)
+  end
+
+  step 'I leave comment with a single emoji' do
+    page.within('.js-main-target-form') do
+      fill_in 'note[note]', with: ':smile:'
+      click_button 'Add Comment'
+    end
   end
 end
diff --git a/features/steps/shared/diff_note.rb b/features/steps/shared/diff_note.rb
index 72621911a37..dd466cde28d 100644
--- a/features/steps/shared/diff_note.rb
+++ b/features/steps/shared/diff_note.rb
@@ -87,6 +87,17 @@ module SharedDiffNote
     end
   end
 
+  step 'I write a diff comment like ":smile:"' do
+    page.within(diff_file_selector) do
+      click_diff_line(sample_commit.line_code)
+
+      page.within("form[rel$='#{sample_commit.line_code}']") do
+        fill_in 'note[note]', with: ':smile:'
+        click_button('Add Comment')
+      end
+    end
+  end
+
   step 'I submit the diff comment' do
     page.within(diff_file_selector) do
       click_button("Add Comment")
@@ -197,6 +208,12 @@ module SharedDiffNote
     end
   end
 
+  step 'I should see a diff comment with an emoji image' do
+    page.within("#{diff_file_selector} .note") do
+      expect(page).to have_xpath("//img[@alt=':smile:']")
+    end
+  end
+
   step 'I click side-by-side diff button' do
     find('#parallel-diff-btn').trigger('click')
   end