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

app/assets/javascripts/awards_handler.coffee

@@ -89,3 +89,8 @@ class @AwardsHandler
 
   findEmojiIcon: (emoji) ->
     $(".icon[data-emoji='" + emoji + "']")
+
+  scrollToAwards: ->
+    $('body, html').animate({
+      scrollTop: $('.awards').offset().top - 80
+    }, 200)

app/assets/javascripts/flash.js.coffee

 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')

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

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;
+}

app/controllers/projects/notes_controller.rb

@@ -131,7 +131,9 @@ class Projects::NotesController < Projects::ApplicationController
   end
 
   def render_note_json(note)
+    if note.valid?
       render json: {
+        valid: true,
         id: note.id,
         discussion_id: note.discussion_id,
         html: note_to_html(note),
@@ -141,6 +143,13 @@ class Projects::NotesController < Projects::ApplicationController
         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!

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

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

features/project/commits/diff_comments.feature

@@ -13,6 +13,12 @@ Feature: Project Commits Diff Comments
     Given I leave a diff comment like "Typo, please fix"
     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

features/project/issues/award_emoji.feature

@@ -12,3 +12,7 @@ Feature: Award Emoji
     Then I have award added
     And I can remove it by clicking to icon
 
+  @javascript
+  Scenario: I add award emoji using regular comment
+  Given I leave comment with a single emoji
+  Then I have award added

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

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