Commit 4558b5b9 authored by Zeger-Jan van de Weg's avatar Zeger-Jan van de Weg

Incorporate feedback

parent 2f0d89ec
...@@ -34,23 +34,23 @@ module Awardable ...@@ -34,23 +34,23 @@ module Awardable
end end
end end
def grouped_awards(with_thumbs = true) def grouped_awards(with_thumbs: true)
awards = award_emoji.group_by(&:name) awards = award_emoji.group_by(&:name)
if with_thumbs if with_thumbs
awards[AwardEmoji::UPVOTE_NAME] ||= AwardEmoji.none awards[AwardEmoji::UPVOTE_NAME] ||= []
awards[AwardEmoji::DOWNVOTE_NAME] ||= AwardEmoji.none awards[AwardEmoji::DOWNVOTE_NAME] ||= []
end end
awards awards
end end
def downvotes def downvotes
award_emoji.where(name: AwardEmoji::DOWNVOTE_NAME).count award_emoji.downvotes.count
end end
def upvotes def upvotes
award_emoji.where(name: AwardEmoji::UPVOTE_NAME).count award_emoji.upvotes.count
end end
def emoji_awardable? def emoji_awardable?
......
...@@ -323,10 +323,6 @@ class Note < ActiveRecord::Base ...@@ -323,10 +323,6 @@ class Note < ActiveRecord::Base
award_emoji_supported? && contains_emoji_only? award_emoji_supported? && contains_emoji_only?
end end
def create_award_emoji
self.noteable.award_emoji(award_emoji_name, author)
end
def clear_blank_line_code! def clear_blank_line_code!
self.line_code = nil if self.line_code.blank? self.line_code = nil if self.line_code.blank?
end end
......
...@@ -7,7 +7,7 @@ module Notes ...@@ -7,7 +7,7 @@ module Notes
if note.award_emoji? if note.award_emoji?
return ToggleAwardEmojiService.new(project, current_user, params). return ToggleAwardEmojiService.new(project, current_user, params).
execute(note.noteable, note.note) execute(note.award_emoji_name, note.note)
end end
return unless valid_project?(note) return unless valid_project?(note)
......
require_relative 'base_service' require_relative 'base_service'
class ToggleAwardEmojiService < BaseService class ToggleAwardEmojiService < BaseService
# For an award emoji being posted we should:
# - Mark the TODO as done for this issuable (skip on snippets)
# - Save the award emoji
def execute(awardable, emoji) def execute(awardable, emoji)
todo_service.new_award_emoji(awardable, current_user) todo_service.new_award_emoji(awardable, current_user)
# Needed if its posted as a note containing only :+1:
emoji = award_emoji_name(emoji) if emoji.start_with? ':'
awardable.toggle_award_emoji(emoji, current_user) awardable.toggle_award_emoji(emoji, current_user)
end end
private
def award_emoji_name(emoji)
original_name = emoji.match(Banzai::Filter::EmojiFilter.emoji_pattern)[1]
Gitlab::AwardEmoji.normalize_emoji_name(original_name)
end
end end
- grouped_emojis = awardable.grouped_awards(inline) - grouped_emojis = awardable.grouped_awards(with_thumbs: inline)
.awards.js-awards-block{ class: ("hidden" if !inline && grouped_emojis.size == 0), data: { award_url: url_for([:toggle_award_emoji, @project.namespace.becomes(Namespace), @project, awardable]) } } .awards.js-awards-block{ class: ("hidden" if !inline && grouped_emojis.size == 0), data: { award_url: url_for([:toggle_award_emoji, @project.namespace.becomes(Namespace), @project, awardable]) } }
- awards_sort(grouped_emojis).each do |emoji, awards| - awards_sort(grouped_emojis).each do |emoji, awards|
%button.btn.award-control.js-emoji-btn.has-tooltip{ type: "button", class: (award_active_class(awards, current_user)),data: { placement: "bottom", original_title: award_user_list(awards, current_user)} } %button.btn.award-control.js-emoji-btn.has-tooltip{ type: "button", class: (award_active_class(awards, current_user)),data: { placement: "bottom", title: award_user_list(awards, current_user) } }
= emoji_icon(emoji) = emoji_icon(emoji)
%span.award-control-text.js-counter %span.award-control-text.js-counter
= awards.count = awards.count
......
...@@ -28,14 +28,8 @@ ...@@ -28,14 +28,8 @@
= downvotes = downvotes
- note_count = issue.notes.user.count - note_count = issue.notes.user.count
- if note_count > 0
%li %li
= link_to issue_path(issue) + "#notes" do = link_to issue_path(issue, anchor: 'notes'), class: ('issue-no-comments' if note_count.zero?) do
= icon('comments')
= note_count
- else
%li
= link_to issue_path(issue) + "#notes", class: "issue-no-comments" do
= icon('comments') = icon('comments')
= note_count = note_count
......
...@@ -36,14 +36,8 @@ ...@@ -36,14 +36,8 @@
= downvotes = downvotes
- note_count = merge_request.mr_and_commit_notes.user.count - note_count = merge_request.mr_and_commit_notes.user.count
- if note_count > 0
%li %li
= link_to merge_request_path(merge_request) + "#notes" do = link_to merge_request_path(merge_request, anchor: 'notes'), class: ('merge-request-no-comments' if note_count.zero?) do
= icon('comments')
= note_count
- else
%li
= link_to merge_request_path(merge_request) + "#notes", class: "merge-request-no-comments" do
= icon('comments') = icon('comments')
= note_count = note_count
......
...@@ -3,15 +3,5 @@ class ConvertAwardNoteToEmojiAward < ActiveRecord::Migration ...@@ -3,15 +3,5 @@ class ConvertAwardNoteToEmojiAward < ActiveRecord::Migration
def up def up
execute "INSERT INTO award_emoji (awardable_type, awardable_id, user_id, name, created_at, updated_at) (SELECT noteable_type, noteable_id, author_id, note, created_at, updated_at FROM notes WHERE is_award = true)" execute "INSERT INTO award_emoji (awardable_type, awardable_id, user_id, name, created_at, updated_at) (SELECT noteable_type, noteable_id, author_id, note, created_at, updated_at FROM notes WHERE is_award = true)"
end end
def down
execute <<-SQL
INSERT INTO notes (noteable_type, noteable_id, author_id, note, created_at, updated_at, is_award)
(SELECT awardable_type, awardable_id, user_id, name, created_at, updated_at, TRUE
FROM award_emoji
WHERE awardable_type IN ('Issue', 'MergeRequest')
)
SQL
end
end end
end end
...@@ -174,7 +174,7 @@ module API ...@@ -174,7 +174,7 @@ module API
expose :subscribed do |issue, options| expose :subscribed do |issue, options|
issue.subscribed?(options[:current_user]) issue.subscribed?(options[:current_user])
end end
expose :user_notes_count expose :upvotes, :downvotes
end end
class MergeRequest < ProjectEntity class MergeRequest < ProjectEntity
...@@ -191,7 +191,6 @@ module API ...@@ -191,7 +191,6 @@ module API
expose :subscribed do |merge_request, options| expose :subscribed do |merge_request, options|
merge_request.subscribed?(options[:current_user]) merge_request.subscribed?(options[:current_user])
end end
expose :user_notes_count
end end
class MergeRequestChanges < MergeRequest class MergeRequestChanges < MergeRequest
......
...@@ -257,9 +257,9 @@ describe Projects::IssuesController do ...@@ -257,9 +257,9 @@ describe Projects::IssuesController do
project.team << [user, :developer] project.team << [user, :developer]
end end
it "yields status code 200" do it "toggles the award emoji" do
post(:toggle_award_emoji, namespace_id: project.namespace.path, expect { post(:toggle_award_emoji, namespace_id: project.namespace.path,
project_id: project.path, id: issue.iid, name: "thumbsup") project_id: project.path, id: issue.iid, name: "thumbsup") }.to change { AwardEmoji.count }.by(1)
expect(response.status).to eq(200) expect(response.status).to eq(200)
end end
......
...@@ -28,7 +28,6 @@ describe 'Awards Emoji', feature: true do ...@@ -28,7 +28,6 @@ describe 'Awards Emoji', feature: true do
end end
context 'click the thumbsup emoji' do context 'click the thumbsup emoji' do
it 'should increment the thumbsup emoji', js: true do it 'should increment the thumbsup emoji', js: true do
find('[data-emoji="thumbsup"]').click find('[data-emoji="thumbsup"]').click
sleep 2 sleep 2
...@@ -41,7 +40,6 @@ describe 'Awards Emoji', feature: true do ...@@ -41,7 +40,6 @@ describe 'Awards Emoji', feature: true do
end end
context 'click the thumbsdown emoji' do context 'click the thumbsdown emoji' do
it 'should increment the thumbsdown emoji', js: true do it 'should increment the thumbsdown emoji', js: true do
find('[data-emoji="thumbsdown"]').click find('[data-emoji="thumbsdown"]').click
sleep 2 sleep 2
......
...@@ -203,11 +203,10 @@ describe Issue, "Issuable" do ...@@ -203,11 +203,10 @@ describe Issue, "Issuable" do
end end
end end
# TODO ZJ
describe "votes" do describe "votes" do
before do before do
create!(:award_emoji, :upvote, awardable: issue) create(:award_emoji, :upvote, awardable: issue)
create!(:award_emoji, :downvote, awardable: issue) create(:award_emoji, :downvote, awardable: issue)
end end
it "returns correct values" do it "returns correct values" do
...@@ -215,34 +214,4 @@ describe Issue, "Issuable" do ...@@ -215,34 +214,4 @@ describe Issue, "Issuable" do
expect(issue.downvotes).to eq(1) expect(issue.downvotes).to eq(1)
end end
end end
describe ".with_label" do
let(:project) { create(:project, :public) }
let(:bug) { create(:label, project: project, title: 'bug') }
let(:feature) { create(:label, project: project, title: 'feature') }
let(:enhancement) { create(:label, project: project, title: 'enhancement') }
let(:issue1) { create(:issue, title: "Bugfix1", project: project) }
let(:issue2) { create(:issue, title: "Bugfix2", project: project) }
let(:issue3) { create(:issue, title: "Feature1", project: project) }
before(:each) do
issue1.labels << bug
issue1.labels << feature
issue2.labels << bug
issue2.labels << enhancement
issue3.labels << feature
end
it 'finds the correct issue containing just enhancement label' do
expect(Issue.with_label(enhancement.title)).to match_array([issue2])
end
it 'finds the correct issues containing the same label' do
expect(Issue.with_label(bug.title)).to match_array([issue1, issue2])
end
it 'finds the correct issues containing only both labels' do
expect(Issue.with_label([bug.title, enhancement.title])).to match_array([issue2])
end
end
end end
...@@ -249,7 +249,6 @@ describe API::API, api: true do ...@@ -249,7 +249,6 @@ describe API::API, api: true do
expect(json_response['milestone']).to be_a Hash expect(json_response['milestone']).to be_a Hash
expect(json_response['assignee']).to be_a Hash expect(json_response['assignee']).to be_a Hash
expect(json_response['author']).to be_a Hash expect(json_response['author']).to be_a Hash
expect(json_response['user_notes_count']).to be(1)
end end
it "should return a project issue by id" do it "should return a project issue by id" do
......
...@@ -138,7 +138,6 @@ describe API::API, api: true do ...@@ -138,7 +138,6 @@ describe API::API, api: true do
expect(json_response['work_in_progress']).to be_falsy expect(json_response['work_in_progress']).to be_falsy
expect(json_response['merge_when_build_succeeds']).to be_falsy expect(json_response['merge_when_build_succeeds']).to be_falsy
expect(json_response['merge_status']).to eq('can_be_merged') expect(json_response['merge_status']).to eq('can_be_merged')
expect(json_response['user_notes_count']).to be(2)
end end
it "should return merge_request" do it "should return merge_request" do
......
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