Commit 6d5b4ce3 authored by Dheeraj Joshi's avatar Dheeraj Joshi Committed by Andy Soiron

Refactor MR Widget related links component

This aims to remove dangerous v-html by moving the
link generation for `assign issue to yourself` from
backend to frontend
parent 2a9efeec
<script> <script>
import { GlSafeHtmlDirective as SafeHtml } from '@gitlab/ui'; import { GlSafeHtmlDirective as SafeHtml, GlLink } from '@gitlab/ui';
import { s__, n__ } from '~/locale'; import { s__, n__ } from '~/locale';
import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
...@@ -8,6 +8,9 @@ export default { ...@@ -8,6 +8,9 @@ export default {
directives: { directives: {
SafeHtml, SafeHtml,
}, },
components: {
GlLink,
},
mixins: [glFeatureFlagMixin()], mixins: [glFeatureFlagMixin()],
props: { props: {
relatedLinks: { relatedLinks: {
...@@ -37,6 +40,17 @@ export default { ...@@ -37,6 +40,17 @@ export default {
return n__('mrWidget|Closes issue', 'mrWidget|Closes issues', this.relatedLinks.closingCount); return n__('mrWidget|Closes issue', 'mrWidget|Closes issues', this.relatedLinks.closingCount);
}, },
assignIssueText() {
if (this.relatedLinks.unassignedCount > 1) {
return s__('mrWidget|Assign yourself to these issues');
}
return s__('mrWidget|Assign yourself to this issue');
},
shouldShowAssignToMeLink() {
return (
this.relatedLinks.unassignedCount && this.relatedLinks.assignToMe && this.showAssignToMe
);
},
}, },
}; };
</script> </script>
...@@ -57,10 +71,14 @@ export default { ...@@ -57,10 +71,14 @@ export default {
<span v-safe-html="relatedLinks.mentioned"></span> <span v-safe-html="relatedLinks.mentioned"></span>
</p> </p>
<p <p
v-if="relatedLinks.assignToMe && showAssignToMe" v-if="shouldShowAssignToMeLink"
:class="{ 'gl-display-line gl-m-0': glFeatures.restructuredMrWidget }" :class="{ 'gl-display-line gl-m-0': glFeatures.restructuredMrWidget }"
> >
<span v-html="relatedLinks.assignToMe /* eslint-disable-line vue/no-v-html */"></span> <span>
<gl-link rel="nofollow" data-method="post" :href="relatedLinks.assignToMe">{{
assignIssueText
}}</gl-link>
</span>
</p> </p>
</section> </section>
</template> </template>
...@@ -82,14 +82,16 @@ export default class MergeRequestStore { ...@@ -82,14 +82,16 @@ export default class MergeRequestStore {
const { closing } = links; const { closing } = links;
const mentioned = links.mentioned_but_not_closing; const mentioned = links.mentioned_but_not_closing;
const assignToMe = links.assign_to_closing; const assignToMe = links.assign_to_closing;
const unassignedCount = links.assign_to_closing_count;
if (closing || mentioned || assignToMe) { if (closing || mentioned || unassignedCount) {
this.relatedLinks = { this.relatedLinks = {
closing, closing,
mentioned, mentioned,
assignToMe, assignToMe,
closingCount: links.closing_count, closingCount: links.closing_count,
mentionedCount: links.mentioned_count, mentionedCount: links.mentioned_count,
unassignedCount: links.assign_to_closing_count,
}; };
} }
} }
......
...@@ -149,7 +149,11 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated ...@@ -149,7 +149,11 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
) )
end end
def assign_to_closing_issues_link def assign_to_closing_issues_path
assign_related_issues_project_merge_request_path(project, merge_request)
end
def assign_to_closing_issues_count
# rubocop: disable CodeReuse/ServiceClass # rubocop: disable CodeReuse/ServiceClass
issues = MergeRequests::AssignIssuesService.new(project: project, issues = MergeRequests::AssignIssuesService.new(project: project,
current_user: current_user, current_user: current_user,
...@@ -157,14 +161,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated ...@@ -157,14 +161,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
merge_request: merge_request, merge_request: merge_request,
closes_issues: closing_issues closes_issues: closing_issues
}).assignable_issues }).assignable_issues
path = assign_related_issues_project_merge_request_path(project, merge_request) issues.count
if issues.present?
if issues.count > 1
link_to _('Assign yourself to these issues'), path, method: :post
else
link_to _('Assign yourself to this issue'), path, method: :post
end
end
# rubocop: enable CodeReuse/ServiceClass # rubocop: enable CodeReuse/ServiceClass
end end
......
...@@ -104,7 +104,11 @@ class MergeRequestWidgetEntity < Grape::Entity ...@@ -104,7 +104,11 @@ class MergeRequestWidgetEntity < Grape::Entity
# include them if they are explicitly requested on first load. # include them if they are explicitly requested on first load.
expose :issues_links, if: -> (_, opts) { opts[:issues_links] } do expose :issues_links, if: -> (_, opts) { opts[:issues_links] } do
expose :assign_to_closing do |merge_request| expose :assign_to_closing do |merge_request|
presenter(merge_request).assign_to_closing_issues_link presenter(merge_request).assign_to_closing_issues_path
end
expose :assign_to_closing_count do |merge_request|
presenter(merge_request).assign_to_closing_issues_count
end end
expose :closing do |merge_request| expose :closing do |merge_request|
......
...@@ -4936,12 +4936,6 @@ msgstr "" ...@@ -4936,12 +4936,6 @@ msgstr ""
msgid "Assign to me" msgid "Assign to me"
msgstr "" msgstr ""
msgid "Assign yourself to these issues"
msgstr ""
msgid "Assign yourself to this issue"
msgstr ""
msgid "Assigned %{assignee_users_sentence}." msgid "Assigned %{assignee_users_sentence}."
msgstr "" msgstr ""
...@@ -43990,6 +43984,12 @@ msgstr "" ...@@ -43990,6 +43984,12 @@ msgstr ""
msgid "mrWidget|Approved by you and others" msgid "mrWidget|Approved by you and others"
msgstr "" msgstr ""
msgid "mrWidget|Assign yourself to these issues"
msgstr ""
msgid "mrWidget|Assign yourself to this issue"
msgstr ""
msgid "mrWidget|Cancel auto-merge" msgid "mrWidget|Cancel auto-merge"
msgstr "" msgstr ""
......
import { GlLink } from '@gitlab/ui';
import { shallowMount } from '@vue/test-utils'; import { shallowMount } from '@vue/test-utils';
import RelatedLinks from '~/vue_merge_request_widget/components/mr_widget_related_links.vue'; import RelatedLinks from '~/vue_merge_request_widget/components/mr_widget_related_links.vue';
...@@ -85,13 +86,29 @@ describe('MRWidgetRelatedLinks', () => { ...@@ -85,13 +86,29 @@ describe('MRWidgetRelatedLinks', () => {
expect(content).toContain('Mentions issues #23 and #42'); expect(content).toContain('Mentions issues #23 and #42');
}); });
it('should have assing issues link', () => { describe('should have correct assign issues link', () => {
createComponent({ it.each([
relatedLinks: { [1, 'Assign yourself to this issue'],
assignToMe: '<a href="#">Assign yourself to these issues</a>', [2, 'Assign yourself to these issues'],
}, ])('when issue count is %s, link displays correct text', (unassignedCount, text) => {
const assignToMe = '/assign';
createComponent({
relatedLinks: { assignToMe, unassignedCount },
});
const glLinkWrapper = wrapper.findComponent(GlLink);
expect(glLinkWrapper.attributes('href')).toBe(assignToMe);
expect(glLinkWrapper.text()).toBe(text);
}); });
expect(wrapper.text().trim()).toContain('Assign yourself to these issues'); it('when no link is present', () => {
createComponent({
relatedLinks: { assignToMe: '#', unassignedCount: 0 },
});
expect(wrapper.findComponent(GlLink).exists()).toBe(false);
});
}); });
}); });
...@@ -162,10 +162,19 @@ RSpec.describe MergeRequestPresenter do ...@@ -162,10 +162,19 @@ RSpec.describe MergeRequestPresenter do
end end
end end
describe '#assign_to_closing_issues_link' do describe '#assign_to_closing_issues_path' do
subject do subject do
described_class.new(resource, current_user: user) described_class.new(resource, current_user: user)
.assign_to_closing_issues_link .assign_to_closing_issues_path
end
it { is_expected.to match("#{project.full_path}/-/merge_requests/#{resource.iid}/assign_related_issues") }
end
describe '#assign_to_closing_issues_count' do
subject do
described_class.new(resource, current_user: user)
.assign_to_closing_issues_count
end end
before do before do
...@@ -178,33 +187,28 @@ RSpec.describe MergeRequestPresenter do ...@@ -178,33 +187,28 @@ RSpec.describe MergeRequestPresenter do
let(:issue) { create(:issue) } let(:issue) { create(:issue) }
let(:assignable_issues) { [issue] } let(:assignable_issues) { [issue] }
it 'returns correct link with correct text' do it 'returns correct count' do
is_expected is_expected
.to match("#{project.full_path}/-/merge_requests/#{resource.iid}/assign_related_issues") .to match(1)
is_expected
.to match("Assign yourself to this issue")
end end
end end
context 'multiple closing issues' do context 'multiple closing issues' do
let(:issues) { create_list(:issue, 2) } let(:issues) { build_list(:issue, 2) }
let(:assignable_issues) { issues } let(:assignable_issues) { issues }
it 'returns correct link with correct text' do it 'returns correct count' do
is_expected
.to match("#{project.full_path}/-/merge_requests/#{resource.iid}/assign_related_issues")
is_expected is_expected
.to match("Assign yourself to these issues") .to match(2)
end end
end end
context 'no closing issue' do context 'no closing issue' do
let(:assignable_issues) { [] } let(:assignable_issues) { [] }
it 'returns correct link with correct text' do it 'returns correct count' do
is_expected.to be_nil is_expected
.to match(0)
end end
end end
end end
......
...@@ -59,7 +59,7 @@ RSpec.describe MergeRequestWidgetEntity do ...@@ -59,7 +59,7 @@ RSpec.describe MergeRequestWidgetEntity do
data = described_class.new(resource, request: request, issues_links: true).as_json data = described_class.new(resource, request: request, issues_links: true).as_json
expect(data).to include(:issues_links) expect(data).to include(:issues_links)
expect(data[:issues_links]).to include(:assign_to_closing, :closing, :mentioned_but_not_closing, :closing_count, :mentioned_count) expect(data[:issues_links]).to include(:assign_to_closing, :assign_to_closing_count, :closing, :mentioned_but_not_closing, :closing_count, :mentioned_count)
end end
it 'omits issue links by default' do it 'omits issue links by default' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment