Commit 1f4d254e authored by Patrick Bajao's avatar Patrick Bajao

Include inapplicable_reason in API response

In order for frontend to display appropriate messages on why
a suggestion can't be applied, it needs to know the reason.

A new `inapplicable_reason` property is introduced and included in
the API response to achieve that. It is based on the checks to
determine whether a suggestion can be applied.

The following reasons are:

- "You don't have write access to the source branch." - when user
  has no write access to branch.
- "This merge request was merged. To apply this suggestion, edit
  this file directly." - when MR was merged.
- "This merge request is closed. To apply this suggestion, edit
  this file directly." - when MR is closed.
- "Can't apply as the source branch was deleted." - when source
  branch is deleted.
- "Can't apply as this line was changed in a more recent version."
  - when the line where suggestion is on is outdated.
- "Can't apply as these lines were changed in a more recent
  version." - when lines where suggestion is on are outdated.
- "This suggestion already matches its content." - when suggestion
  has the same content as the line it is on.
- "Can't apply this suggestion." - default message when suggestion
  cannot be applied but doesn't fall into any of criteria above.
  We won't show this message anywhere right now but it makes sense
  to return something (e.g. when suggestion is already applied).
parent 72002e21
...@@ -38,11 +38,18 @@ class Suggestion < ApplicationRecord ...@@ -38,11 +38,18 @@ class Suggestion < ApplicationRecord
end end
def appliable?(cached: true) def appliable?(cached: true)
!applied? && inapplicable_reason(cached: cached).nil?
noteable.opened? && end
!outdated?(cached: cached) &&
different_content? && def inapplicable_reason(cached: true)
note.active? strong_memoize("inapplicable_reason_#{cached}") do
next :applied if applied?
next :merge_request_merged if noteable.merged?
next :merge_request_closed if noteable.closed?
next :source_branch_deleted unless noteable.source_branch_exists?
next :outdated if outdated?(cached: cached) || !note.active?
next :same_content unless different_content?
end
end end
# Overwrites outdated column # Overwrites outdated column
...@@ -53,6 +60,10 @@ class Suggestion < ApplicationRecord ...@@ -53,6 +60,10 @@ class Suggestion < ApplicationRecord
from_content != fetch_from_content from_content != fetch_from_content
end end
def single_line?
lines_above.zero? && lines_below.zero?
end
def target_line def target_line
position.new_line position.new_line
end end
......
...@@ -2,6 +2,7 @@ ...@@ -2,6 +2,7 @@
class SuggestionEntity < API::Entities::Suggestion class SuggestionEntity < API::Entities::Suggestion
include RequestAwareEntity include RequestAwareEntity
include Gitlab::Utils::StrongMemoize
unexpose :from_line, :to_line, :from_content, :to_content unexpose :from_line, :to_line, :from_content, :to_content
expose :diff_lines, using: DiffLineEntity do |suggestion| expose :diff_lines, using: DiffLineEntity do |suggestion|
...@@ -9,7 +10,29 @@ class SuggestionEntity < API::Entities::Suggestion ...@@ -9,7 +10,29 @@ class SuggestionEntity < API::Entities::Suggestion
end end
expose :current_user do expose :current_user do
expose :can_apply do |suggestion| expose :can_apply do |suggestion|
Ability.allowed?(current_user, :apply_suggestion, suggestion) can_apply?(suggestion)
end
end
expose :inapplicable_reason do |suggestion|
next _("You don't have write access to the source branch.") unless can_apply?(suggestion)
next if suggestion.appliable?
case suggestion.inapplicable_reason
when :merge_request_merged
_("This merge request was merged. To apply this suggestion, edit this file directly.")
when :merge_request_closed
_("This merge request is closed. To apply this suggestion, edit this file directly.")
when :source_branch_deleted
_("Can't apply as the source branch was deleted.")
when :outdated
phrase = suggestion.single_line? ? 'this line was' : 'these lines were'
_("Can't apply as %{phrase} changed in a more recent version.") % { phrase: phrase }
when :same_content
_("This suggestion already matches its content.")
else
_("Can't apply this suggestion.")
end end
end end
...@@ -18,4 +41,10 @@ class SuggestionEntity < API::Entities::Suggestion ...@@ -18,4 +41,10 @@ class SuggestionEntity < API::Entities::Suggestion
def current_user def current_user
request.current_user request.current_user
end end
def can_apply?(suggestion)
strong_memoize(:can_apply) do
Ability.allowed?(current_user, :apply_suggestion, suggestion)
end
end
end end
...@@ -3991,9 +3991,18 @@ msgstr "" ...@@ -3991,9 +3991,18 @@ msgstr ""
msgid "Can override approvers and approvals required per merge request" msgid "Can override approvers and approvals required per merge request"
msgstr "" msgstr ""
msgid "Can't apply as %{phrase} changed in a more recent version."
msgstr ""
msgid "Can't apply as the source branch was deleted."
msgstr ""
msgid "Can't apply as this line has changed or the suggestion already matches its content." msgid "Can't apply as this line has changed or the suggestion already matches its content."
msgstr "" msgstr ""
msgid "Can't apply this suggestion."
msgstr ""
msgid "Can't create snippet: %{err}" msgid "Can't create snippet: %{err}"
msgstr "" msgstr ""
...@@ -23537,9 +23546,15 @@ msgstr "" ...@@ -23537,9 +23546,15 @@ msgstr ""
msgid "This merge request does not have accessibility reports" msgid "This merge request does not have accessibility reports"
msgstr "" msgstr ""
msgid "This merge request is closed. To apply this suggestion, edit this file directly."
msgstr ""
msgid "This merge request is locked." msgid "This merge request is locked."
msgstr "" msgstr ""
msgid "This merge request was merged. To apply this suggestion, edit this file directly."
msgstr ""
msgid "This namespace has already been taken! Please choose another one." msgid "This namespace has already been taken! Please choose another one."
msgstr "" msgstr ""
...@@ -23630,6 +23645,9 @@ msgstr "" ...@@ -23630,6 +23645,9 @@ msgstr ""
msgid "This subscription is for" msgid "This subscription is for"
msgstr "" msgstr ""
msgid "This suggestion already matches its content."
msgstr ""
msgid "This timeout will take precedence when lower than project-defined timeout and accepts a human readable time input language like \"1 hour\". Values without specification represent seconds." msgid "This timeout will take precedence when lower than project-defined timeout and accepts a human readable time input language like \"1 hour\". Values without specification represent seconds."
msgstr "" msgstr ""
...@@ -26274,6 +26292,9 @@ msgstr "" ...@@ -26274,6 +26292,9 @@ msgstr ""
msgid "You don't have sufficient permission to perform this action." msgid "You don't have sufficient permission to perform this action."
msgstr "" msgstr ""
msgid "You don't have write access to the source branch."
msgstr ""
msgid "You don’t have access to Productivity Analytics in this group" msgid "You don’t have access to Productivity Analytics in this group"
msgstr "" msgstr ""
......
...@@ -38,26 +38,106 @@ RSpec.describe Suggestion do ...@@ -38,26 +38,106 @@ RSpec.describe Suggestion do
end end
describe '#appliable?' do describe '#appliable?' do
context 'when patch is already applied' do let(:suggestion) { build(:suggestion) }
let(:suggestion) { create(:suggestion, :applied) }
it 'returns false' do subject(:appliable) { suggestion.appliable? }
expect(suggestion).not_to be_appliable
before do
allow(suggestion).to receive(:inapplicable_reason).and_return(inapplicable_reason)
end
context 'when inapplicable_reason is nil' do
let(:inapplicable_reason) { nil }
it { is_expected.to be_truthy }
end
context 'when inapplicable_reason is not nil' do
let(:inapplicable_reason) { :applied }
it { is_expected.to be_falsey }
end
end
describe '#inapplicable_reason' do
let(:merge_request) { create(:merge_request) }
let!(:note) do
create(
:diff_note_on_merge_request,
project: merge_request.project,
noteable: merge_request
)
end
let(:suggestion) { build(:suggestion, note: note) }
subject(:inapplicable_reason) { suggestion.inapplicable_reason }
context 'when suggestion is already applied' do
let(:suggestion) { build(:suggestion, :applied, note: note) }
it { is_expected.to eq(:applied) }
end
context 'when merge request was merged' do
before do
merge_request.mark_as_merged!
end
it { is_expected.to eq(:merge_request_merged) }
end
context 'when merge request is closed' do
before do
merge_request.close!
end end
it { is_expected.to eq(:merge_request_closed) }
end end
context 'when merge request is not opened' do context 'when source branch is deleted' do
let(:merge_request) { create(:merge_request, :merged) } before do
let(:note) do merge_request.project.repository.rm_branch(merge_request.author, merge_request.source_branch)
create(:diff_note_on_merge_request, project: merge_request.project,
noteable: merge_request)
end end
let(:suggestion) { create(:suggestion, note: note) } it { is_expected.to eq(:source_branch_deleted) }
end
it 'returns false' do context 'when content is outdated' do
expect(suggestion).not_to be_appliable before do
allow(suggestion).to receive(:outdated?).and_return(true)
end
it { is_expected.to eq(:outdated) }
end
context 'when note is outdated' do
before do
allow(note).to receive(:active?).and_return(false)
end end
it { is_expected.to eq(:outdated) }
end
context 'when applicable' do
it { is_expected.to be_nil }
end
end
describe '#single_line?' do
subject(:single_line) { suggestion.single_line? }
context 'when suggestion is for a single line' do
let(:suggestion) { build(:suggestion, lines_above: 0, lines_below: 0) }
it { is_expected.to eq(true) }
end
context 'when suggestion is for multiple lines' do
let(:suggestion) { build(:suggestion, lines_above: 2, lines_below: 0) }
it { is_expected.to eq(false) }
end end
end end
end end
...@@ -13,10 +13,119 @@ RSpec.describe SuggestionEntity do ...@@ -13,10 +13,119 @@ RSpec.describe SuggestionEntity do
subject { entity.as_json } subject { entity.as_json }
it 'exposes correct attributes' do it 'exposes correct attributes' do
expect(subject.keys).to match_array([:id, :appliable, :applied, :diff_lines, :current_user]) expect(subject.keys).to match_array([:id, :appliable, :applied, :diff_lines, :current_user, :inapplicable_reason])
end end
it 'exposes current user abilities' do it 'exposes current user abilities' do
expect(subject[:current_user]).to include(:can_apply) expect(subject[:current_user]).to include(:can_apply)
end end
describe 'inapplicable_reason' do
let(:inapplicable_reason) { subject[:inapplicable_reason] }
before do
allow(Ability).to receive(:allowed?).and_call_original
allow(Ability)
.to receive(:allowed?)
.with(user, :apply_suggestion, suggestion)
.and_return(can_apply_suggestion)
end
context 'when user can apply suggestion' do
let(:can_apply_suggestion) { true }
before do
allow(suggestion).to receive(:appliable?).and_return(appliable)
end
context 'and suggestion is appliable' do
let(:appliable) { true }
it 'returns nil' do
expect(inapplicable_reason).to be_nil
end
end
context 'but suggestion is not applicable' do
let(:appliable) { false }
before do
allow(suggestion).to receive(:inapplicable_reason).and_return(reason)
end
context 'and merge request was merged' do
let(:reason) { :merge_request_merged }
it 'returns appropriate message' do
expect(inapplicable_reason).to eq("This merge request was merged. To apply this suggestion, edit this file directly.")
end
end
context 'and source branch was deleted' do
let(:reason) { :source_branch_deleted }
it 'returns appropriate message' do
expect(inapplicable_reason).to eq("Can't apply as the source branch was deleted.")
end
end
context 'and merge request is closed' do
let(:reason) { :merge_request_closed }
it 'returns appropriate message' do
expect(inapplicable_reason).to eq("This merge request is closed. To apply this suggestion, edit this file directly.")
end
end
context 'and suggestion is outdated' do
let(:reason) { :outdated }
before do
allow(suggestion).to receive(:single_line?).and_return(single_line)
end
context 'and suggestion is for a single line' do
let(:single_line) { true }
it 'returns appropriate message' do
expect(inapplicable_reason).to eq("Can't apply as this line was changed in a more recent version.")
end
end
context 'and suggestion is for multiple lines' do
let(:single_line) { false }
it 'returns appropriate message' do
expect(inapplicable_reason).to eq("Can't apply as these lines were changed in a more recent version.")
end
end
end
context 'and suggestion has the same content' do
let(:reason) { :same_content }
it 'returns appropriate message' do
expect(inapplicable_reason).to eq("This suggestion already matches its content.")
end
end
context 'and suggestion is inapplicable for other reasons' do
let(:reason) { :some_other_reason }
it 'returns default message' do
expect(inapplicable_reason).to eq("Can't apply this suggestion.")
end
end
end
end
context 'when user cannot apply suggestion' do
let(:can_apply_suggestion) { false }
it 'returns appropriate message' do
expect(inapplicable_reason).to eq("You don't have write access to the source branch.")
end
end
end
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