Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(pci.private-registry): edit cidr #15009

Open
wants to merge 3 commits into
base: feat/pci-private-registry_create-restriction_tapc-584
Choose a base branch
from

Conversation

ppprevost
Copy link
Contributor

Question Answer
Branch? develop
Bug fix? no
New feature? yes
Breaking change? no
Tickets TAPC-2320
License BSD 3-Clause
  • Try to keep pull requests small so they can be easily reviewed.
  • Commits are signed-off
  • Only FR translations have been updated
  • Branch is up-to-date with target branch
  • Lint has passed locally
  • Standalone app was ran and tested locally
  • Ticket reference is mentioned in linked commits (internal only)
  • Breaking change is mentioned in relevant commits

Description

Related

Pierre-Philippe added 3 commits January 17, 2025 17:50
ref: TAPC-2320
Signed-off-by: Pierre-Philippe <[email protected]>
ref: TAPC-2320
Signed-off-by: Pierre-Philippe <[email protected]>
ref: TAPC-2320
Signed-off-by: Pierre-Philippe <[email protected]>
@ppprevost ppprevost requested review from a team as code owners January 20, 2025 13:10
@ppprevost ppprevost requested review from chipp972, ghyenne, kqesar, frenautvh, oalkabouss and sachinrameshn and removed request for a team January 20, 2025 13:10
@github-actions github-actions bot added translation required feature New feature has conflicts Has conflicts to resolve before merging labels Jan 20, 2025
Comment on lines +15 to +21
vi.mock('@/pages/CIDR/useDatagridContext', async () => ({
__esModule: true,
default: () => ({
editActualRow: vi.fn(),
isDraft: false,
}),
}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should prefer do a global mock of the file and then mock only the default export when importing it ?

Also async isn't necessary here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

effectivly we have a test provider and setup on zimbra with all mocks
packages/manager/apps/zimbra/src/utils/test.provider.tsx
packages/manager/apps/zimbra/src/utils/test.setup.tsx

you can do same functional

Comment on lines +17 to +22
vi.mock('@/pages/CIDR/useDatagridContext', () => ({
__esModule: true,
default: () => ({
removeDraftRow: vi.fn(),
}),
}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the implementation required ?

@ppprevost ppprevost force-pushed the feat/pci-private-registry_edit_tapc-2320 branch from 7b6ced4 to 303282d Compare January 21, 2025 08:43
@github-actions github-actions bot removed the has conflicts Has conflicts to resolve before merging label Jan 21, 2025
@@ -25,19 +25,85 @@ import { useDatagridColumn } from '@/pages/CIDR/useDatagridColumn';
import Filters from '@/components/CIDR/Filters.component';
import { getRegistryQueyPrefixWithId } from '@/api/hooks/useIpRestrictions';
import useDataGridContext from '@/pages/CIDR/useDatagridContext';
import { FilterRestrictionsEnum } from '@/types';

const schemaAddCidr = (dataCIDR: string[], isUpdating: boolean) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's about separate schemaAddCidr in a dedicated file ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think better is on a dedicated file for all schema with zod

@github-actions github-actions bot added the has conflicts Has conflicts to resolve before merging label Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature has conflicts Has conflicts to resolve before merging translation required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants