This instance will be upgraded to Heptapod 0.31.0 (final) on 2022-05-24 at 14:00 UTC+2 (a few minutes of down time)

Commit 8286c434 authored by Luke Duncalfe's avatar Luke Duncalfe
Browse files

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
parent 4a27be1850c0
......@@ -7,12 +7,9 @@ def toggle_award_emoji
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 @@ def toggle_award_emoji
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
......
# 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
......@@ -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
......
......@@ -106,30 +106,6 @@ def user_authored?(current_user)
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
# 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
# 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
# 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
# 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
......@@ -344,10 +344,7 @@ def change_todo(issuable)
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)
......
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
......@@ -69,12 +69,12 @@ class AwardEmoji < Grape::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
......
......@@ -288,11 +288,13 @@
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 @@
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)
......
# 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
......@@ -44,6 +44,29 @@
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) }
......
......@@ -82,16 +82,6 @@
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) }
......
# 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
# 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]) <