Commit 359d9460 authored by Grzegorz Bizon's avatar Grzegorz Bizon

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
parents 104df74f 893d08c0
...@@ -89,3 +89,8 @@ class @AwardsHandler ...@@ -89,3 +89,8 @@ class @AwardsHandler
findEmojiIcon: (emoji) -> findEmojiIcon: (emoji) ->
$(".icon[data-emoji='" + emoji + "']") $(".icon[data-emoji='" + emoji + "']")
scrollToAwards: ->
$('body, html').animate({
scrollTop: $('.awards').offset().top - 80
}, 200)
class @Flash class @Flash
constructor: (message, type)-> constructor: (message, type)->
flash = $(".flash-container") @flash = $(".flash-container")
flash.html("") @flash.html("")
$('<div/>', innerDiv = $('<div/>',
class: "flash-#{type}", class: "flash-#{type}",
text: message text: message
).appendTo(".flash-container") )
innerDiv.appendTo(".flash-container")
flash.click -> $(@).fadeOut() @flash.click -> $(@).fadeOut()
flash.show() @flash.show()
pin: ->
@flash.addClass('flash-pinned flash-raised')
...@@ -111,6 +111,12 @@ class @Notes ...@@ -111,6 +111,12 @@ class @Notes
Note: for rendering inline notes use renderDiscussionNote Note: for rendering inline notes use renderDiscussionNote
### ###
renderNote: (note) -> 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 # render note if it not present in loaded list
# or skip if rendered # or skip if rendered
if @isNewNote(note) && !note.award if @isNewNote(note) && !note.award
...@@ -122,6 +128,7 @@ class @Notes ...@@ -122,6 +128,7 @@ class @Notes
if note.award if note.award
awards_handler.addAwardToEmojiBar(note.note, note.emoji_path) awards_handler.addAwardToEmojiBar(note.note, note.emoji_path)
awards_handler.scrollToAwards()
### ###
Check if note does not exists on page Check if note does not exists on page
......
...@@ -15,3 +15,13 @@ ...@@ -15,3 +15,13 @@
@extend .alert-danger; @extend .alert-danger;
} }
} }
.flash-pinned {
position: fixed;
top: 80px;
width: 80%;
}
.flash-raised {
z-index: 1000;
}
...@@ -131,7 +131,9 @@ class Projects::NotesController < Projects::ApplicationController ...@@ -131,7 +131,9 @@ class Projects::NotesController < Projects::ApplicationController
end end
def render_note_json(note) def render_note_json(note)
if note.valid?
render json: { render json: {
valid: true,
id: note.id, id: note.id,
discussion_id: note.discussion_id, discussion_id: note.discussion_id,
html: note_to_html(note), html: note_to_html(note),
...@@ -141,6 +143,13 @@ class Projects::NotesController < Projects::ApplicationController ...@@ -141,6 +143,13 @@ class Projects::NotesController < Projects::ApplicationController
discussion_html: note_to_discussion_html(note), discussion_html: note_to_discussion_html(note),
discussion_with_diff_html: note_to_discussion_with_diff_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 end
def authorize_admin_note! def authorize_admin_note!
......
...@@ -39,8 +39,11 @@ class Note < ActiveRecord::Base ...@@ -39,8 +39,11 @@ class Note < ActiveRecord::Base
delegate :name, to: :project, prefix: true delegate :name, to: :project, prefix: true
delegate :name, :email, to: :author, prefix: true delegate :name, :email, to: :author, prefix: true
before_validation :set_award!
validates :note, :project, presence: true validates :note, :project, presence: true
validates :note, uniqueness: { scope: [:author, :noteable_type, :noteable_id] }, if: ->(n) { n.is_award } 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 validates :line_code, format: { with: /\A[a-z0-9]+_\d+_\d+\Z/ }, allow_blank: true
# Attachments are deprecated and are handled by Markdown uploader # Attachments are deprecated and are handled by Markdown uploader
validates :attachment, file_size: { maximum: :max_attachment_size } validates :attachment, file_size: { maximum: :max_attachment_size }
...@@ -348,4 +351,31 @@ class Note < ActiveRecord::Base ...@@ -348,4 +351,31 @@ class Note < ActiveRecord::Base
def editable? def editable?
!system? !system?
end 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 end
...@@ -5,11 +5,6 @@ module Notes ...@@ -5,11 +5,6 @@ module Notes
note.author = current_user note.author = current_user
note.system = false note.system = false
if contains_emoji_only?(params[:note])
note.is_award = true
note.note = emoji_name(params[:note])
end
if note.save if note.save
notification_service.new_note(note) notification_service.new_note(note)
...@@ -33,13 +28,5 @@ module Notes ...@@ -33,13 +28,5 @@ module Notes
note.project.execute_hooks(note_data, :note_hooks) note.project.execute_hooks(note_data, :note_hooks)
note.project.execute_services(note_data, :note_hooks) note.project.execute_services(note_data, :note_hooks)
end 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
end end
...@@ -13,6 +13,12 @@ Feature: Project Commits Diff Comments ...@@ -13,6 +13,12 @@ Feature: Project Commits Diff Comments
Given I leave a diff comment like "Typo, please fix" Given I leave a diff comment like "Typo, please fix"
Then I should see a diff comment saying "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 @javascript
Scenario: I get a temporary form for the first comment on a diff line Scenario: I get a temporary form for the first comment on a diff line
Given I open a diff comment form Given I open a diff comment form
......
...@@ -12,3 +12,7 @@ Feature: Award Emoji ...@@ -12,3 +12,7 @@ Feature: Award Emoji
Then I have award added Then I have award added
And I can remove it by clicking to icon 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
...@@ -9,33 +9,40 @@ class Spinach::Features::AwardEmoji < Spinach::FeatureSteps ...@@ -9,33 +9,40 @@ class Spinach::Features::AwardEmoji < Spinach::FeatureSteps
end end
step 'I click to emoji-picker' do step 'I click to emoji-picker' do
page.within ".awards-controls" do page.within '.awards-controls' do
page.find(".add-award").click page.find('.add-award').click
end end
end end
step 'I click to emoji in the picker' do step 'I click to emoji in the picker' do
page.within ".awards-menu" do page.within '.awards-menu' do
page.first("img").click page.first('img').click
end end
end end
step 'I can remove it by clicking to icon' do step 'I can remove it by clicking to icon' do
page.within ".awards" do page.within '.awards' do
page.first(".award").click page.first('.award').click
expect(page).to_not have_selector ".award" expect(page).to_not have_selector '.award'
end end
end end
step 'I have award added' do step 'I have award added' do
page.within ".awards" do page.within '.awards' do
expect(page).to have_selector ".award" expect(page).to have_selector '.award'
expect(page.find(".award .counter")).to have_content "1" expect(page.find('.award .counter')).to have_content '1'
end end
end end
step 'project "Shop" has issue "Bugfix"' do step 'project "Shop" has issue "Bugfix"' do
@project = Project.find_by(name: "Shop") @project = Project.find_by(name: 'Shop')
@issue = create(:issue, title: "Bugfix", project: project) @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
end end
...@@ -87,6 +87,17 @@ module SharedDiffNote ...@@ -87,6 +87,17 @@ module SharedDiffNote
end end
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 step 'I submit the diff comment' do
page.within(diff_file_selector) do page.within(diff_file_selector) do
click_button("Add Comment") click_button("Add Comment")
...@@ -197,6 +208,12 @@ module SharedDiffNote ...@@ -197,6 +208,12 @@ module SharedDiffNote
end end
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 step 'I click side-by-side diff button' do
find('#parallel-diff-btn').trigger('click') find('#parallel-diff-btn').trigger('click')
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment