Add service classes for mutating AwardEmoji

Adding, destroying and toggling emoji previously lacked services and
instead were performed through methods called on Awardable models.

This led to inconsistencies where relevant todos would be marked as done
only when emoji were awarded through our controllers, but not through
the API. Todos could also be marked as done when an emoji was being
removed.

Behaviour changes

- Awarding emoji through the API will now mark a relevant Todo as done
- Toggling an emoji off (destroying it) through our controllers will no
  longer mark a relevant Todo as done

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/63372

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

app/finders/award_emojis_finder.rb

+# 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

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

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

app/services/award_emojis/add_service.rb

+# 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

app/services/award_emojis/base_service.rb

+# 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

app/services/award_emojis/destroy_service.rb

+# 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

app/services/award_emojis/toggle_service.rb

+# 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

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)

db/fixtures/development/15_award_emoji.rb

 require './spec/support/sidekiq'
 Gitlab::Seeder.quiet do
-  emoji = Gitlab::Emoji.emojis.keys
-  Issue.order(Gitlab::Database.random).limit(Issue.count / 2).each do |issue|
-    project = issue.project
-    project.team.users.sample(2).each do |user|
-      issue.create_award_emoji(emoji.sample, user)
-      issue.notes.sample(2).each do |note|
-        next if note.system?
-        note.create_award_emoji(emoji.sample, user)
+  EMOJI = Gitlab::Emoji.emojis.keys
+  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
+        awardable.notes.user.sample(2).each do |note|
+          AwardEmojis::AddService.new(note, EMOJI.sample, user).execute
+        end
+        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

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)
-            if award.persisted?
-              present award, with: Entities::AwardEmoji
+            service = AwardEmojis::AddService.new(awardable, params[:name], current_user).execute
+            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

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)

spec/finders/award_emojis_finder_spec.rb

+# 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

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

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

spec/services/award_emojis/add_service_spec.rb

+# 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

spec/services/award_emojis/destroy_service_spec.rb

+# 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

spec/services/award_emojis/toggle_service_spec.rb

+# 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

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

spec/support/shared_examples/award_emoji_todo_shared_examples.rb

+# 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