From 97b60a820eea3f74355eead74a87783d9f9c3338 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 2 Jun 2020 08:43:21 -0600 Subject: [PATCH 1/4] Update the MR approvals UI to be more clear --- .../merge_requests/_approvals_count.html.haml | 32 +++++++++++++++---- 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/ee/app/views/projects/merge_requests/_approvals_count.html.haml b/ee/app/views/projects/merge_requests/_approvals_count.html.haml index 32b732a9f4d911..be3e851d1d6ce6 100644 --- a/ee/app/views/projects/merge_requests/_approvals_count.html.haml +++ b/ee/app/views/projects/merge_requests/_approvals_count.html.haml @@ -1,7 +1,27 @@ - if merge_request.approval_needed? - - approval_tooltip = merge_request.has_approved?(current_user) ? _("Approvals (you've approved)") : _("Approvals") - %li.d-none.d-sm-inline-block.has-tooltip{ title: approval_tooltip, class: ('text-success' if merge_request.approved?) } - = sprite_icon((merge_request.has_approved?(current_user) ? 'approval-solid' : 'approval'), size: 16, css_class: 'align-middle') - = merge_request.approvals_given - %span of - = merge_request.approvals_required + + - approved = merge_request.approved? + - self_approved = merge_request.has_approved?(current_user) + - given = merge_request.approvals_given + - total = merge_request.total_approvals_count + + - approved_text = _("Required approvals (%{approvals_given} given, you've approved)") % { approvals_given: given } + - unapproved_text = _("Required approvals (%{approvals_given} given)") % { approvals_given: given } + + - final_text = n_("%d approver", "%d approvers", total) % total + - final_self_text = n_("%d approver (you've approved)", "%d approvers (you've approved)", total) % total + + - if approved + - approval_tooltip = self_approved ? final_self_text : final_text + - else + - approval_tooltip = self_approved ? approved_text : unapproved_text + + - approval_icon = sprite_icon((self_approved ? 'approval-solid' : 'approval'), size: 16, css_class: 'align-middle') + + %li.d-none.d-sm-inline-block.has-tooltip{ title: approval_tooltip, class: ('text-success' if approved) } + = approval_icon + + - if approved + = _("Approved") + - else + = _("%{remaining_approvals} left") % { remaining_approvals: merge_request.approvals_left } -- GitLab From 90e87c2e96bd1e3a1b037b9f097f013ee1bcd9f3 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 2 Jun 2020 09:43:04 -0600 Subject: [PATCH 2/4] Update translations for new MR approvals UI --- locale/gitlab.pot | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index c7bfb3d7366a1f..0d65df72a022e0 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -76,6 +76,16 @@ msgid_plural "%d URLs scanned" msgstr[0] "" msgstr[1] "" +msgid "%d approver" +msgid_plural "%d approvers" +msgstr[0] "" +msgstr[1] "" + +msgid "%d approver (you've approved)" +msgid_plural "%d approvers (you've approved)" +msgstr[0] "" +msgstr[1] "" + msgid "%d changed file" msgid_plural "%d changed files" msgstr[0] "" @@ -506,6 +516,9 @@ msgid_plural "%{releases} releases" msgstr[0] "" msgstr[1] "" +msgid "%{remaining_approvals} left" +msgstr "" + msgid "%{retryButtonStart}Try again%{retryButtonEnd} or %{newFileButtonStart}attach a new file%{newFileButtonEnd}" msgstr "" @@ -2633,12 +2646,6 @@ msgstr "" msgid "ApprovalRule|e.g. QA, Security, etc." msgstr "" -msgid "Approvals" -msgstr "" - -msgid "Approvals (you've approved)" -msgstr "" - msgid "Approve" msgstr "" @@ -2648,6 +2655,9 @@ msgstr "" msgid "Approve the current merge request." msgstr "" +msgid "Approved" +msgstr "" + msgid "Approved by: " msgstr "" @@ -18654,6 +18664,12 @@ msgstr "" msgid "Require users to prove ownership of custom domains" msgstr "" +msgid "Required approvals (%{approvals_given} given)" +msgstr "" + +msgid "Required approvals (%{approvals_given} given, you've approved)" +msgstr "" + msgid "Requirement" msgstr "" -- GitLab From 1ffae13a502872d5c1a4074eca3adaca63c72dec Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 4 Jun 2020 13:06:20 -0600 Subject: [PATCH 3/4] Update tests for new MR approvals UI configurations --- .../user_views_all_merge_requests_spec.rb | 53 +++++++++++++++++-- 1 file changed, 48 insertions(+), 5 deletions(-) diff --git a/ee/spec/features/merge_requests/user_views_all_merge_requests_spec.rb b/ee/spec/features/merge_requests/user_views_all_merge_requests_spec.rb index e23e9096c7546e..f1947b676cd60f 100644 --- a/ee/spec/features/merge_requests/user_views_all_merge_requests_spec.rb +++ b/ee/spec/features/merge_requests/user_views_all_merge_requests_spec.rb @@ -6,17 +6,60 @@ let!(:merge_request) { create(:merge_request, source_project: project, target_project: project) } let(:project) { create(:project, :public, approvals_before_merge: 1) } let(:user) { create(:user) } + let(:another_user) { create(:user) } - it 'shows generic approvals tooltip' do - visit(project_merge_requests_path(project, state: :all)) - expect(page.all('li').any? { |item| item["title"] == "Approvals"}).to be true + before do + project.add_developer(user) + end + + describe 'more approvals are required' do + let!(:approval_rule) { create( :approval_merge_request_rule, merge_request: merge_request, users: [user, another_user], approvals_required: 2, name: "test rule" ) } + + it 'shows generic approvals tooltip' do + visit(project_merge_requests_path(project, state: :all)) + expect(page.all('li').any? { |item| item["title"] == "Required approvals (0 given)"}).to be true + end + + it 'shows custom tooltip after a different user has approved' do + merge_request.approvals.create(user: another_user) + visit(project_merge_requests_path(project, state: :all)) + expect(page.all('li').any? { |item| item["title"] == "Required approvals (1 given)"}).to be true + end + + it 'shows custom tooltip after self has approved' do + merge_request.approvals.create(user: user) + sign_in(user) + visit(project_merge_requests_path(project, state: :all)) + expect(page.all('li').any? { |item| item["title"] == "Required approvals (1 given, you've approved)"}).to be true + end end it 'shows custom tooltip after user has approved' do - project.add_developer(user) sign_in(user) merge_request.approvals.create(user: user) visit(project_merge_requests_path(project, state: :all)) - expect(page.all('li').any? { |item| item["title"] == "Approvals (you've approved)"}).to be true + expect(page.all('li').any? { |item| item["title"] == "1 approver (you've approved)"}).to be true + end + + it 'shows custom tooltip after a different user has approved' do + merge_request.approvals.create(user: another_user) + sign_in(user) + visit(project_merge_requests_path(project, state: :all)) + expect(page.all('li').any? { |item| item["title"] == "1 approver"}).to be true + end + + it 'shows custom tooltip after multiple users have approved' do + merge_request.approvals.create(user: another_user) + merge_request.approvals.create(user: user) + visit(project_merge_requests_path(project, state: :all)) + expect(page.all('li').any? { |item| item["title"] == "2 approvers"}).to be true + end + + it 'shows custom tooltip after multiple users have approved, including self' do + merge_request.approvals.create(user: another_user) + merge_request.approvals.create(user: user) + sign_in(user) + visit(project_merge_requests_path(project, state: :all)) + expect(page.all('li').any? { |item| item["title"] == "2 approvers (you've approved)"}).to be true end end -- GitLab From d093782b16cbeb29d042beb4865c2b72a46e092b Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 1 Jun 2020 15:25:35 -0600 Subject: [PATCH 4/4] Add changelog for improved MR approvals UI --- ee/changelogs/unreleased/approvals-count-ui.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 ee/changelogs/unreleased/approvals-count-ui.yml diff --git a/ee/changelogs/unreleased/approvals-count-ui.yml b/ee/changelogs/unreleased/approvals-count-ui.yml new file mode 100644 index 00000000000000..0e21cc32a030f5 --- /dev/null +++ b/ee/changelogs/unreleased/approvals-count-ui.yml @@ -0,0 +1,5 @@ +--- +title: Improve the clarity of the MR approvals tooltip UI +merge_request: 33329 +author: +type: changed -- GitLab