Merge branch 'merge-request-list-improve-approvals' into 'master'

Merge request list: Improve approvals

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/148364



Merged-by: Brandon Labuschagne <blabuschagne@gitlab.com>
Approved-by: Amy Qualls <aqualls@gitlab.com>
Approved-by: Alex Fracazo <afracazo@gitlab.com>
Approved-by: Brandon Labuschagne <blabuschagne@gitlab.com>
Reviewed-by: Angelo Gulina <agulina@gitlab.com>
Reviewed-by: Sascha Eggenberger <seggenberger@gitlab.com>
Co-authored-by: Sascha Eggenberger <seggenberger@gitlab.com>

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.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' })

ee/app/views/projects/merge_requests/_approvals_count.html.haml

 - 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' : 'approval'
+  - 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.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

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('[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').any? { |item| item["title"] == "Required approvals (1 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').any? { |item| item["title"] == "Required approvals (1 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
-
-  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

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] ""
@@ -42952,10 +42952,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."

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').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