From 28de806cc43615d16cda9bcb280b3acc4a92e9c6 Mon Sep 17 00:00:00 2001 From: Gerardo Date: Fri, 29 Dec 2023 12:46:44 +0100 Subject: [PATCH 1/3] feature: Add keyset pagination to the list of package protection rules This MR includes: - Adding (keyset) pagination for the list of package protection rules - Integrating keyset pagination logic with GraphQL API - Add tests for pagination Changelog: added --- .../components/packages_protection_rules.vue | 73 +++++++--- ...et_packages_protection_rules.query.graphql | 15 +- .../packages_protection_rules_spec.js | 133 +++++++++++++++++- .../settings/project/settings/mock_data.js | 28 ++-- 4 files changed, 206 insertions(+), 43 deletions(-) diff --git a/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue b/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue index d7219e3a91c349..7d23f1e1c4bbc3 100644 --- a/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue +++ b/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue @@ -1,5 +1,5 @@ @@ -135,7 +154,7 @@ export default { :fields="$options.fields" show-empty stacked="md" - class="mb-3" + class="gl-mb-3" :aria-label="$options.i18n.settingBlockTitle" :busy="isLoadingPackageProtectionRules" > @@ -143,6 +162,16 @@ export default { + +
+ +
diff --git a/app/assets/javascripts/packages_and_registries/settings/project/graphql/queries/get_packages_protection_rules.query.graphql b/app/assets/javascripts/packages_and_registries/settings/project/graphql/queries/get_packages_protection_rules.query.graphql index e0a072b93e4c0f..c90c00b4d1a328 100644 --- a/app/assets/javascripts/packages_and_registries/settings/project/graphql/queries/get_packages_protection_rules.query.graphql +++ b/app/assets/javascripts/packages_and_registries/settings/project/graphql/queries/get_packages_protection_rules.query.graphql @@ -1,13 +1,24 @@ -query getProjectPackageProtectionRules($projectPath: ID!, $first: Int) { +#import "~/graphql_shared/fragments/page_info.fragment.graphql" + +query getProjectPackageProtectionRules( + $projectPath: ID! + $first: Int + $last: Int + $after: String + $before: String +) { project(fullPath: $projectPath) { id - packagesProtectionRules(first: $first) { + packagesProtectionRules(first: $first, last: $last, after: $after, before: $before) { nodes { id packageNamePattern packageType pushProtectedUpToAccessLevel } + pageInfo { + ...PageInfo + } } } } diff --git a/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rules_spec.js b/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rules_spec.js index 27b7ec44db3220..0c2995adeae6d1 100644 --- a/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rules_spec.js +++ b/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rules_spec.js @@ -1,4 +1,4 @@ -import { GlLoadingIcon } from '@gitlab/ui'; +import { GlLoadingIcon, GlKeysetPagination } from '@gitlab/ui'; import { shallowMount } from '@vue/test-utils'; import Vue from 'vue'; import VueApollo from 'vue-apollo'; @@ -60,7 +60,7 @@ describe('Packages protection rules project settings', () => { expect(findTable().exists()).toBe(true); }); - describe('table package protection rules', () => { + describe('table "package protection rules"', () => { it('renders table with packages protection rules', async () => { createComponent({ mountFn: mountExtended }); @@ -68,11 +68,13 @@ describe('Packages protection rules project settings', () => { expect(findTable().exists()).toBe(true); - packagesProtectionRulesData.forEach((protectionRule, i) => { - expect(findTableRow(i).text()).toContain(protectionRule.packageNamePattern); - expect(findTableRow(i).text()).toContain(protectionRule.packageType); - expect(findTableRow(i).text()).toContain(protectionRule.pushProtectedUpToAccessLevel); - }); + packagesProtectionRuleQueryPayload().data.project.packagesProtectionRules.nodes.forEach( + (protectionRule, i) => { + expect(findTableRow(i).text()).toContain(protectionRule.packageNamePattern); + expect(findTableRow(i).text()).toContain(protectionRule.packageType); + expect(findTableRow(i).text()).toContain(protectionRule.pushProtectedUpToAccessLevel); + }, + ); }); it('displays table in busy state and shows loading icon inside table', async () => { @@ -88,6 +90,123 @@ describe('Packages protection rules project settings', () => { expect(findTableLoadingIcon().exists()).toBe(false); expect(findTable().attributes('aria-busy')).toBe('false'); }); + + it('calls graphql api query', () => { + const resolver = jest.fn().mockResolvedValue(packagesProtectionRuleQueryPayload()); + createComponent({ resolver }); + + expect(resolver).toHaveBeenCalledWith( + expect.objectContaining({ projectPath: defaultProvidedValues.projectPath }), + ); + }); + + describe('table pagination', () => { + const findPagination = () => wrapper.findComponent(GlKeysetPagination); + + it('renders pagination', async () => { + createComponent({ mountFn: mountExtended }); + + await waitForPromises(); + + expect(findPagination().exists()).toBe(true); + expect(findPagination().props()).toMatchObject({ + endCursor: '10', + startCursor: '0', + hasNextPage: true, + hasPreviousPage: false, + }); + }); + + it('calls initial graphql api query with pagination information', () => { + const resolver = jest.fn().mockResolvedValue(packagesProtectionRuleQueryPayload()); + createComponent({ resolver }); + + expect(resolver).toHaveBeenCalledWith( + expect.objectContaining({ + projectPath: defaultProvidedValues.projectPath, + first: 10, + }), + ); + }); + + describe('when button "prev" is clicked', () => { + const resolver = jest + .fn() + .mockResolvedValueOnce( + packagesProtectionRuleQueryPayload({ + nodes: packagesProtectionRulesData.slice(10), + pageInfo: { + hasNextPage: false, + hasPreviousPage: true, + startCursor: '10', + endCursor: '16', + }, + }), + ) + .mockResolvedValueOnce(packagesProtectionRuleQueryPayload()); + + const findPaginationButtonPrev = () => + extendedWrapper(findPagination()).findByRole('button', { name: 'Prev' }); + + beforeEach(async () => { + createComponent({ mountFn: mountExtended, resolver }); + + await waitForPromises(); + + findPaginationButtonPrev().trigger('click'); + }); + + it('sends a second graphql api query with new pagination params', () => { + expect(resolver).toHaveBeenCalledTimes(2); + expect(resolver).toHaveBeenLastCalledWith( + expect.objectContaining({ + before: '10', + last: 10, + projectPath: 'path', + }), + ); + }); + }); + + describe('when button "next" is clicked', () => { + const resolver = jest + .fn() + .mockResolvedValueOnce(packagesProtectionRuleQueryPayload()) + .mockResolvedValueOnce( + packagesProtectionRuleQueryPayload({ + nodes: packagesProtectionRulesData.slice(10), + pageInfo: { + hasNextPage: true, + hasPreviousPage: false, + startCursor: '1', + endCursor: '10', + }, + }), + ); + + const findPaginationButtonNext = () => + extendedWrapper(findPagination()).findByRole('button', { name: 'Next' }); + + beforeEach(async () => { + createComponent({ mountFn: mountExtended, resolver }); + + await waitForPromises(); + + findPaginationButtonNext().trigger('click'); + }); + + it('sends a second graphql api query with new pagination params', () => { + expect(resolver).toHaveBeenCalledTimes(2); + expect(resolver).toHaveBeenLastCalledWith( + expect.objectContaining({ + after: '10', + first: 10, + projectPath: 'path', + }), + ); + }); + }); + }); }); it('does not initially render package protection form', async () => { diff --git a/spec/frontend/packages_and_registries/settings/project/settings/mock_data.js b/spec/frontend/packages_and_registries/settings/project/settings/mock_data.js index a8133c0ace6a1a..e49bf8c6131e6e 100644 --- a/spec/frontend/packages_and_registries/settings/project/settings/mock_data.js +++ b/spec/frontend/packages_and_registries/settings/project/settings/mock_data.js @@ -81,18 +81,12 @@ export const packagesCleanupPolicyMutationPayload = ({ override, errors = [] } = }); export const packagesProtectionRulesData = [ - { - id: `gid://gitlab/Packages::Protection::Rule/14`, - packageNamePattern: `@flight/flight-maintainer-14-*`, + ...Array.from(Array(15)).map((_e, i) => ({ + id: `gid://gitlab/Packages::Protection::Rule/${i}`, + packageNamePattern: `@flight/flight-maintainer-${i}-*`, packageType: 'NPM', pushProtectedUpToAccessLevel: 'MAINTAINER', - }, - { - id: `gid://gitlab/Packages::Protection::Rule/15`, - packageNamePattern: `@flight/flight-maintainer-15-*`, - packageType: 'NPM', - pushProtectedUpToAccessLevel: 'MAINTAINER', - }, + })), { id: 'gid://gitlab/Packages::Protection::Rule/16', packageNamePattern: '@flight/flight-owner-16-*', @@ -101,12 +95,22 @@ export const packagesProtectionRulesData = [ }, ]; -export const packagesProtectionRuleQueryPayload = ({ override, errors = [] } = {}) => ({ +export const packagesProtectionRuleQueryPayload = ({ + errors = [], + nodes = packagesProtectionRulesData.slice(0, 10), + pageInfo = { + hasNextPage: true, + hasPreviousPage: false, + startCursor: '0', + endCursor: '10', + }, +} = {}) => ({ data: { project: { id: '1', packagesProtectionRules: { - nodes: override || packagesProtectionRulesData, + nodes, + pageInfo: { __typename: 'PageInfo', ...pageInfo }, }, errors, }, -- GitLab From 1bce3ee76ffb60cb3fadcfcb6bc5157830ba7eb6 Mon Sep 17 00:00:00 2001 From: Rahul Chanila Date: Tue, 23 Jan 2024 14:33:58 +0000 Subject: [PATCH 2/3] refactor: Apply suggestion from @rchanila Commit includes - Changing one translation --- .../settings/project/components/packages_protection_rules.vue | 4 ++-- .../settings/components/packages_protection_rules_spec.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue b/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue index 7d23f1e1c4bbc3..2f559c8211ba2b 100644 --- a/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue +++ b/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue @@ -67,7 +67,7 @@ export default { }; }, update(data) { - return { ...data.project?.packagesProtectionRules }; + return data.project?.packagesProtectionRules ?? this.packageProtectionRulesQueryPayload; }, error(e) { this.fetchSettingsError = e; @@ -166,7 +166,7 @@ export default {
{ .mockResolvedValueOnce(packagesProtectionRuleQueryPayload()); const findPaginationButtonPrev = () => - extendedWrapper(findPagination()).findByRole('button', { name: 'Prev' }); + extendedWrapper(findPagination()).findByRole('button', { name: 'Previous' }); beforeEach(async () => { createComponent({ mountFn: mountExtended, resolver }); -- GitLab From 0b9d4917730bbaa434fdcd367b3c6c8ae52aa485 Mon Sep 17 00:00:00 2001 From: Paul Gascou-Vaillancourt Date: Tue, 23 Jan 2024 21:31:15 +0000 Subject: [PATCH 3/3] refactor: Apply suggestion from @pgascouvaillancourt --- .../settings/project/components/packages_protection_rules.vue | 2 +- .../settings/components/packages_protection_rules_spec.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue b/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue index 2f559c8211ba2b..2b85a787c933a9 100644 --- a/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue +++ b/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue @@ -154,7 +154,7 @@ export default { :fields="$options.fields" show-empty stacked="md" - class="gl-mb-3" + class="gl-mb-5!" :aria-label="$options.i18n.settingBlockTitle" :busy="isLoadingPackageProtectionRules" > diff --git a/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rules_spec.js b/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rules_spec.js index 4bbda015effd15..507711a0764c4e 100644 --- a/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rules_spec.js +++ b/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rules_spec.js @@ -129,7 +129,7 @@ describe('Packages protection rules project settings', () => { ); }); - describe('when button "prev" is clicked', () => { + describe('when button "Previous" is clicked', () => { const resolver = jest .fn() .mockResolvedValueOnce( @@ -168,7 +168,7 @@ describe('Packages protection rules project settings', () => { }); }); - describe('when button "next" is clicked', () => { + describe('when button "Next" is clicked', () => { const resolver = jest .fn() .mockResolvedValueOnce(packagesProtectionRuleQueryPayload()) -- GitLab