diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb index f111c7ca8cc..30a567c3bef 100644 --- a/app/controllers/autocomplete_controller.rb +++ b/app/controllers/autocomplete_controller.rb @@ -36,7 +36,7 @@ class AutocompleteController < ApplicationController end def award_emojis - render json: AwardedEmojiFinder.new(current_user).execute + render json: AwardEmojis::CollectUserEmojiService.new(current_user).execute end def merge_request_target_branches diff --git a/app/controllers/concerns/toggle_award_emoji.rb b/app/controllers/concerns/toggle_award_emoji.rb index 97b343f8b1a..24d178781d6 100644 --- a/app/controllers/concerns/toggle_award_emoji.rb +++ b/app/controllers/concerns/toggle_award_emoji.rb @@ -7,12 +7,9 @@ module ToggleAwardEmoji authenticate_user! name = params.require(:name) - if awardable.user_can_award?(current_user) - awardable.toggle_award_emoji(name, current_user) - - todoable = to_todoable(awardable) - TodoService.new.new_award_emoji(todoable, current_user) if todoable + service = AwardEmojis::ToggleService.new(awardable, name, current_user).execute + if service[:status] == :success render json: { ok: true } else render json: { ok: false } @@ -21,18 +18,6 @@ module ToggleAwardEmoji private - def to_todoable(awardable) - case awardable - when Note - # we don't create todos for personal snippet comments for now - awardable.for_personal_snippet? ? nil : awardable.noteable - when MergeRequest, Issue - awardable - when Snippet - nil - end - end - def awardable raise NotImplementedError end diff --git a/app/finders/award_emojis_finder.rb b/app/finders/award_emojis_finder.rb new file mode 100644 index 00000000000..7320e035409 --- /dev/null +++ b/app/finders/award_emojis_finder.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +class AwardEmojisFinder + attr_reader :awardable, :params + + def initialize(awardable, params = {}) + @awardable = awardable + @params = params + + validate_params + end + + def execute + awards = awardable.award_emoji + awards = by_name(awards) + awards = by_awarded_by(awards) + awards + end + + private + + def by_name(awards) + return awards unless params[:name] + + awards.named(params[:name]) + end + + def by_awarded_by(awards) + return awards unless params[:awarded_by] + + awards.awarded_by(params[:awarded_by]) + end + + def validate_params + return unless params.present? + + validate_name_param + validate_awarded_by_param + end + + def validate_name_param + return unless params[:name] + + raise ArgumentError, 'Invalid name param' unless params[:name].in?(Gitlab::Emoji.emojis_names) + end + + def validate_awarded_by_param + return unless params[:awarded_by] + + # awarded_by can be a `User`, or an ID + unless params[:awarded_by].is_a?(User) || params[:awarded_by].to_s.match(/\A\d+\Z/) + raise ArgumentError, 'Invalid awarded_by param' + end + end +end diff --git a/app/finders/awarded_emoji_finder.rb b/app/finders/awarded_emoji_finder.rb deleted file mode 100644 index f0cc17f3b26..00000000000 --- a/app/finders/awarded_emoji_finder.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true - -# Class for retrieving information about emoji awarded _by_ a particular user. -class AwardedEmojiFinder - attr_reader :current_user - - # current_user - The User to generate the data for. - def initialize(current_user = nil) - @current_user = current_user - end - - def execute - return [] unless current_user - - # We want the resulting data set to be an Array containing the emoji names - # in descending order, based on how often they were awarded. - AwardEmoji - .award_counts_for_user(current_user) - .map { |name, _| { name: name } } - end -end diff --git a/app/graphql/mutations/award_emojis/add.rb b/app/graphql/mutations/award_emojis/add.rb index 8e050dd6d29..85f3eb065bb 100644 --- a/app/graphql/mutations/award_emojis/add.rb +++ b/app/graphql/mutations/award_emojis/add.rb @@ -10,14 +10,11 @@ module Mutations check_object_is_awardable!(awardable) - # TODO this will be handled by AwardEmoji::AddService - # See https://gitlab.com/gitlab-org/gitlab-ce/issues/63372 and - # https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/29782 - award = awardable.create_award_emoji(args[:name], current_user) + service = ::AwardEmojis::AddService.new(awardable, args[:name], current_user).execute { - award_emoji: (award if award.persisted?), - errors: errors_on_object(award) + award_emoji: (service[:award] if service[:status] == :success), + errors: service[:errors] || [] } end end diff --git a/app/graphql/mutations/award_emojis/remove.rb b/app/graphql/mutations/award_emojis/remove.rb index 3ba85e445b8..f8a3d0ce390 100644 --- a/app/graphql/mutations/award_emojis/remove.rb +++ b/app/graphql/mutations/award_emojis/remove.rb @@ -10,22 +10,11 @@ module Mutations check_object_is_awardable!(awardable) - # TODO this check can be removed once AwardEmoji services are available. - # See https://gitlab.com/gitlab-org/gitlab-ce/issues/63372 and - # https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/29782 - unless awardable.awarded_emoji?(args[:name], current_user) - raise Gitlab::Graphql::Errors::ResourceNotAvailable, - 'You have not awarded emoji of type name to the awardable' - end - - # TODO this will be handled by AwardEmoji::DestroyService - # See https://gitlab.com/gitlab-org/gitlab-ce/issues/63372 and - # https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/29782 - awardable.remove_award_emoji(args[:name], current_user) + service = ::AwardEmojis::DestroyService.new(awardable, args[:name], current_user).execute { # Mutation response is always a `nil` award_emoji - errors: [] + errors: service[:errors] || [] } end end diff --git a/app/graphql/mutations/award_emojis/toggle.rb b/app/graphql/mutations/award_emojis/toggle.rb index c03902e8035..d822048f3a6 100644 --- a/app/graphql/mutations/award_emojis/toggle.rb +++ b/app/graphql/mutations/award_emojis/toggle.rb @@ -15,23 +15,15 @@ module Mutations check_object_is_awardable!(awardable) - # TODO this will be handled by AwardEmoji::ToggleService - # See https://gitlab.com/gitlab-org/gitlab-ce/issues/63372 and - # https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/29782 - award = awardable.toggle_award_emoji(args[:name], current_user) - - # Destroy returns a collection :( - award = award.first if award.is_a?(Array) - - errors = errors_on_object(award) + service = ::AwardEmojis::ToggleService.new(awardable, args[:name], current_user).execute toggled_on = awardable.awarded_emoji?(args[:name], current_user) { # For consistency with the AwardEmojis::Remove mutation, only return # the AwardEmoji if it was created and not destroyed - award_emoji: (award if toggled_on), - errors: errors, + award_emoji: (service[:award] if toggled_on), + errors: service[:errors] || [], toggled_on: toggled_on } end diff --git a/app/models/award_emoji.rb b/app/models/award_emoji.rb index e26162f6151..0ab302a0f3e 100644 --- a/app/models/award_emoji.rb +++ b/app/models/award_emoji.rb @@ -16,8 +16,10 @@ class AwardEmoji < ApplicationRecord participant :user - scope :downvotes, -> { where(name: DOWNVOTE_NAME) } - scope :upvotes, -> { where(name: UPVOTE_NAME) } + scope :downvotes, -> { named(DOWNVOTE_NAME) } + scope :upvotes, -> { named(UPVOTE_NAME) } + scope :named, -> (names) { where(name: names) } + scope :awarded_by, -> (users) { where(user: users) } after_save :expire_etag_cache after_destroy :expire_etag_cache diff --git a/app/models/concerns/awardable.rb b/app/models/concerns/awardable.rb index 14bc56f0eee..f229b42ade6 100644 --- a/app/models/concerns/awardable.rb +++ b/app/models/concerns/awardable.rb @@ -106,30 +106,6 @@ module Awardable end def awarded_emoji?(emoji_name, current_user) - award_emoji.where(name: emoji_name, user: current_user).exists? - end - - def create_award_emoji(name, current_user) - return unless emoji_awardable? - - award_emoji.create(name: normalize_name(name), user: current_user) - end - - def remove_award_emoji(name, current_user) - award_emoji.where(name: name, user: current_user).destroy_all # rubocop: disable DestroyAll - end - - def toggle_award_emoji(emoji_name, current_user) - if awarded_emoji?(emoji_name, current_user) - remove_award_emoji(emoji_name, current_user) - else - create_award_emoji(emoji_name, current_user) - end - end - - private - - def normalize_name(name) - Gitlab::Emoji.normalize_emoji_name(name) + award_emoji.named(emoji_name).awarded_by(current_user).exists? end end diff --git a/app/services/award_emojis/add_service.rb b/app/services/award_emojis/add_service.rb new file mode 100644 index 00000000000..eac15dabbf0 --- /dev/null +++ b/app/services/award_emojis/add_service.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +module AwardEmojis + class AddService < AwardEmojis::BaseService + include Gitlab::Utils::StrongMemoize + + def execute + unless awardable.user_can_award?(current_user) + return error('User cannot award emoji to awardable', status: :forbidden) + end + + unless awardable.emoji_awardable? + return error('Awardable cannot be awarded emoji', status: :unprocessable_entity) + end + + award = awardable.award_emoji.create(name: name, user: current_user) + + if award.persisted? + TodoService.new.new_award_emoji(todoable, current_user) if todoable + success(award: award) + else + error(award.errors.full_messages, award: award) + end + end + + private + + def todoable + strong_memoize(:todoable) do + case awardable + when Note + # We don't create todos for personal snippet comments for now + awardable.noteable unless awardable.for_personal_snippet? + when MergeRequest, Issue + awardable + when Snippet + nil + end + end + end + end +end diff --git a/app/services/award_emojis/base_service.rb b/app/services/award_emojis/base_service.rb new file mode 100644 index 00000000000..a677d03a221 --- /dev/null +++ b/app/services/award_emojis/base_service.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module AwardEmojis + class BaseService < ::BaseService + attr_accessor :awardable, :name + + def initialize(awardable, name, current_user) + @awardable = awardable + @name = normalize_name(name) + + super(awardable.project, current_user) + end + + private + + def normalize_name(name) + Gitlab::Emoji.normalize_emoji_name(name) + end + + # Provide more error state data than what BaseService allows. + # - An array of errors + # - The `AwardEmoji` if present + def error(errors, award: nil, status: nil) + errors = Array.wrap(errors) + + super(errors.to_sentence.presence, status).merge({ + award: award, + errors: errors + }) + end + end +end diff --git a/app/services/award_emojis/collect_user_emoji_service.rb b/app/services/award_emojis/collect_user_emoji_service.rb new file mode 100644 index 00000000000..6cab23f3edf --- /dev/null +++ b/app/services/award_emojis/collect_user_emoji_service.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +# Class for retrieving information about emoji awarded _by_ a particular user. +module AwardEmojis + class CollectUserEmojiService + attr_reader :current_user + + # current_user - The User to generate the data for. + def initialize(current_user = nil) + @current_user = current_user + end + + def execute + return [] unless current_user + + # We want the resulting data set to be an Array containing the emoji names + # in descending order, based on how often they were awarded. + AwardEmoji + .award_counts_for_user(current_user) + .map { |name, _| { name: name } } + end + end +end diff --git a/app/services/award_emojis/destroy_service.rb b/app/services/award_emojis/destroy_service.rb new file mode 100644 index 00000000000..3789a8403bc --- /dev/null +++ b/app/services/award_emojis/destroy_service.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module AwardEmojis + class DestroyService < AwardEmojis::BaseService + def execute + unless awardable.user_can_award?(current_user) + return error('User cannot destroy emoji on the awardable', status: :forbidden) + end + + awards = AwardEmojisFinder.new(awardable, name: name, awarded_by: current_user).execute + + if awards.empty? + return error("User has not awarded emoji of type #{name} on the awardable", status: :forbidden) + end + + award = awards.destroy_all.first # rubocop: disable DestroyAll + + success(award: award) + end + end +end diff --git a/app/services/award_emojis/toggle_service.rb b/app/services/award_emojis/toggle_service.rb new file mode 100644 index 00000000000..812dd1c2889 --- /dev/null +++ b/app/services/award_emojis/toggle_service.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module AwardEmojis + class ToggleService < AwardEmojis::BaseService + def execute + if awardable.awarded_emoji?(name, current_user) + DestroyService.new(awardable, name, current_user).execute + else + AddService.new(awardable, name, current_user).execute + end + end + end +end diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 77c2224ee3b..2ab6e88599f 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -344,10 +344,7 @@ class IssuableBaseService < BaseService def toggle_award(issuable) award = params.delete(:emoji_award) - if award - todo_service.new_award_emoji(issuable, current_user) - issuable.toggle_award_emoji(award, current_user) - end + AwardEmojis::ToggleService.new(issuable, award, current_user).execute if award end def associations_before_update(issuable) diff --git a/db/fixtures/development/15_award_emoji.rb b/db/fixtures/development/15_award_emoji.rb index 137a036edaf..a9dcc048586 100644 --- a/db/fixtures/development/15_award_emoji.rb +++ b/db/fixtures/development/15_award_emoji.rb @@ -1,35 +1,22 @@ require './spec/support/sidekiq' Gitlab::Seeder.quiet do - emoji = Gitlab::Emoji.emojis.keys + EMOJI = Gitlab::Emoji.emojis.keys - Issue.order(Gitlab::Database.random).limit(Issue.count / 2).each do |issue| - project = issue.project + def seed_award_emoji(klass) + klass.order(Gitlab::Database.random).limit(klass.count / 2).each do |awardable| + awardable.project.authorized_users.where('project_authorizations.access_level > ?', Gitlab::Access::GUEST).sample(2).each do |user| + AwardEmojis::AddService.new(awardable, EMOJI.sample, user).execute - project.team.users.sample(2).each do |user| - issue.create_award_emoji(emoji.sample, user) + awardable.notes.user.sample(2).each do |note| + AwardEmojis::AddService.new(note, EMOJI.sample, user).execute + end - issue.notes.sample(2).each do |note| - next if note.system? - note.create_award_emoji(emoji.sample, user) + print '.' end - - print '.' end end - MergeRequest.order(Gitlab::Database.random).limit(MergeRequest.count / 2).each do |mr| - project = mr.project - - project.team.users.sample(2).each do |user| - mr.create_award_emoji(emoji.sample, user) - - mr.notes.sample(2).each do |note| - next if note.system? - note.create_award_emoji(emoji.sample, user) - end - - print '.' - end - end + seed_award_emoji(Issue) + seed_award_emoji(MergeRequest) end diff --git a/lib/api/award_emoji.rb b/lib/api/award_emoji.rb index a1851ba3627..89b7e5c5e4b 100644 --- a/lib/api/award_emoji.rb +++ b/lib/api/award_emoji.rb @@ -69,12 +69,12 @@ module API post endpoint do not_found!('Award Emoji') unless can_read_awardable? && can_award_awardable? - award = awardable.create_award_emoji(params[:name], current_user) + service = AwardEmojis::AddService.new(awardable, params[:name], current_user).execute - if award.persisted? - present award, with: Entities::AwardEmoji + if service[:status] == :success + present service[:award], with: Entities::AwardEmoji else - not_found!("Award Emoji #{award.errors.messages}") + not_found!("Award Emoji #{service[:message]}") end end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index fab47aa4701..187c7864ad7 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -1104,18 +1104,39 @@ describe Projects::IssuesController do project.add_developer(user) end + subject do + post(:toggle_award_emoji, params: { + namespace_id: project.namespace, + project_id: project, + id: issue.iid, + name: emoji_name + }) + end + let(:emoji_name) { 'thumbsup' } + it "toggles the award emoji" do expect do - post(:toggle_award_emoji, params: { - namespace_id: project.namespace, - project_id: project, - id: issue.iid, - name: "thumbsup" - }) + subject end.to change { issue.award_emoji.count }.by(1) expect(response).to have_gitlab_http_status(200) end + + it "removes the already awarded emoji" do + create(:award_emoji, awardable: issue, name: emoji_name, user: user) + + expect { subject }.to change { AwardEmoji.count }.by(-1) + + expect(response).to have_gitlab_http_status(200) + end + + it 'marks Todos on the Issue as done' do + todo = create(:todo, target: issue, project: project, user: user) + + subject + + expect(todo.reload).to be_done + end end describe 'POST create_merge_request' do diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index 9ab565dc2e8..4500c412521 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -543,23 +543,32 @@ describe Projects::NotesController do project.add_developer(user) end + subject { post(:toggle_award_emoji, params: request_params.merge(name: emoji_name)) } + let(:emoji_name) { 'thumbsup' } + it "toggles the award emoji" do expect do - post(:toggle_award_emoji, params: request_params.merge(name: "thumbsup")) + subject end.to change { note.award_emoji.count }.by(1) expect(response).to have_gitlab_http_status(200) end it "removes the already awarded emoji" do - post(:toggle_award_emoji, params: request_params.merge(name: "thumbsup")) + create(:award_emoji, awardable: note, name: emoji_name, user: user) - expect do - post(:toggle_award_emoji, params: request_params.merge(name: "thumbsup")) - end.to change { AwardEmoji.count }.by(-1) + expect { subject }.to change { AwardEmoji.count }.by(-1) expect(response).to have_gitlab_http_status(200) end + + it 'marks Todos on the Noteable as done' do + todo = create(:todo, target: note.noteable, project: project, user: user) + + subject + + expect(todo.reload).to be_done + end end describe "resolving and unresolving" do diff --git a/spec/controllers/snippets/notes_controller_spec.rb b/spec/controllers/snippets/notes_controller_spec.rb index 652533ac49f..fd4b95ce226 100644 --- a/spec/controllers/snippets/notes_controller_spec.rb +++ b/spec/controllers/snippets/notes_controller_spec.rb @@ -288,11 +288,13 @@ describe Snippets::NotesController do describe 'POST toggle_award_emoji' do let(:note) { create(:note_on_personal_snippet, noteable: public_snippet) } + let(:emoji_name) { 'thumbsup'} + before do sign_in(user) end - subject { post(:toggle_award_emoji, params: { snippet_id: public_snippet, id: note.id, name: "thumbsup" }) } + subject { post(:toggle_award_emoji, params: { snippet_id: public_snippet, id: note.id, name: emoji_name }) } it "toggles the award emoji" do expect { subject }.to change { note.award_emoji.count }.by(1) @@ -301,7 +303,7 @@ describe Snippets::NotesController do end it "removes the already awarded emoji when it exists" do - note.toggle_award_emoji('thumbsup', user) # create award emoji before + create(:award_emoji, awardable: note, name: emoji_name, user: user) expect { subject }.to change { AwardEmoji.count }.by(-1) diff --git a/spec/finders/award_emojis_finder_spec.rb b/spec/finders/award_emojis_finder_spec.rb new file mode 100644 index 00000000000..ccac475daad --- /dev/null +++ b/spec/finders/award_emojis_finder_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe AwardEmojisFinder do + set(:issue_1) { create(:issue) } + set(:issue_1_thumbsup) { create(:award_emoji, name: 'thumbsup', awardable: issue_1) } + set(:issue_1_thumbsdown) { create(:award_emoji, name: 'thumbsdown', awardable: issue_1) } + # Create a matching set of emoji for a second issue. + # These should never appear in our finder results + set(:issue_2) { create(:issue) } + set(:issue_2_thumbsup) { create(:award_emoji, name: 'thumbsup', awardable: issue_2) } + set(:issue_2_thumbsdown) { create(:award_emoji, name: 'thumbsdown', awardable: issue_2) } + + describe 'param validation' do + it 'raises an error if `name` is invalid' do + expect { described_class.new(issue_1, { name: 'invalid' }).execute }.to raise_error( + ArgumentError, + 'Invalid name param' + ) + end + + it 'raises an error if `awarded_by` is invalid' do + expectation = [ArgumentError, 'Invalid awarded_by param'] + + expect { described_class.new(issue_1, { awarded_by: issue_2 }).execute }.to raise_error(*expectation) + expect { described_class.new(issue_1, { awarded_by: 'not-an-id' }).execute }.to raise_error(*expectation) + expect { described_class.new(issue_1, { awarded_by: 1.123 }).execute }.to raise_error(*expectation) + end + end + + describe '#execute' do + it 'scopes to the awardable' do + expect(described_class.new(issue_1).execute).to contain_exactly( + issue_1_thumbsup, issue_1_thumbsdown + ) + end + + it 'filters by emoji name' do + expect(described_class.new(issue_1, { name: 'thumbsup' }).execute).to contain_exactly(issue_1_thumbsup) + expect(described_class.new(issue_1, { name: '8ball' }).execute).to be_empty + end + + it 'filters by user' do + expect(described_class.new(issue_1, { awarded_by: issue_1_thumbsup.user }).execute).to contain_exactly(issue_1_thumbsup) + expect(described_class.new(issue_1, { awarded_by: issue_2_thumbsup.user }).execute).to be_empty + end + end +end diff --git a/spec/models/award_emoji_spec.rb b/spec/models/award_emoji_spec.rb index 8452ac69734..b15b26b1630 100644 --- a/spec/models/award_emoji_spec.rb +++ b/spec/models/award_emoji_spec.rb @@ -44,6 +44,29 @@ describe AwardEmoji do end end + describe 'scopes' do + set(:thumbsup) { create(:award_emoji, name: 'thumbsup') } + set(:thumbsdown) { create(:award_emoji, name: 'thumbsdown') } + + describe '.upvotes' do + it { expect(described_class.upvotes).to contain_exactly(thumbsup) } + end + + describe '.downvotes' do + it { expect(described_class.downvotes).to contain_exactly(thumbsdown) } + end + + describe '.named' do + it { expect(described_class.named('thumbsup')).to contain_exactly(thumbsup) } + it { expect(described_class.named(%w[thumbsup thumbsdown])).to contain_exactly(thumbsup, thumbsdown) } + end + + describe '.awarded_by' do + it { expect(described_class.awarded_by(thumbsup.user)).to contain_exactly(thumbsup) } + it { expect(described_class.awarded_by([thumbsup.user, thumbsdown.user])).to contain_exactly(thumbsup, thumbsdown) } + end + end + describe 'expiring ETag cache' do context 'on a note' do let(:note) { create(:note_on_issue) } diff --git a/spec/models/concerns/awardable_spec.rb b/spec/models/concerns/awardable_spec.rb index 9e7106281ee..76da42cf243 100644 --- a/spec/models/concerns/awardable_spec.rb +++ b/spec/models/concerns/awardable_spec.rb @@ -82,16 +82,6 @@ describe Awardable do end end - describe "#toggle_award_emoji" do - it "adds an emoji if it isn't awarded yet" do - expect { issue.toggle_award_emoji("thumbsup", award_emoji.user) }.to change { AwardEmoji.count }.by(1) - end - - it "toggles already awarded emoji" do - expect { issue.toggle_award_emoji("thumbsdown", award_emoji.user) }.to change { AwardEmoji.count }.by(-1) - end - end - describe 'querying award_emoji on an Awardable' do let(:issue) { create(:issue) } diff --git a/spec/requests/api/award_emoji_spec.rb b/spec/requests/api/award_emoji_spec.rb index 6c67d84b59b..342fcfa1041 100644 --- a/spec/requests/api/award_emoji_spec.rb +++ b/spec/requests/api/award_emoji_spec.rb @@ -155,6 +155,14 @@ describe API::AwardEmoji do expect(json_response['user']['username']).to eq(user.username) end + it 'marks Todos on the Issue as done' do + todo = create(:todo, target: issue, project: project, user: user) + + post api("/projects/#{project.id}/issues/#{issue.iid}/award_emoji", user), params: { name: '8ball' } + + expect(todo.reload).to be_done + end + it "returns a 400 bad request error if the name is not given" do post api("/projects/#{project.id}/issues/#{issue.iid}/award_emoji", user) @@ -209,6 +217,14 @@ describe API::AwardEmoji do expect(json_response['user']['username']).to eq(user.username) end + it 'marks Todos on the Noteable as done' do + todo = create(:todo, target: note2.noteable, project: project, user: user) + + post api("/projects/#{project.id}/issues/#{issue.iid}/notes/#{note.id}/award_emoji", user), params: { name: 'rocket' } + + expect(todo.reload).to be_done + end + it "normalizes +1 as thumbsup award" do post api("/projects/#{project.id}/issues/#{issue.iid}/notes/#{note.id}/award_emoji", user), params: { name: '+1' } diff --git a/spec/requests/api/graphql/mutations/award_emojis/add_spec.rb b/spec/requests/api/graphql/mutations/award_emojis/add_spec.rb index 3982125a38a..5b910d5bfe0 100644 --- a/spec/requests/api/graphql/mutations/award_emojis/add_spec.rb +++ b/spec/requests/api/graphql/mutations/award_emojis/add_spec.rb @@ -5,9 +5,9 @@ require 'spec_helper' describe 'Adding an AwardEmoji' do include GraphqlHelpers - let(:current_user) { create(:user) } - let(:awardable) { create(:note) } - let(:project) { awardable.project } + set(:current_user) { create(:user) } + set(:project) { create(:project) } + set(:awardable) { create(:note, project: project) } let(:emoji_name) { 'thumbsup' } let(:mutation) do variables = { @@ -43,7 +43,7 @@ describe 'Adding an AwardEmoji' do end context 'when the given awardable is not an Awardable' do - let(:awardable) { create(:label) } + let(:awardable) { create(:label, project: project) } it_behaves_like 'a mutation that does not create an AwardEmoji' @@ -52,7 +52,7 @@ describe 'Adding an AwardEmoji' do end context 'when the given awardable is an Awardable but still cannot be awarded an emoji' do - let(:awardable) { create(:system_note) } + let(:awardable) { create(:system_note, project: project) } it_behaves_like 'a mutation that does not create an AwardEmoji' @@ -73,6 +73,13 @@ describe 'Adding an AwardEmoji' do expect(mutation_response['awardEmoji']['name']).to eq(emoji_name) end + describe 'marking Todos as done' do + let(:user) { current_user} + subject { post_graphql_mutation(mutation, current_user: user) } + + include_examples 'creating award emojis marks Todos as done' + end + context 'when there were active record validation errors' do before do expect_next_instance_of(AwardEmoji) do |award| diff --git a/spec/requests/api/graphql/mutations/award_emojis/toggle_spec.rb b/spec/requests/api/graphql/mutations/award_emojis/toggle_spec.rb index 31145730f10..ae628d3e56c 100644 --- a/spec/requests/api/graphql/mutations/award_emojis/toggle_spec.rb +++ b/spec/requests/api/graphql/mutations/award_emojis/toggle_spec.rb @@ -5,9 +5,9 @@ require 'spec_helper' describe 'Toggling an AwardEmoji' do include GraphqlHelpers - let(:current_user) { create(:user) } - let(:awardable) { create(:note) } - let(:project) { awardable.project } + set(:current_user) { create(:user) } + set(:project) { create(:project) } + set(:awardable) { create(:note, project: project) } let(:emoji_name) { 'thumbsup' } let(:mutation) do variables = { @@ -40,7 +40,7 @@ describe 'Toggling an AwardEmoji' do end context 'when the given awardable is not an Awardable' do - let(:awardable) { create(:label) } + let(:awardable) { create(:label, project: project) } it_behaves_like 'a mutation that does not create or destroy an AwardEmoji' @@ -49,7 +49,7 @@ describe 'Toggling an AwardEmoji' do end context 'when the given awardable is an Awardable but still cannot be awarded an emoji' do - let(:awardable) { create(:system_note) } + let(:awardable) { create(:system_note, project: project) } it_behaves_like 'a mutation that does not create or destroy an AwardEmoji' @@ -81,6 +81,13 @@ describe 'Toggling an AwardEmoji' do expect(mutation_response['toggledOn']).to eq(true) end + describe 'marking Todos as done' do + let(:user) { current_user} + subject { post_graphql_mutation(mutation, current_user: user) } + + include_examples 'creating award emojis marks Todos as done' + end + context 'when there were active record validation errors' do before do expect_next_instance_of(AwardEmoji) do |award| diff --git a/spec/services/award_emojis/add_service_spec.rb b/spec/services/award_emojis/add_service_spec.rb new file mode 100644 index 00000000000..037db39ba80 --- /dev/null +++ b/spec/services/award_emojis/add_service_spec.rb @@ -0,0 +1,103 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe AwardEmojis::AddService do + set(:user) { create(:user) } + set(:project) { create(:project) } + set(:awardable) { create(:note, project: project) } + let(:name) { 'thumbsup' } + subject(:service) { described_class.new(awardable, name, user) } + + describe '#execute' do + context 'when user is not authorized' do + it 'does not add an emoji' do + expect { service.execute }.not_to change { AwardEmoji.count } + end + + it 'returns an error state' do + result = service.execute + + expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(:forbidden) + end + end + + context 'when user is authorized' do + before do + project.add_developer(user) + end + + it 'creates an award emoji' do + expect { service.execute }.to change { AwardEmoji.count }.by(1) + end + + it 'returns the award emoji' do + result = service.execute + + expect(result[:award]).to be_kind_of(AwardEmoji) + end + + it 'return a success status' do + result = service.execute + + expect(result[:status]).to eq(:success) + end + + it 'sets the correct properties on the award emoji' do + award = service.execute[:award] + + expect(award.name).to eq(name) + expect(award.user).to eq(user) + end + + describe 'marking Todos as done' do + subject { service.execute } + + include_examples 'creating award emojis marks Todos as done' + end + + context 'when the awardable cannot have emoji awarded to it' do + before do + expect(awardable).to receive(:emoji_awardable?).and_return(false) + end + + it 'does not add an emoji' do + expect { service.execute }.not_to change { AwardEmoji.count } + end + + it 'returns an error status' do + result = service.execute + + expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(:unprocessable_entity) + end + end + + context 'when the awardable is invalid' do + before do + expect_next_instance_of(AwardEmoji) do |award| + expect(award).to receive(:valid?).and_return(false) + expect(award).to receive_message_chain(:errors, :full_messages).and_return(['Error 1', 'Error 2']) + end + end + + it 'does not add an emoji' do + expect { service.execute }.not_to change { AwardEmoji.count } + end + + it 'returns an error status' do + result = service.execute + + expect(result[:status]).to eq(:error) + end + + it 'returns an error message' do + result = service.execute + + expect(result[:message]).to eq('Error 1 and Error 2') + end + end + end + end +end diff --git a/spec/finders/awarded_emoji_finder_spec.rb b/spec/services/award_emojis/collect_user_emoji_service_spec.rb similarity index 92% rename from spec/finders/awarded_emoji_finder_spec.rb rename to spec/services/award_emojis/collect_user_emoji_service_spec.rb index d4479df7418..a0dea31b403 100644 --- a/spec/finders/awarded_emoji_finder_spec.rb +++ b/spec/services/award_emojis/collect_user_emoji_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe AwardedEmojiFinder do +describe AwardEmojis::CollectUserEmojiService do describe '#execute' do it 'returns an Array containing the awarded emoji names' do user = create(:user) diff --git a/spec/services/award_emojis/destroy_service_spec.rb b/spec/services/award_emojis/destroy_service_spec.rb new file mode 100644 index 00000000000..c4a7d5ec20e --- /dev/null +++ b/spec/services/award_emojis/destroy_service_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe AwardEmojis::DestroyService do + set(:user) { create(:user) } + set(:awardable) { create(:note) } + set(:project) { awardable.project } + let(:name) { 'thumbsup' } + let!(:award_from_other_user) do + create(:award_emoji, name: name, awardable: awardable, user: create(:user)) + end + subject(:service) { described_class.new(awardable, name, user) } + + describe '#execute' do + shared_examples_for 'a service that does not authorize the user' do |error:| + it 'does not remove the emoji' do + expect { service.execute }.not_to change { AwardEmoji.count } + end + + it 'returns an error state' do + result = service.execute + + expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(:forbidden) + end + + it 'returns a nil award' do + result = service.execute + + expect(result).to have_key(:award) + expect(result[:award]).to be_nil + end + + it 'returns the error' do + result = service.execute + + expect(result[:message]).to eq(error) + expect(result[:errors]).to eq([error]) + end + end + + context 'when user is not authorized' do + it_behaves_like 'a service that does not authorize the user', + error: 'User cannot destroy emoji on the awardable' + end + + context 'when the user is authorized' do + before do + project.add_developer(user) + end + + context 'when user has not awarded an emoji to the awardable' do + let!(:award_from_user) { create(:award_emoji, name: name, user: user) } + + it_behaves_like 'a service that does not authorize the user', + error: 'User has not awarded emoji of type thumbsup on the awardable' + end + + context 'when user has awarded an emoji to the awardable' do + let!(:award_from_user) { create(:award_emoji, name: name, awardable: awardable, user: user) } + + it 'removes the emoji' do + expect { service.execute }.to change { AwardEmoji.count }.by(-1) + end + + it 'returns a success status' do + result = service.execute + + expect(result[:status]).to eq(:success) + end + + it 'returns no errors' do + result = service.execute + + expect(result).not_to have_key(:error) + expect(result).not_to have_key(:errors) + end + + it 'returns the destroyed award' do + result = service.execute + + expect(result[:award]).to eq(award_from_user) + expect(result[:award]).to be_destroyed + end + end + end + end +end diff --git a/spec/services/award_emojis/toggle_service_spec.rb b/spec/services/award_emojis/toggle_service_spec.rb new file mode 100644 index 00000000000..972a1d5fc06 --- /dev/null +++ b/spec/services/award_emojis/toggle_service_spec.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe AwardEmojis::ToggleService do + set(:user) { create(:user) } + set(:project) { create(:project, :public) } + set(:awardable) { create(:note, project: project) } + let(:name) { 'thumbsup' } + subject(:service) { described_class.new(awardable, name, user) } + + describe '#execute' do + context 'when user has awarded an emoji' do + let!(:award_from_other_user) { create(:award_emoji, name: name, awardable: awardable, user: create(:user)) } + let!(:award) { create(:award_emoji, name: name, awardable: awardable, user: user) } + + it 'calls AwardEmojis::DestroyService' do + expect(AwardEmojis::AddService).not_to receive(:new) + + expect_next_instance_of(AwardEmojis::DestroyService) do |service| + expect(service).to receive(:execute) + end + + service.execute + end + + it 'destroys an AwardEmoji' do + expect { service.execute }.to change { AwardEmoji.count }.by(-1) + end + + it 'returns the result of DestroyService#execute' do + mock_result = double(foo: true) + + expect_next_instance_of(AwardEmojis::DestroyService) do |service| + expect(service).to receive(:execute).and_return(mock_result) + end + + result = service.execute + + expect(result).to eq(mock_result) + end + end + + context 'when user has not awarded an emoji' do + it 'calls AwardEmojis::AddService' do + expect_next_instance_of(AwardEmojis::AddService) do |service| + expect(service).to receive(:execute) + end + + expect(AwardEmojis::DestroyService).not_to receive(:new) + + service.execute + end + + it 'creates an AwardEmoji' do + expect { service.execute }.to change { AwardEmoji.count }.by(1) + end + + it 'returns the result of AddService#execute' do + mock_result = double(foo: true) + + expect_next_instance_of(AwardEmojis::AddService) do |service| + expect(service).to receive(:execute).and_return(mock_result) + end + + result = service.execute + + expect(result).to eq(mock_result) + end + end + end +end diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index b0b74407812..d4fa62fa85d 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -78,6 +78,7 @@ describe Projects::CreateService, '#execute' do expect(project).to be_valid expect(project.owner).to eq(group) expect(project.namespace).to eq(group) + expect(project.team.owners).to include(user) expect(user.authorized_projects).to include(project) end end diff --git a/spec/support/shared_examples/award_emoji_todo_shared_examples.rb b/spec/support/shared_examples/award_emoji_todo_shared_examples.rb new file mode 100644 index 00000000000..88ad37d232f --- /dev/null +++ b/spec/support/shared_examples/award_emoji_todo_shared_examples.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +# Shared examples to that test code that creates AwardEmoji also mark Todos +# as done. +# +# The examples expect these to be defined in the calling spec: +# - `subject` the callable code that executes the creation of an AwardEmoji +# - `user` +# - `project` +RSpec.shared_examples 'creating award emojis marks Todos as done' do + using RSpec::Parameterized::TableSyntax + + before do + project.add_developer(user) + end + + where(:type, :expectation) do + :issue | true + :merge_request | true + :project_snippet | false + end + + with_them do + let(:project) { awardable.project } + let(:awardable) { create(type) } + let!(:todo) { create(:todo, target: awardable, project: project, user: user) } + + it do + subject + + expect(todo.reload.done?).to eq(expectation) + end + end + + # Notes have more complicated rules than other Todoables + describe 'for notes' do + let!(:todo) { create(:todo, target: awardable.noteable, project: project, user: user) } + + context 'regular Notes' do + let(:awardable) { create(:note, project: project) } + + it 'marks the Todo as done' do + subject + + expect(todo.reload.done?).to eq(true) + end + end + + context 'PersonalSnippet Notes' do + let(:awardable) { create(:note, noteable: create(:personal_snippet, author: user)) } + + it 'does not mark the Todo as done' do + subject + + expect(todo.reload.done?).to eq(false) + end + end + end +end