From 4a2d37be6a2082b1fbe48d4898aae7486a76c7f7 Mon Sep 17 00:00:00 2001 From: Sascha Eggenberger Date: Tue, 2 Apr 2024 08:10:25 +0200 Subject: [PATCH 1/4] Merge request list: Improve approvals Improves the display of approvals in the list view. Shows approvals X of Y. Changelog: changed --- .../merge_requests/_approvals_count.html.haml | 10 +++---- .../merge_requests/_approvals_count.html.haml | 28 ++++++------------- locale/gitlab.pot | 10 +++---- 3 files changed, 18 insertions(+), 30 deletions(-) diff --git a/app/views/projects/merge_requests/_approvals_count.html.haml b/app/views/projects/merge_requests/_approvals_count.html.haml index 35dd30c302bab8..7d36abd4f89272 100644 --- a/app/views/projects/merge_requests/_approvals_count.html.haml +++ b/app/views/projects/merge_requests/_approvals_count.html.haml @@ -5,9 +5,9 @@ - if total > 0 - 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 + - approval_tooltip = self_approved ? final_self_text : final_text + - approval_icon = 'check' + - badge_variant = :success + - badge_label = _("Approved") - - approval_icon = sprite_icon((self_approved ? 'approval-solid' : 'approval'), css_class: 'align-middle') - - %li.gl-display-none.gl-sm-display-inline-block.has-tooltip.text-success{ title: self_approved ? final_self_text : final_text } - = approval_icon - = _("Approved") + %li= render Pajamas::BadgeComponent.new(badge_label, size: 'sm', variant: badge_variant, icon: approval_icon, title: approval_tooltip, class: 'has-tooltip') 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 c90f0d3a57271e..c58fab8074e45f 100644 --- a/ee/app/views/projects/merge_requests/_approvals_count.html.haml +++ b/ee/app/views/projects/merge_requests/_approvals_count.html.haml @@ -1,29 +1,17 @@ - if merge_request.approval_needed? - - approved = merge_request.approved? - self_approved = merge_request.approved_by?(current_user) - given = merge_request.approvals_given - total = merge_request.total_approvals_count + - required = merge_request.approvals_required - - 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'), css_class: 'align-middle') - - %li.gl-display-none.gl-sm-display-inline-block.has-tooltip{ title: approval_tooltip, class: ('text-success' if approved) } - = approval_icon + - approved_text = _("Required approvals (%{approvals_given} of %{required} given, you've approved)") % { approvals_given: given, required: required } + - unapproved_text = _("Required approvals (%{approvals_given} of %{required} given)") % { approvals_given: given, required: required } + - approval_tooltip = self_approved ? approved_text : unapproved_text + - approval_icon = approved ? 'check' : 'user' + - badge_variant = approved ? :success : :muted + - badge_label = approved ? _("Approved") : _("%{approvals_given} of %{required} Approvals") % { approvals_given: total, required: required } - - if approved - = _("Approved") - - else - = _("%{remaining_approvals} left") % { remaining_approvals: merge_request.approvals_left } + %li= render Pajamas::BadgeComponent.new(badge_label, size: 'sm', variant: badge_variant, icon: approval_icon, title: approval_tooltip, class: 'has-tooltip') - else = render_ce "projects/merge_requests/approvals_count", merge_request: merge_request diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 6889995bf840f9..f07d2373b0b35f 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -540,6 +540,9 @@ msgstr "" msgid "%{address} is an invalid IP address range" msgstr "" +msgid "%{approvals_given} of %{required} Approvals" +msgstr "" + msgid "%{argument_name} must be formatted correctly. For example: 1h 30m." msgstr "" @@ -1100,9 +1103,6 @@ msgid_plural "%{releases} releases" msgstr[0] "" msgstr[1] "" -msgid "%{remaining_approvals} left" -msgstr "" - msgid "%{requireStart}Require%{requireEnd} %{approvalsRequired} %{approvalStart}approval%{approvalEnd} from:" msgid_plural "%{requireStart}Require%{requireEnd} %{approvalsRequired} %{approvalStart}approvals%{approvalEnd} from:" msgstr[0] "" @@ -42856,10 +42856,10 @@ msgstr "" msgid "Require expiration date" msgstr "" -msgid "Required approvals (%{approvals_given} given)" +msgid "Required approvals (%{approvals_given} of %{required} given)" msgstr "" -msgid "Required approvals (%{approvals_given} given, you've approved)" +msgid "Required approvals (%{approvals_given} of %{required} given, you've approved)" msgstr "" msgid "Required in this project." -- GitLab From 1ea40f4ddb957c13a079b7e0c3eb366ea0e3efaa Mon Sep 17 00:00:00 2001 From: Sascha Eggenberger Date: Tue, 2 Apr 2024 18:09:27 +0200 Subject: [PATCH 2/4] Fix alignment of disabled labels FF --- app/views/projects/merge_requests/_approvals_count.html.haml | 2 +- ee/app/views/projects/merge_requests/_approvals_count.html.haml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/projects/merge_requests/_approvals_count.html.haml b/app/views/projects/merge_requests/_approvals_count.html.haml index 7d36abd4f89272..4487c3397be589 100644 --- a/app/views/projects/merge_requests/_approvals_count.html.haml +++ b/app/views/projects/merge_requests/_approvals_count.html.haml @@ -10,4 +10,4 @@ - badge_variant = :success - badge_label = _("Approved") - %li= render Pajamas::BadgeComponent.new(badge_label, size: 'sm', variant: badge_variant, icon: approval_icon, title: approval_tooltip, class: 'has-tooltip') + %li.gl-display-flex= render Pajamas::BadgeComponent.new(badge_label, size: 'sm', variant: badge_variant, icon: approval_icon, title: approval_tooltip, class: 'has-tooltip') 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 c58fab8074e45f..d793a4eaabcf1f 100644 --- a/ee/app/views/projects/merge_requests/_approvals_count.html.haml +++ b/ee/app/views/projects/merge_requests/_approvals_count.html.haml @@ -12,6 +12,6 @@ - badge_variant = approved ? :success : :muted - badge_label = approved ? _("Approved") : _("%{approvals_given} of %{required} Approvals") % { approvals_given: total, required: required } - %li= render Pajamas::BadgeComponent.new(badge_label, size: 'sm', variant: badge_variant, icon: approval_icon, title: approval_tooltip, class: 'has-tooltip') + %li.gl-display-flex= render Pajamas::BadgeComponent.new(badge_label, size: 'sm', variant: badge_variant, icon: approval_icon, title: approval_tooltip, class: 'has-tooltip') - else = render_ce "projects/merge_requests/approvals_count", merge_request: merge_request -- GitLab From 7f200d6ad83f446a9e04062b862bfb1c9b532ff3 Mon Sep 17 00:00:00 2001 From: Sascha Eggenberger Date: Wed, 3 Apr 2024 09:03:22 +0200 Subject: [PATCH 3/4] Update approval icon --- .../merge_requests/_approvals_count.html.haml | 2 +- .../user_views_all_merge_requests_spec.rb | 35 ++----------------- .../merge_request/user_approves_spec.rb | 2 +- 3 files changed, 5 insertions(+), 34 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 d793a4eaabcf1f..218a6a5b8abf98 100644 --- a/ee/app/views/projects/merge_requests/_approvals_count.html.haml +++ b/ee/app/views/projects/merge_requests/_approvals_count.html.haml @@ -8,7 +8,7 @@ - approved_text = _("Required approvals (%{approvals_given} of %{required} given, you've approved)") % { approvals_given: given, required: required } - unapproved_text = _("Required approvals (%{approvals_given} of %{required} given)") % { approvals_given: given, required: required } - approval_tooltip = self_approved ? approved_text : unapproved_text - - approval_icon = approved ? 'check' : 'user' + - approval_icon = approved ? 'check' : 'approval' - badge_variant = approved ? :success : :muted - badge_label = approved ? _("Approved") : _("%{approvals_given} of %{required} Approvals") % { approvals_given: total, required: required } 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 62a26081637f81..9d8944d159f358 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 @@ -17,49 +17,20 @@ 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 + expect(page.all('li > .gl-badge').any? { |item| item["title"] == "Required approvals (0 of 2 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 + expect(page.all('li > .gl-badge').any? { |item| item["title"] == "Required approvals (1 of 2 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 + expect(page.all('li > .gl-badge').any? { |item| item["title"] == "Required approvals (1 of 2 given, you've approved)" }).to be true end end - - it 'shows custom tooltip after user has approved' do - 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"] == "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 diff --git a/spec/features/merge_request/user_approves_spec.rb b/spec/features/merge_request/user_approves_spec.rb index 5b5ad4468ec454..f05646b6fa69fb 100644 --- a/spec/features/merge_request/user_approves_spec.rb +++ b/spec/features/merge_request/user_approves_spec.rb @@ -27,7 +27,7 @@ def verify_approvals_count_on_index! visit(project_merge_requests_path(project, state: :all)) - expect(page.all('li').any? { |item| item["title"] == "1 approver (you've approved)" }).to be true + expect(page.all('li > .gl-badge').any? { |item| item["title"] == "1 approver (you've approved)" }).to be true visit project_merge_request_path(project, merge_request) end -- GitLab From f722aaf68ae739ed5a58288b86d88a5d8e70b4dd Mon Sep 17 00:00:00 2001 From: Sascha Eggenberger Date: Wed, 3 Apr 2024 09:21:59 +0200 Subject: [PATCH 4/4] Update specs to use testid --- .../projects/merge_requests/_approvals_count.html.haml | 2 +- .../projects/merge_requests/_approvals_count.html.haml | 2 +- .../merge_requests/user_views_all_merge_requests_spec.rb | 6 +++--- spec/features/merge_request/user_approves_spec.rb | 6 +++++- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/app/views/projects/merge_requests/_approvals_count.html.haml b/app/views/projects/merge_requests/_approvals_count.html.haml index 4487c3397be589..e4c226dab509a6 100644 --- a/app/views/projects/merge_requests/_approvals_count.html.haml +++ b/app/views/projects/merge_requests/_approvals_count.html.haml @@ -10,4 +10,4 @@ - badge_variant = :success - badge_label = _("Approved") - %li.gl-display-flex= render Pajamas::BadgeComponent.new(badge_label, size: 'sm', variant: badge_variant, icon: approval_icon, title: approval_tooltip, class: 'has-tooltip') + %li.gl-display-flex= render Pajamas::BadgeComponent.new(badge_label, size: 'sm', variant: badge_variant, icon: approval_icon, title: approval_tooltip, class: 'has-tooltip', data: { 'testid': 'mr-appovals' }) 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 218a6a5b8abf98..72d17d397236ec 100644 --- a/ee/app/views/projects/merge_requests/_approvals_count.html.haml +++ b/ee/app/views/projects/merge_requests/_approvals_count.html.haml @@ -12,6 +12,6 @@ - badge_variant = approved ? :success : :muted - badge_label = approved ? _("Approved") : _("%{approvals_given} of %{required} Approvals") % { approvals_given: total, required: required } - %li.gl-display-flex= render Pajamas::BadgeComponent.new(badge_label, size: 'sm', variant: badge_variant, icon: approval_icon, title: approval_tooltip, class: 'has-tooltip') + %li.gl-display-flex= render Pajamas::BadgeComponent.new(badge_label, size: 'sm', variant: badge_variant, icon: approval_icon, title: approval_tooltip, class: 'has-tooltip', data: { 'testid': 'mr-appovals' }) - else = render_ce "projects/merge_requests/approvals_count", merge_request: merge_request 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 9d8944d159f358..a9b3c94a8b2a4d 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 @@ -17,20 +17,20 @@ it 'shows generic approvals tooltip' do visit(project_merge_requests_path(project, state: :all)) - expect(page.all('li > .gl-badge').any? { |item| item["title"] == "Required approvals (0 of 2 given)" }).to be true + expect(page.all('[data-testid="mr-appovals"]').any? { |item| item["title"] == "Required approvals (0 of 2 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 > .gl-badge').any? { |item| item["title"] == "Required approvals (1 of 2 given)" }).to be true + expect(page.all('[data-testid="mr-appovals"]').any? { |item| item["title"] == "Required approvals (1 of 2 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 > .gl-badge').any? { |item| item["title"] == "Required approvals (1 of 2 given, you've approved)" }).to be true + expect(page.all('[data-testid="mr-appovals"]').any? { |item| item["title"] == "Required approvals (1 of 2 given, you've approved)" }).to be true end end end diff --git a/spec/features/merge_request/user_approves_spec.rb b/spec/features/merge_request/user_approves_spec.rb index f05646b6fa69fb..8b4d1a5adf78b7 100644 --- a/spec/features/merge_request/user_approves_spec.rb +++ b/spec/features/merge_request/user_approves_spec.rb @@ -27,7 +27,11 @@ def verify_approvals_count_on_index! visit(project_merge_requests_path(project, state: :all)) - expect(page.all('li > .gl-badge').any? { |item| item["title"] == "1 approver (you've approved)" }).to be true + expect( + page.all('[data-testid="mr-appovals"]').any? do |item| + item["title"] == "1 approver (you've approved)" + end + ).to be true visit project_merge_request_path(project, merge_request) end -- GitLab