feat(auth-admin): Create paper delegation #15992
Conversation
Fetches Zendesk ticket by id
Form for creating delegation, wip form action
…to feat/delegation-ui-gql
…is into feat/delegation-ui-gql
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
Outside diff range and nitpick comments (34)
libs/auth-api-lib/src/lib/delegations/constants/zendesk.ts (3)
1-1: LGTM! Consider adding a comment for non-Icelandic speakers.The
DELEGATION_TAGconstant is well-defined and follows proper naming conventions. Its export makes it reusable across the application, which is good for maintaining consistency.Consider adding a comment explaining the meaning of the Icelandic text for non-Icelandic speaking developers. This would improve code readability and maintainability.
+// 'umsokn_um_umboð_a_mínum_síðum' means 'application for delegation on my pages' in English export const DELEGATION_TAG = 'umsokn_um_umboð_a_mínum_síðum'
2-5: LGTM! Consider adjusting property naming and adding type safety.The
ZENDESK_CUSTOM_FIELDSconstant is well-structured and exported for reuse. However, there are a couple of suggestions for improvement:
- Adjust property naming: In JavaScript/TypeScript, it's more common to use camelCase for object properties. Consider renaming the properties:
export const ZENDESK_CUSTOM_FIELDS = { - DelegationFromReferenceId: 21401464004498, - DelegationToReferenceId: 21401435545234, + delegationFromReferenceId: 21401464004498, + delegationToReferenceId: 21401435545234, }
- Add type safety: To improve type checking and prevent potential errors, consider defining a type for the
ZENDESK_CUSTOM_FIELDSobject:type ZendeskCustomFields = { delegationFromReferenceId: number; delegationToReferenceId: number; }; export const ZENDESK_CUSTOM_FIELDS: ZendeskCustomFields = { delegationFromReferenceId: 21401464004498, delegationToReferenceId: 21401435545234, };This change enhances type safety and makes the code more robust.
1-5: Good overall structure and adherence to coding guidelines.The file structure and content align well with the coding guidelines:
- Reusability: These constants are exported and can be easily reused across different NextJS apps.
- TypeScript usage: The file uses TypeScript for defining constants.
- Tree-shaking and bundling: The small, focused nature of this file should contribute to effective tree-shaking and bundling.
To further improve the file:
- Consider adding a brief file-level comment explaining the purpose of these Zendesk-related constants in the context of delegations.
- If these constants are used in multiple places, ensure they are imported from this centralized location to maintain consistency across the application.
libs/auth-api-lib/src/lib/delegations/dto/create-paper-delegation.dto.ts (1)
13-15: LGTM with a minor suggestion:referenceIdproperty.The
referenceIdproperty is correctly implemented as a required string, with appropriate validation and Swagger documentation. The non-null assertion (!) is correctly used for this required field.For consistency with other properties, consider reordering the decorators to match the pattern used elsewhere in the file:
- @ApiProperty() - @IsString() + @IsString() + @ApiProperty() referenceId!: stringThis change would improve code consistency and readability.
apps/services/auth/admin-api/src/openApi.ts (1)
10-27: LGTM: OAuth2 configuration is well-implemented.The OAuth2 configuration is correctly implemented using the
addOAuth2method. It follows OpenAPI standards and uses environment variables for URL configuration, which is a good practice. The inclusion of both the default 'openid' scope and a custom delegation scope aligns well with the PR objectives.Consider adding a brief comment explaining the purpose of the 'ias' string parameter in the
addOAuth2call. This would improve code readability for future maintainers.libs/portals/admin/delegation-admin/src/screens/DelegationAdminDetails/DelegationAdmin.graphql (1)
62-64: LGTM: New mutation for deleting delegations.The addition of the
deleteCustomDelegationAdminmutation is a valuable enhancement to the delegation management capabilities. It aligns well with the PR objectives and provides a crucial feature for managing the lifecycle of delegations.A minor suggestion for improvement:
Consider adding a return type to the mutation for better clarity. For example:
mutation deleteCustomDelegationAdmin($id: String!): Boolean!This would explicitly indicate that the mutation returns a boolean value (success or failure of the deletion operation).
libs/api/domains/auth-admin/src/lib/delegationAdmin/delegation-admin.service.ts (1)
45-47: Implementation aligns with PR objectives.The changes to the
createDelegationAdminmethod successfully implement the new endpoint for creating paper delegations, which aligns with the PR objectives. The method now calls thedelegationAdminControllerCreatewith the appropriate input structure.Consider adding error handling.
To improve the robustness of the implementation, consider adding error handling to this method. This would allow for better management of potential issues that may arise during the delegation creation process.
Here's a suggested implementation with error handling:
async createDelegationAdmin(user: User, input: CreateDelegationInput) { try { return await this.delegationsWithAuth(user).delegationAdminControllerCreate({ createPaperDelegationDto: input, }); } catch (error) { // Log the error or handle it as appropriate for your application console.error('Error creating delegation admin:', error); throw new Error('Failed to create delegation admin'); } }libs/auth-api-lib/src/lib/delegations/models/delegation-delegation-type.model.ts (2)
Line range hint
1-70: Compliance with coding guidelines confirmedThe code adheres to the coding guidelines for
libs/**/*files:
- It uses TypeScript for defining props and exporting types.
- The model is defined in a way that makes it reusable across different NextJS apps.
To further improve tree-shaking capabilities, consider using named exports instead of default exports for the
DelegationDelegationTypeclass.Change the export statement to:
export { DelegationDelegationType };This allows for more efficient tree-shaking in consuming applications.
Line range hint
1-70: Suggestion for improved code readabilityThe overall structure of the
DelegationDelegationTypemodel is well-defined and follows Sequelize best practices. To further improve code readability and maintainability, consider extracting the table name and index definition into constants at the top of the file.Add the following constants at the beginning of the file:
const TABLE_NAME = 'delegation_delegation_type'; const UNIQUE_INDEX = { fields: ['delegation_id', 'delegation_type_id'], unique: true, };Then update the
@Tabledecorator to use these constants:@Table({ tableName: TABLE_NAME, timestamps: false, indexes: [UNIQUE_INDEX], })This change will make it easier to maintain and update the table configuration in the future.
libs/portals/admin/delegation-admin/src/components/DelegationList.tsx (2)
13-15: LGTM: Proper use of hooks enhances component functionality.The addition of
deleteCustomDelegationAdminMutationandrevalidatehooks is well-implemented:
- They're correctly placed at the top level of the component.
- They enhance the component's functionality for deleting delegations and updating the UI.
- This implementation supports reusability across different NextJS apps.
Consider adding error handling for the mutation:
const [deleteCustomDelegationAdminMutation] = useDeleteCustomDelegationAdminMutation({ onError: (error) => { // Handle error (e.g., show an error message) console.error('Failed to delete delegation:', error); } });This will improve the robustness of the component and provide a better user experience in case of errors.
20-40: LGTM: AccessCard implementation enhanced with deletion functionality.The changes to the AccessCard component usage are well-implemented:
- The
canModifyprop ensures only paper delegations can be deleted, enhancing security.- The
onDeletecallback correctly uses the mutation and revalidation.- The implementation aligns with TypeScript usage guidelines.
Consider the following improvements:
- Add error handling:
onDelete={async () => { try { const { data } = await deleteCustomDelegationAdminMutation({ variables: { id: delegation.id as string, }, }); if (data) { revalidate(); } } catch (error) { console.error('Failed to delete delegation:', error); // Show error message to user } }}
- Add user feedback:
onDelete={async () => { // Show loading indicator try { const { data } = await deleteCustomDelegationAdminMutation({ variables: { id: delegation.id as string, }, }); if (data) { revalidate(); // Show success message } } catch (error) { console.error('Failed to delete delegation:', error); // Show error message } finally { // Hide loading indicator } }}These changes will improve error handling and provide better user feedback during the deletion process.
libs/api/domains/auth/src/lib/models/delegation.model.ts (1)
60-61: LGTM! Consider adding a comment for clarity.The addition of the
referenceIdfield is well-implemented and aligns with the PR objectives. It's correctly typed and decorated for GraphQL use.Consider adding a brief comment explaining the purpose of this field, e.g.:
/** * Optional reference ID for the delegation, e.g., to link to a paper document. */ @Field(() => String, { nullable: true }) referenceId?: stringThis would enhance code readability and maintainability.
libs/services/auth/testing/src/fixtures/types.ts (2)
38-42: LGTM! Consider adding a comment for the newreferenceIdproperty.The addition of the
referenceIdproperty to theCreateCustomDelegationtype aligns well with the PR objective of creating a new endpoint for paper delegation. The implementation maintains consistency with the existing code structure and follows TypeScript best practices.Consider adding a brief comment explaining the purpose of the
referenceIdproperty to enhance code documentation. For example:export type CreateCustomDelegation = Optional< Pick< DelegationDTO, 'toNationalId' | 'fromNationalId' | 'fromName' | 'referenceId' // referenceId: Unique identifier for the paper delegation >, 'toNationalId' | 'fromNationalId' | 'fromName' | 'referenceId' > & { domainName: string scopes?: CreateCustomDelegationScope[] }
38-42: Changes align with PR objectives. Consider adding a type for the initiating national ID.The addition of the
referenceIdproperty to theCreateCustomDelegationtype supports the new paper delegation feature described in the PR objectives. The existing properties (toNationalId,fromNationalId,fromName) already accommodate the delegation process between two national IDs.To fully align with the PR objective of having a third national ID initiate the delegation, consider adding a property for the initiating ID:
export type CreateCustomDelegation = Optional< Pick< DelegationDTO, 'toNationalId' | 'fromNationalId' | 'fromName' | 'referenceId' | 'initiatedByNationalId' >, 'toNationalId' | 'fromNationalId' | 'fromName' | 'referenceId' | 'initiatedByNationalId' > & { domainName: string scopes?: CreateCustomDelegationScope[] }This addition would explicitly represent the third national ID that initiates the paper delegation process.
libs/api/domains/auth-admin/src/lib/delegationAdmin/delegation-admin.resolver.ts (1)
112-121: Improve handling of missing domain dataThe addition of the conditional check for
delegation.domainNameis a good improvement in error handling. However, there are a few suggestions to enhance this further:
Instead of returning empty strings for missing data, consider using
nullorundefined. This more accurately represents the absence of data and aligns better with TypeScript's strict null checks.Add a comment explaining the rationale behind this change to improve code maintainability.
Consider refactoring the code as follows:
if (!delegation.domainName) { // Return null values when domain name is missing to indicate absence of data return { name: null, displayName: null, description: null, nationalId: null, organisationLogoKey: null, }; }Also, add a comment above this block explaining why this check is necessary:
// Handle cases where domainName is missing to prevent errors in downstream logicThis change will make the code more robust and easier to understand for future maintainers.
libs/portals/admin/delegation-admin/src/screens/DelegationAdminDetails/DelegationAdmin.tsx (2)
93-97: LGTM! Consider refactoring for improved reusability.The changes correctly implement the swap in handling incoming delegations, completing the reversal mentioned in the summary. The TypeScript usage for props is appropriate, adhering to the coding guidelines.
To improve reusability across different NextJS apps, consider extracting the delegation rendering logic into a separate component or hook. This would make it easier to reuse this functionality in other parts of the application or in different apps. Here's a suggested refactoring:
type DelegationDirection = 'incoming' | 'outgoing'; const useDelegationList = (delegationAdmin: DelegationAdminResult, direction: DelegationDirection) => { const { formatMessage } = useLocale(); const delegations = direction === 'incoming' ? delegationAdmin.incoming : delegationAdmin.outgoing; return delegations.length > 0 ? ( <DelegationList direction={direction} delegationsList={delegations as AuthCustomDelegation[]} /> ) : ( <DelegationsEmptyState message={formatMessage(direction === 'incoming' ? m.delegationToNotFound : m.delegationFromNotFound)} imageAlt={formatMessage(direction === 'incoming' ? m.delegationToNotFound : m.delegationFromNotFound)} /> ); }; // Usage in the component: // const incomingDelegations = useDelegationList(delegationAdmin, 'incoming'); // const outgoingDelegations = useDelegationList(delegationAdmin, 'outgoing');This refactoring would make the code more DRY and easier to maintain.
Line range hint
1-110: Overall changes look good. Consider enhancing reusability.The changes successfully implement the swap in handling incoming and outgoing delegations as described in the PR objectives. The code adheres to the coding guidelines for the
libs/**/*pattern, using TypeScript for props and types, and maintaining a structure that allows for effective tree-shaking and bundling.To further improve the reusability of this component across different NextJS apps, consider the following suggestions:
Extract the tab content generation logic into a separate function or component. This would make it easier to reuse or modify the tab structure in other parts of the application.
Create a custom hook for handling the delegation admin data and actions. This would encapsulate the data fetching and manipulation logic, making it more reusable across different components.
Here's an example of how you could implement these suggestions:
const useDelegationAdminData = (delegationAdmin: DelegationAdminResult) => { const { formatMessage } = useLocale(); const navigate = useNavigate(); const { userInfo } = useAuth(); const hasAdminAccess = userInfo?.scopes.includes(AdminPortalScope.delegationSystemAdmin); const handleCreateDelegation = async () => { const maskedNationalId = await maskString(delegationAdmin.nationalId, userInfo?.profile.nationalId || ''); const query = new URLSearchParams({ fromNationalId: maskedNationalId ?? '' }); navigate(`${DelegationAdminPaths.CreateDelegation}?${query.toString()}`); }; const renderDelegationList = (direction: 'incoming' | 'outgoing') => { const delegations = direction === 'incoming' ? delegationAdmin.incoming : delegationAdmin.outgoing; const emptyMessage = direction === 'incoming' ? m.delegationToNotFound : m.delegationFromNotFound; return delegations.length > 0 ? ( <DelegationList direction={direction} delegationsList={delegations as AuthCustomDelegation[]} /> ) : ( <DelegationsEmptyState message={formatMessage(emptyMessage)} imageAlt={formatMessage(emptyMessage)} /> ); }; return { delegationAdmin, hasAdminAccess, handleCreateDelegation, renderDelegationList, }; }; // Usage in the component: // const { delegationAdmin, hasAdminAccess, handleCreateDelegation, renderDelegationList } = useDelegationAdminData(delegationAdminData);These changes would make the component more modular and easier to maintain, while also improving its reusability across different NextJS apps.
libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx (1)
82-82: LGTM: AccessCard href prop added correctly.The addition of the
hrefprop to the AccessCard component enhances its functionality by providing a direct link to the specific delegation. The use ofDelegationPaths.Delegationsfor constructing the URL is a good practice for centralized route management.Consider adding a type check for
delegation.idto ensure it's always defined:href={delegation.id ? `${DelegationPaths.Delegations}/${delegation.id}` : undefined}This change would improve type safety and prevent potential runtime errors if
delegation.idis unexpectedly undefined.libs/auth-api-lib/src/lib/delegations/delegations.module.ts (2)
8-11: LGTM! Consider grouping related imports.The new imports for ZendeskModule and ZendeskServiceOptions are correctly added. The import for the environment is also appropriate for accessing Zendesk options.
Consider grouping related imports together for better readability. For example, you could move the UserSystemNotificationModule import closer to other @island.is imports.
Also applies to: 13-13, 41-41
Line range hint
1-93: Consider exporting types for better reusability.The module complies with most of the coding guidelines for
libs/**/*files:
- It's designed for reusability across different NextJS apps.
- TypeScript is used effectively throughout the file.
- The module structure allows for effective tree-shaking and bundling.
To fully adhere to the guidelines, consider exporting any relevant types from this module. This will enhance reusability and type safety when this module is used in different NextJS apps. For example:
// Add this at the end of the file export type { Delegation, DelegationScope, DelegationIndex } from './models';This change will make it easier for other parts of the application to use these types when interacting with the DelegationsModule.
apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.auth.spec.ts (2)
19-34: LGTM: Well-implemented helper function with room for improvementThe
formatUrlfunction effectively handles dynamic URL generation for tests, creating necessary fixtures for domains and delegations. This approach aligns with best practices for test data generation.Consider adding error handling to the function. For example:
async function formatUrl(app: TestApp, endpoint: string, user?: User) { try { if (!endpoint.includes(':delegation')) { return endpoint } const factory = new FixtureFactory(app) const domain = await factory.createDomain({ name: 'd1', apiScopes: [{ name: 's1' }], }) const delegation = await factory.createCustomDelegation({ fromNationalId: user?.nationalId, domainName: domain.name, scopes: [{ scopeName: 's1' }], }) return endpoint.replace(':delegation', encodeURIComponent(delegation.id)) } catch (error) { console.error('Error formatting URL:', error) throw new Error('Failed to format URL for test') } }This addition will make the function more robust and easier to debug if issues arise during test execution.
100-135: LGTM: Well-implemented tests for users lacking admin scopeThese test cases effectively verify the behavior for users lacking admin scope:
- Consistent structure with previous tests, maintaining good code organization.
- Proper setup of user with read scope but without admin scope.
- Specific targeting of admin scope requirement for DELETE operation.
- Comprehensive assertions and proper cleanup for test isolation.
Consider adding a test case for the GET endpoint with a user having only the read scope. This would provide complete coverage of all scenarios:
it.each` method | endpoint | expectedStatus ${'GET'} | ${'/delegation-admin'} | ${200} ${'DELETE'} | ${'/delegation-admin/:delegation'} | ${403} `( '$method $endpoint should return $expectedStatus when user has only read scope', async ({ method, endpoint, expectedStatus }: TestEndpointOptions & { expectedStatus: number }) => { // Arrange const user = createCurrentUser({ scope: [DelegationAdminScopes.read], }) const app = await setupApp({ AppModule, SequelizeConfigService, user, dbType: 'postgres', }) const server = request(app.getHttpServer()) const url = await formatUrl(app, endpoint, user) // Act const res = await getRequestMethod(server, method)(url) // Assert expect(res.status).toEqual(expectedStatus) if (expectedStatus === 403) { expect(res.body).toMatchObject({ status: 403, type: 'https://httpstatuses.org/403', title: 'Forbidden', detail: 'Forbidden resource', }) } // CleanUp app.cleanUp() }, )This addition would ensure that the GET endpoint correctly allows access with only the read scope, while still forbidding the DELETE operation.
libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.tsx (2)
118-118: LGTM: Addition ofhrefprop enhances functionality.The
hrefprop added to theAccessCardcomponent improves the user experience by providing a direct link to specific delegation details. The use ofDelegationPaths.Delegationsensures consistency in path management.Consider adding type checking for
delegation.idto ensure it's always defined:href={`${DelegationPaths.Delegations}/${delegation.id ?? ''}`}This change would prevent potential runtime errors if
delegation.idis unexpectedlyundefined.
Line range hint
1-143: Overall compliance with coding guidelines is excellent.The component adheres well to the guidelines for
libs/**/*files:
- It's designed for reusability across different NextJS apps.
- TypeScript is effectively used for defining props and exporting types.
- The use of hooks and memoization suggests attention to performance.
To further improve tree-shaking, consider importing
sortByfrom lodash-es instead of the main lodash package:import sortBy from 'lodash-es/sortBy'This change would allow for more effective tree-shaking and potentially reduce the bundle size.
libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts (1)
63-73: LGTM: Improved flexibility withvalidityparameterThe changes to the
findAllValidIncomingmethod enhance its flexibility by allowing different validity values. The use of a default value maintains backward compatibility, which is excellent. This modification aligns well with the reusability guidelines for thelibsdirectory.For consistency, consider using object destructuring for all parameters in the
this.findAllIncomingcall:const { delegations, fromNameInfo } = await this.findAllIncoming( - { - nationalId, - validity, - domainName, - }, + { nationalId, domainName, validity }, useMaster, )This minor change would make the code more consistent with the method's parameter style.
libs/services/auth/testing/src/fixtures/fixture-factory.ts (1)
388-388: Consider using optional chaining forreferenceIdThe current implementation uses the nullish coalescing operator, which is good. However, you could make it even more concise by using optional chaining.
Consider updating the line to:
- referenceId: referenceId ?? undefined, + referenceId: referenceId?.toString(),This change ensures that if
referenceIdis provided, it's converted to a string, and if it's not provided, it remainsundefined.libs/clients/zendesk/src/lib/zendesk.service.ts (3)
Line range hint
142-146: Remove unnecessaryJSON.stringifyinsubmitTicketAxios automatically serializes objects to JSON when the
Content-Typeheader is set toapplication/json. Manually callingJSON.stringifyon theticketobject is unnecessary.Apply this diff to simplify the code:
-const newTicket = JSON.stringify({ +const newTicket = { ticket: { requester_id: requesterId, subject: subject?.trim() ?? '', comment: { body: message ?? '' }, tags, }, -}) +}
Line range hint
152-160: Handle potential undefined properties in error handlingIn the catch block of
submitTicket, accessinge.response.data.descriptionwithout checking might cause an error ife.responseore.response.datais undefined. This can occur in network errors or unexpected exceptions.Consider updating the error handling to safely access the description:
const errMsg = 'Failed to submit Zendesk ticket' -const description = e.response.data.description +const description = e?.response?.data?.description || e.message || 'Unknown error' this.logger.error(errMsg, { message: description, }) -throw new Error(`${errMsg}: ${description}`) +throw new Error(`${errMsg}: ${description}`)
Line range hint
152-160: Refactor duplicated error handling into a helper methodThe error handling logic in
submitTicketandgetTicketis similar. To adhere to the DRY principle, consider extracting this logic into a private helper method to reduce code duplication.Add a private
_handleErrormethod:+private _handleError(e: any, errMsg: string): never { + const description = e?.response?.data?.description || e.message || 'Unknown error' + this.logger.error(errMsg, { + message: description, + }) + throw new Error(`${errMsg}: ${description}`) +}Update the catch blocks to use the helper method:
In
submitTicket:} catch (e) { const errMsg = 'Failed to submit Zendesk ticket' - const description = e.response.data.description - this.logger.error(errMsg, { - message: description, - }) - throw new Error(`${errMsg}: ${description}`) + this._handleError(e, errMsg) }In
getTicket:} catch (e) { const errMsg = 'Failed to get Zendesk ticket' - const description = e.response.data.description - this.logger.error(errMsg, { - message: description, - }) - throw new Error(`${errMsg}: ${description}`) + this._handleError(e, errMsg) }Also applies to: 174-182
libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx (3)
59-61: Consider adding documentation for new propsThe newly added props
hrefandisAdminViewin theAccessCardPropsinterface lack documentation comments. Adding JSDoc comments will improve code readability and help other developers understand the purpose and usage of these props.Apply this suggestion to add documentation:
interface AccessCardProps { delegation: AuthCustomDelegation onDelete(delegation: AuthCustomDelegation): void onView?(delegation: AuthCustomDelegation): void variant?: 'outgoing' | 'incoming' direction?: 'incoming' | 'outgoing' canModify?: boolean + /** + * The URL to navigate to when editing or renewing a delegation. + */ href?: string + /** + * Indicates if the component is being viewed in the admin interface. + */ isAdminView?: boolean }
248-265: Use<Box>instead of<div>for consistencyThe use of a native
<div>element on line 248 introduces inconsistency in the component's styling and theming. Since the rest of the layout relies on@island.is/island-ui/corecomponents, consider replacing<div>with a<Box>component to maintain consistency and leverage the styling props provided byBox.Apply this diff to update the component:
<div> + <Box> {hasTags && ( <Box width="full"> <VisuallyHidden>{formatMessage(m.accessScopes)}</VisuallyHidden> <Inline alignY="bottom" space={1}> {tags.map((tag, index) => ( <Tag disabled key={index} variant={tag.isExpired ? 'disabled' : 'blue'} > {tag.name} </Tag> ))} </Inline> </Box> )} </div> + </Box>
Line range hint
296-316: Refactor nested ternary operators for improved readabilityThe nested ternary operators between lines 296 and 316 make the code harder to read and understand. Consider refactoring this section by using if-else statements or extracting the button rendering logic into separate functions or variables. This will enhance code clarity and maintainability.
Apply this refactor to improve readability:
{!isOutgoing && onView ? ( <Button size="small" variant="utility" onClick={() => onView(delegation)} > {formatMessage(coreMessages.view)} </Button> - ) : !isExpired && href ? ( + ) : !isExpired && href ? ( <Button icon="pencil" iconType="outline" size="small" variant="utility" onClick={() => navigate(href)} > {formatMessage(coreMessages.buttonEdit)} </Button> - ) : href ? ( + ) : isExpired && href ? ( <Button icon="reload" iconType="outline" size="small" variant="utility" onClick={() => navigate(href)} > {formatMessage(coreMessages.buttonRenew)} </Button> ) : null}Alternatively, extract the button rendering logic:
const renderActionButton = () => { if (!isOutgoing && onView) { return ( <Button size="small" variant="utility" onClick={() => onView(delegation)} > {formatMessage(coreMessages.view)} </Button> ) } else if (!isExpired && href) { return ( <Button icon="pencil" iconType="outline" size="small" variant="utility" onClick={() => navigate(href)} > {formatMessage(coreMessages.buttonEdit)} </Button> ) } else if (isExpired && href) { return ( <Button icon="reload" iconType="outline" size="small" variant="utility" onClick={() => navigate(href)} > {formatMessage(coreMessages.buttonRenew)} </Button> ) } return null }And then replace the ternary operators with:
{renderActionButton()}libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (1)
233-235: Remove redundant null check ondelegation.referenceIdThe check for
!delegation.referenceIdis redundant since it's already performed in lines 231-232. Removing the duplicate check improves code readability.Apply this diff to eliminate the redundant code:
- if (!delegation.referenceId) { - throw new NoContentException() - }apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.spec.ts (1)
198-206: Simplify mock implementation by removing unnecessary null coalescingIn the
mockImplementationofnationalRegistryApi.getIndividual, thereturn user ?? nullis unnecessary sincecreateNationalRegistryUseralways returns a user object. You can return the user directly.Apply this diff to simplify the code:
.mockImplementation(async (id) => { const user = createNationalRegistryUser({ nationalId: id, }) - return user ?? null + return user })
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (27)
- apps/services/auth/admin-api/src/app/v2/clients/me-clients.controller.ts (2 hunks)
- apps/services/auth/admin-api/src/app/v2/delegations/delegation-admin.controller.ts (3 hunks)
- apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.auth.spec.ts (1 hunks)
- apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.spec.ts (1 hunks)
- apps/services/auth/admin-api/src/openApi.ts (1 hunks)
- libs/api/domains/auth-admin/src/lib/delegationAdmin/delegation-admin.resolver.ts (1 hunks)
- libs/api/domains/auth-admin/src/lib/delegationAdmin/delegation-admin.service.ts (1 hunks)
- libs/api/domains/auth/src/lib/models/delegation.model.ts (1 hunks)
- libs/auth-api-lib/src/index.ts (2 hunks)
- libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (4 hunks)
- libs/auth-api-lib/src/lib/delegations/constants/zendesk.ts (1 hunks)
- libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts (2 hunks)
- libs/auth-api-lib/src/lib/delegations/delegations.module.ts (3 hunks)
- libs/auth-api-lib/src/lib/delegations/dto/create-paper-delegation.dto.ts (1 hunks)
- libs/auth-api-lib/src/lib/delegations/models/delegation-delegation-type.model.ts (1 hunks)
- libs/auth-api-lib/src/lib/delegations/models/delegation.model.ts (2 hunks)
- libs/auth-api-lib/src/lib/environments/environment.ts (1 hunks)
- libs/auth-api-lib/src/lib/environments/index.ts (1 hunks)
- libs/clients/zendesk/src/lib/zendesk.service.ts (4 hunks)
- libs/portals/admin/delegation-admin/src/components/DelegationList.tsx (1 hunks)
- libs/portals/admin/delegation-admin/src/screens/DelegationAdminDetails/DelegationAdmin.graphql (3 hunks)
- libs/portals/admin/delegation-admin/src/screens/DelegationAdminDetails/DelegationAdmin.tsx (2 hunks)
- libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx (8 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx (2 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.tsx (2 hunks)
- libs/services/auth/testing/src/fixtures/fixture-factory.ts (2 hunks)
- libs/services/auth/testing/src/fixtures/types.ts (1 hunks)
Additional context used
Path-based instructions (27)
apps/services/auth/admin-api/src/app/v2/clients/me-clients.controller.ts (2)
Pattern
apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/auth/admin-api/src/app/v2/delegations/delegation-admin.controller.ts (2)
Pattern
apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.auth.spec.ts (2)
Pattern
apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.spec.ts (2)
Pattern
apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/auth/admin-api/src/openApi.ts (2)
Pattern
apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/api/domains/auth-admin/src/lib/delegationAdmin/delegation-admin.resolver.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/api/domains/auth-admin/src/lib/delegationAdmin/delegation-admin.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/api/domains/auth/src/lib/models/delegation.model.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-api-lib/src/index.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-api-lib/src/lib/delegations/constants/zendesk.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-api-lib/src/lib/delegations/delegations.module.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-api-lib/src/lib/delegations/dto/create-paper-delegation.dto.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-api-lib/src/lib/delegations/models/delegation-delegation-type.model.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-api-lib/src/lib/delegations/models/delegation.model.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-api-lib/src/lib/environments/environment.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-api-lib/src/lib/environments/index.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/clients/zendesk/src/lib/zendesk.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/delegation-admin/src/components/DelegationList.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/delegation-admin/src/screens/DelegationAdminDetails/DelegationAdmin.graphql (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/delegation-admin/src/screens/DelegationAdminDetails/DelegationAdmin.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/services/auth/testing/src/fixtures/fixture-factory.ts (2)
Pattern
libs/services/**/*: "Confirm that the code adheres to the following:
- Shared services and modules that can be utilized across NestJS apps.
- Proper implementation of NestJS providers, including custom providers.
- Adherence to NestJS module encapsulation and isolation principles."
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/services/auth/testing/src/fixtures/types.ts (2)
Pattern
libs/services/**/*: "Confirm that the code adheres to the following:
- Shared services and modules that can be utilized across NestJS apps.
- Proper implementation of NestJS providers, including custom providers.
- Adherence to NestJS module encapsulation and isolation principles."
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
Additional comments not posted (36)
libs/auth-api-lib/src/lib/environments/environment.ts (1)
1-9: Good job on creating a reusable configuration object.The configuration structure is well-organized and can be easily imported and used across different NextJS apps. Using environment variables for sensitive information is a good security practice.
libs/auth-api-lib/src/lib/delegations/dto/create-paper-delegation.dto.ts (5)
1-4: LGTM: Imports and overall structure.The imports are appropriate for the required functionality, and the file structure is clean and focused on a single responsibility. The use of TypeScript aligns with the coding guidelines for the
libsdirectory.
5-7: LGTM:fromNationalIdproperty.The
fromNationalIdproperty is correctly typed as a required string, with appropriate validation using@IsString()and Swagger documentation using@ApiProperty(). The non-null assertion (!) is correctly used for this required field.
9-11: LGTM:toNationalIdproperty.The
toNationalIdproperty is correctly implemented as a required string, with appropriate validation (@IsString()) and Swagger documentation (@ApiProperty()). The non-null assertion (!) is correctly used for this required field.
17-20: LGTM:validToproperty.The
validToproperty is well-implemented as an optional Date or null field. The use of@IsOptional()and@IsDateString()decorators provides appropriate validation. The Swagger documentation using@ApiPropertyOptional()correctly indicates that the field is optional and nullable. The property declaration with?accurately represents an optional field in TypeScript.
1-21: Compliance with coding guidelines confirmed.This DTO adheres to the coding guidelines for the
libs/**/*pattern:
- It's a reusable component that can be utilized across different NextJS apps.
- TypeScript is used effectively for defining props and exporting types.
- The structure supports effective tree-shaking and bundling practices.
Great job on maintaining consistency with the project's coding standards!
apps/services/auth/admin-api/src/openApi.ts (2)
2-3: LGTM: Imports are appropriate and follow best practices.The added imports for
environmentandAuthScopeare relevant to the changes made and follow good practices for code organization and reuse.
1-27: Confirms adherence to NestJS best practices.The implementation follows NestJS best practices for API documentation using
@nestjs/swagger. The file structure and naming conventions are consistent with NestJS standards.To ensure comprehensive test coverage, consider adding unit tests for this OpenAPI configuration. Run the following command to check for existing test files:
If no test files are found, consider adding tests to verify the correct setup of the OpenAPI configuration.
libs/portals/admin/delegation-admin/src/screens/DelegationAdminDetails/DelegationAdmin.graphql (2)
8-8: LGTM: Addition ofreferenceIdenhances delegation management.The inclusion of
referenceIdin both incoming and outgoing delegations is a valuable addition. It provides a unique identifier for each delegation, which aligns with the PR objective of streamlining the delegation process. This enhancement will likely improve traceability and management of delegations.Also applies to: 35-35
Line range hint
1-64: Overall assessment: Changes enhance delegation management capabilities.The modifications to this GraphQL file effectively support the PR objectives by improving delegation management. The addition of
referenceIdand the new deletion mutation provide valuable enhancements to the system's functionality.Regarding the coding guidelines for
libs/**/*:
- The changes maintain reusability across different NextJS apps.
- TypeScript types are inferred, ensuring type safety.
- The additions are minimal and focused, supporting effective tree-shaking and bundling.
Great job on these improvements!
libs/api/domains/auth-admin/src/lib/delegationAdmin/delegation-admin.service.ts (1)
Line range hint
1-47: Overall assessment: Changes align with PR objectives and coding guidelines.The modifications to the
createDelegationAdminmethod successfully implement the new endpoint for creating paper delegations, aligning with the PR objectives. The code adheres to the coding guidelines forlibs/**/*files, maintaining reusability and proper TypeScript usage. The only suggested improvement is the addition of error handling to enhance robustness.libs/auth-api-lib/src/lib/delegations/models/delegation-delegation-type.model.ts (1)
54-54: Approved: Improved type safety forvalidTopropertyThe change from
DatetoDate | nullfor thevalidToproperty improves type safety by explicitly allowingnullvalues. This is consistent with theallowNull: truesetting in the@Columndecorator and provides better clarity for cases where a delegation might not have an expiration date.libs/portals/admin/delegation-admin/src/components/DelegationList.tsx (1)
4-5: LGTM: New imports align with TypeScript usage and maintainability guidelines.The addition of
useDeleteCustomDelegationAdminMutationfrom a generated file anduseRevalidatorfrom 'react-router-dom' demonstrates good practices:
- It leverages TypeScript for type safety.
- Using a generated file for the mutation enhances maintainability.
- The
useRevalidatorhook suggests efficient state management.These additions align well with the coding guidelines for reusability and TypeScript usage in the
libs/**/*pattern.libs/services/auth/testing/src/fixtures/types.ts (1)
Line range hint
1-42: Adherence to NestJS guidelines for shared services confirmed.This file, containing shared type definitions, aligns with the guideline of providing reusable components across NestJS apps. While it doesn't directly implement NestJS providers or modules, these type definitions contribute to maintaining consistency and type safety throughout the application.
libs/portals/admin/delegation-admin/src/screens/DelegationAdminDetails/DelegationAdmin.tsx (1)
76-80: LGTM! Verify usage ofdelegationAdmin.outgoing.The changes correctly implement the swap in handling outgoing delegations. The TypeScript usage for props is appropriate, adhering to the coding guidelines.
To ensure consistency across the codebase, let's verify the usage of
delegationAdmin.outgoing:Verification successful
Verified usage of
delegationAdmin.outgoing.The
delegationAdmin.outgoingis only used withinlibs/portals/admin/delegation-admin/src/screens/DelegationAdminDetails/DelegationAdmin.tsx, ensuring that the changes are localized and do not affect other parts of the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of delegationAdmin.outgoing in the codebase # Test: Search for occurrences of delegationAdmin.outgoing rg --type typescript -A 5 'delegationAdmin\.outgoing'Length of output: 90
Script:
#!/bin/bash # Description: Verify usage of delegationAdmin.outgoing in the codebase without specifying file type # Test: Search for occurrences of delegationAdmin.outgoing rg 'delegationAdmin\.outgoing'Length of output: 339
libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx (2)
17-17: LGTM: Import statement added correctly.The import of
DelegationPathsfrom the relative path is appropriate and follows TypeScript conventions.
Line range hint
1-110: Overall assessment: Component maintains reusability and follows guidelines.The changes to the
DelegationsIncomingcomponent adhere to the coding guidelines for files in thelibsdirectory:
- Reusability: The component remains reusable across different NextJS apps. The added functionality (linking to specific delegations) is generic and doesn't introduce app-specific dependencies.
- TypeScript usage: The component consistently uses TypeScript for defining props and types throughout the file.
- Tree-shaking and bundling: The component structure, including the new import and prop addition, allows for effective tree-shaking and bundling.
The enhancements made to the component improve its functionality while maintaining its modular and reusable nature.
apps/services/auth/admin-api/src/app/v2/clients/me-clients.controller.ts (2)
10-10: LGTM: Import of Query decoratorThe addition of the
Queryimport from@nestjs/commonis correct and necessary for the changes made to thefindByTenantIdAndClientIdmethod. This aligns with NestJS best practices for handling query parameters.
Line range hint
1-155: Verify and update API documentationThe changes made to the
findByTenantIdAndClientIdmethod may require updates to the API documentation. Please review and update the@Documentationdecorator for this method to reflect the new query parameter.Consider adding or updating the documentation as follows:
@Documentation({ description: 'Get client by id and tenant for the current user.', response: { status: 200, type: AdminClientDto }, parameters: [ { name: 'includeArchived', required: false, type: 'boolean', description: 'Whether to include archived clients in the response', }, ], })Also, ensure that any OpenAPI/Swagger documentation is regenerated to reflect these changes.
apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.auth.spec.ts (4)
1-17: LGTM: Imports and setup are well-structuredThe imports are appropriate for the testing context, utilizing necessary testing utilities and modules. The use of the
@island.isnamespace indicates a modular project structure, which is a good practice for large-scale applications.
36-63: LGTM: Well-structured tests for unauthenticated requestsThe test cases for unauthenticated requests are well-implemented:
- Efficient use of
it.eachto cover multiple endpoints and HTTP methods.- Proper setup of the test environment using
setupAppWithoutAuth.- Comprehensive assertions checking both status code and response body structure.
This approach aligns with NestJS testing best practices and provides good coverage for unauthenticated scenarios.
65-98: LGTM: Comprehensive tests for authenticated users without correct scopesThese test cases are well-implemented and thorough:
- Consistent structure with previous tests, using
it.eachfor multiple scenarios.- Proper setup of authenticated user without required scopes.
- Comprehensive assertions for both status code and response body.
- Inclusion of cleanup step for proper test isolation.
This approach ensures good coverage of authorization scenarios and aligns with NestJS testing best practices.
1-135: Overall: Excellent test coverage and implementationThis test file provides comprehensive coverage for the delegation administration API endpoints, aligning well with the PR objectives. Key strengths include:
- Thorough testing of authentication and authorization scenarios.
- Efficient use of
it.eachfor multiple test cases, promoting code reusability.- Proper setup and cleanup procedures, ensuring test isolation.
- Adherence to NestJS testing best practices.
- Clear and consistent test structure throughout the file.
The test suite effectively verifies the behavior of the new endpoint for delegation processes, covering unauthenticated requests, incorrect scopes, and admin-specific operations. This robust testing approach will help ensure the reliability and security of the delegation feature.
libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.tsx (1)
20-20: LGTM: Import statement is correctly placed and follows conventions.The new import statement for
DelegationPathsis appropriately placed with other imports and follows the expected structure for a shared module. The naming convention is consistent with TypeScript best practices.libs/auth-api-lib/src/index.ts (3)
42-42: LGTM: New DTO export aligns with PR objectivesThe addition of the
create-paper-delegation.dtoexport aligns well with the PR objective of creating a new endpoint for paper delegation. This export adheres to our TypeScript usage guidelines and promotes reusability across different NextJS apps.
62-62: Please clarify the purpose of Zendesk constantsThe addition of the Zendesk constants export is consistent with our module structure. However, could you please clarify the purpose of these constants in relation to the paper delegation feature? This will help ensure that we're maintaining a clear and purposeful API surface.
Line range hint
42-62: Verify completeness of exports for paper delegation featureThe new exports for the paper delegation DTO and Zendesk constants have been added correctly, adhering to our coding guidelines for reusability, TypeScript usage, and effective tree-shaking. To ensure the completeness of this feature:
- Please verify that all necessary types, services, and models related to the paper delegation feature are exported in this file.
- Consider adding a brief comment above the new exports to group them and explain their purpose, enhancing code readability.
libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts (1)
36-36: LGTM: Addition ofvalidityproperty enhances type flexibilityThe addition of the optional
validityproperty to theFindAllValidIncomingOptionstype is a good improvement. It enhances the flexibility of the type while maintaining backward compatibility. This change aligns well with the TypeScript usage guidelines for thelibsdirectory.libs/services/auth/testing/src/fixtures/fixture-factory.ts (2)
379-379: LGTM: New parameter added to method signatureThe addition of the
referenceIdparameter to thecreateCustomDelegationmethod signature is appropriate and aligns with the PR objective of creating a new endpoint for paper delegation.
Line range hint
379-389: Verify usage ofreferenceIdin related componentsThe addition of
referenceIdto thecreateCustomDelegationmethod is a good enhancement. However, we should ensure that this change is reflected in other parts of the system that interact with delegations.Let's verify the usage of
referenceIdin related components:This script will help us identify any other components that might need to be updated to handle the new
referenceIdfield.Verification successful
Usage of
referenceIdVerified SuccessfullyAll occurrences of
referenceIdin delegation-related components have been checked and are consistent with the recent changes.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of referenceId in delegation-related files # Search for files that might be affected by this change echo "Files potentially affected by the referenceId change:" fd -e ts -e js | rg -i 'delegation|custom.*delegation' # Check for existing usage of referenceId in these files echo "\nCurrent usage of referenceId in delegation-related files:" rg 'referenceId' $(fd -e ts -e js | rg -i 'delegation|custom.*delegation')Length of output: 35226
libs/auth-api-lib/src/lib/delegations/models/delegation.model.ts (1)
99-108: Verify the precedence and correctness of 'validTo' calculationThe new logic in the
validTogetter prioritizesdelegationDelegationTypesoverdelegationScopeswhen determining thevalidTodate. Please ensure that this change aligns with the intended business logic and that the precedence is intended. Consider scenarios where bothdelegationDelegationTypesanddelegationScopesare present.Run the following script to verify the usage and impact of
validTo:libs/clients/zendesk/src/lib/zendesk.service.ts (3)
8-15: Good use of TypeScript enums and typesThe addition of the
TicketStatusenum and the defined types forTicketandSubmitTicketInputenhance type safety and code readability. This aligns well with the coding guidelines for TypeScript usage in defining and exporting types.Also applies to: 17-35
Line range hint
17-21: Ensure all required fields are provided inSubmitTicketInputThe
SubmitTicketInputtype definesmessageas required. However, insubmitTicket, there's a fallback to an empty string formessage(message ?? '') in case it's undefined. Sincemessageis required, this fallback might be unnecessary, ormessageshould be optional if an empty message is acceptable.Please verify if
messageshould be optional. If it can be undefined, update theSubmitTicketInputtype accordingly.
166-183:getTicketmethod implementation looks goodThe
getTicketmethod correctly retrieves a ticket by its ID and handles errors appropriately (after applying the suggested error handling improvements). This enhances the service's functionality as per the PR objectives.libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx (1)
83-86: Ensure proper handling ofvalidToforGeneralMandatedelegationsIn the
isExpiredcalculation, the condition now checks ifdelegation.validToexists or if thedelegation.typeisAuthDelegationType.GeneralMandate. Ensure that forGeneralMandatedelegations without avalidTodate, theisDateExpiredfunction handles theundefinedvalue correctly.Run the following script to check if there are any
GeneralMandatedelegations without avalidTodate:This ensures that the code safely handles cases where
validTomight be undefined.Verification successful
GeneralMandate delegations without a
validTodate are properly handledNo
GeneralMandatedelegations without avalidTodate were found. TheisDateExpiredfunction correctly manages all existing cases.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find GeneralMandate delegations without a validTo date. # Test: Search for delegations of type GeneralMandate where validTo is null or undefined. ast-grep --lang typescript --pattern $'delegation: { type: "GeneralMandate", validTo: $_ }' --json | jq '.captures[] | select(.value == null or .value == "undefined")'Length of output: 240
Script:
#!/bin/bash # Description: Find GeneralMandate delegations without a validTo date. # Search for delegations of type GeneralMandate where validTo is null, undefined, or not defined. ast-grep --lang typescript --pattern $'delegation: { type: "GeneralMandate", $$$ }' --json | jq 'map(select((.validTo == null) or (.validTo == "undefined") or (.validTo | not)))'Length of output: 186
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (1)
288-289: Ensure both national IDs are valid person identification numbersWhile the current check verifies that both
fromNationalIdandtoNationalIdare valid person IDs, consider adding error handling or logging in case thekennitalalibrary fails or returns unexpected results.Run the following script to ensure that the
kennitala.isPersonfunction behaves as expected for the provided national IDs:
apps/services/auth/admin-api/src/app/v2/clients/me-clients.controller.ts
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts
Outdated
Show resolved
Hide resolved
apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.spec.ts
Show resolved
Hide resolved
apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.spec.ts
Show resolved
Hide resolved
apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.spec.ts
Outdated
Show resolved
Hide resolved
apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.spec.ts
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts
Outdated
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts
Outdated
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts
Outdated
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts
Outdated
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts
Outdated
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/models/delegation.model.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
libs/auth-api-lib/sequelize.config.js (1)
Line range hint
12-19: Consider updating test environment for consistencyWhile the changes to the development environment improve clarity, the test environment remains unchanged. This inconsistency could potentially lead to confusion or errors when moving between environments.
Consider applying similar changes to the test environment for consistency:
test: { - username: 'test_db', - password: 'test_db', - database: 'test_db', - host: process.env.DB_HOST, + username: process.env.DB_USER_AUTH_DB_TEST ?? 'test_db', + password: process.env.DB_PASS_AUTH_DB_TEST ?? 'test_db', + database: process.env.DB_NAME_AUTH_DB_TEST ?? 'test_db', + host: process.env.DB_HOST_AUTH_DB_TEST ?? 'localhost', dialect: 'postgres', - port: process.env.PORT, + port: process.env.DB_PORT_AUTH_DB_TEST ?? 5433, seederStorage: 'sequelize', },This change would make the test environment consistent with the development environment while still allowing for different values to be set through environment variables.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.spec.ts (1 hunks)
- apps/services/auth/admin-api/src/openApi.ts (1 hunks)
- libs/auth-api-lib/sequelize.config.js (1 hunks)
- libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (3 hunks)
- libs/auth-api-lib/src/lib/delegations/models/delegation.model.ts (4 hunks)
Files skipped from review as they are similar to previous changes (3)
- apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.spec.ts
- apps/services/auth/admin-api/src/openApi.ts
- libs/auth-api-lib/src/lib/delegations/models/delegation.model.ts
Additional context used
Path-based instructions (2)
libs/auth-api-lib/sequelize.config.js (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
Additional comments not posted (3)
libs/auth-api-lib/sequelize.config.js (1)
Line range hint
1-39: Ensure documentation is updated for new environment variablesThe changes in this configuration file align well with the coding guideline for reusability across different NextJS apps. The use of environment variables promotes configurability and allows this library to be easily integrated into various projects.
To ensure proper documentation and usage across projects:
- Update any relevant documentation to reflect the new environment variable names (DB_USER_AUTH_DB, DB_PASS_AUTH_DB, DB_PORT_AUTH_DB, and the suggested DB_NAME_AUTH_DB).
- If there's a .env.example file in the project, make sure it includes these new variables.
- Update any deployment scripts or CI/CD pipelines to use the new environment variable names.
You can use the following script to check for existing documentation that might need updating:
This will help identify files that might reference the old environment variable names and need to be updated.
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (2)
Line range hint
1-270: Overall assessment: Changes align well with PR objectivesThe modifications to
DelegationAdminCustomServiceeffectively implement the new endpoint for paper delegation. The changes include:
- New methods for creating and validating delegations.
- Improved error handling with specific HTTP exceptions.
- Enhanced delegation retrieval with concurrent fetching.
- Proper integration with Zendesk for ticket validation.
These changes align well with the PR objectives and improve the overall functionality of the authentication and delegation system.
48-67: 🛠️ Refactor suggestionUse specific HTTP exception for missing custom fields
Replace the generic
ErrorwithBadRequestExceptionto provide a more appropriate HTTP status code to the client.Apply this diff:
- throw new Error('Zendesk ticket is missing required custom fields') + throw new BadRequestException('Zendesk ticket is missing required custom fields')Likely invalid or redundant comment.
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts
Outdated
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (2)
72-138: LGTM: Improved efficiency and comprehensiveness, with a minor suggestionThe
getAllDelegationsByNationalIdmethod has been effectively refactored to usePromise.allfor concurrent fetching of delegations, which improves efficiency. The inclusion of both custom and general mandate delegations provides a more comprehensive view.Consider simplifying the return statement for better readability:
return { - incoming: [...incomingCustomDelegations, ...incomingGeneralDelegations], - outgoing: [ - ...outgoingGeneralDelegations.map((d) => - d.toDTO(AuthDelegationType.GeneralMandate), - ), - ...outgoingCustomDelegations.map((delegation) => delegation.toDTO()), - ], + incoming: [...incomingCustomDelegations, ...incomingGeneralDelegations], + outgoing: [ + ...outgoingGeneralDelegations.map((d) => d.toDTO(AuthDelegationType.GeneralMandate)), + ...outgoingCustomDelegations.map((d) => d.toDTO()), + ], }This minor change improves consistency and reduces line count without affecting functionality.
142-207: LGTM: Comprehensive implementation with a suggestion for error handlingThe
createDelegationmethod is well-implemented with thorough validation steps, proper use of specific exceptions, and a complete delegation creation process.Consider adding error handling for the indexing step:
- void this.delegationIndexService.indexCustomDelegations( - delegation.toNationalId, - ) + this.delegationIndexService.indexCustomDelegations(delegation.toNationalId) + .catch(error => { + console.error('Failed to index custom delegations:', error); + // Consider whether to throw an error or handle it in another way + });This change ensures that any errors during the indexing process are logged and can be handled appropriately, improving the robustness of the method.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (3 hunks)
Additional context used
Path-based instructions (1)
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
Additional comments not posted (4)
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (4)
1-30: LGTM: New imports align with added functionalityThe new imports are correctly added and correspond to the new features and modifications in the service. This enhances the service's capabilities for handling paper delegations and Zendesk ticket integration.
37-38: LGTM: Constructor updated with new dependenciesThe constructor has been correctly updated to include the new dependencies (DelegationDelegationType model and DelegationsIncomingCustomService) required for the added functionality.
253-270: LGTM: Robust validation of national IDsThe
validatePersonsNationalIdsmethod provides crucial validation for the creation of delegations. It correctly checks if the IDs are different and if they are valid person national IDs using the kennitala library. The use ofBadRequestExceptionfor error handling is appropriate.This implementation enhances the overall data integrity and security of the delegation process.
48-67: 🛠️ Refactor suggestionConsider using a specific exception for missing custom fields
The
getNationalIdsFromZendeskTicketmethod correctly extracts national IDs from the Zendesk ticket. However, consider using a more specific exception, such asBadRequestException, instead of a genericErrorwhen required custom fields are missing.Replace the generic Error with BadRequestException:
- throw new Error('Zendesk ticket is missing required custom fields') + throw new BadRequestException('Zendesk ticket is missing required custom fields')This change will provide a more appropriate HTTP status code (400) to the client, indicating a bad request due to missing fields.
Likely invalid or redundant comment.
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts
Show resolved
Hide resolved
# Conflicts: # libs/auth-api-lib/src/lib/delegations/delegations.module.ts
apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.spec.ts
Outdated
Show resolved
Hide resolved
apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.spec.ts
Outdated
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts
Outdated
Show resolved
Hide resolved
libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (3)
72-138: LGTM: Improved delegation fetching with minor suggestionThe refactoring to use
Promise.allfor concurrent fetching of delegations is a good performance improvement. The method correctly aggregates results from different types of delegations.Consider using more consistent variable naming for better readability:
- const [ - incomingCustomDelegations, - incomingGeneralDelegations, - outgoingCustomDelegations, - outgoingGeneralDelegations, - ] = await Promise.all([ + const [ + incomingCustom, + incomingGeneral, + outgoingCustom, + outgoingGeneral, + ] = await Promise.all([ // ... (rest of the code remains the same) ]); return { - incoming: [...incomingCustomDelegations, ...incomingGeneralDelegations], + incoming: [...incomingCustom, ...incomingGeneral], outgoing: [ - ...outgoingGeneralDelegations.map((d) => + ...outgoingGeneral.map((d) => d.toDTO(AuthDelegationType.GeneralMandate), ), - ...outgoingCustomDelegations.map((delegation) => delegation.toDTO()), + ...outgoingCustom.map((delegation) => delegation.toDTO()), ], };This change makes the variable names more concise and consistent.
142-204: LGTM: Improved delegation creation with suggestion for error handlingThe
createDelegationmethod has been significantly enhanced with comprehensive validations and improved logic. The use ofPromise.allfor fetching names is a good performance optimization.Consider adding error handling for the indexing operation:
- void this.indexDelegations(delegation.toNationalId) + this.indexDelegations(delegation.toNationalId) + .catch(error => { + console.error('Failed to index delegations:', error); + // Consider whether to throw an error or handle it in another way + });This change ensures that any errors during indexing are logged and can be handled appropriately.
214-216: Remove redundant check and LGTM for delegation deletionThe
deleteDelegationmethod has been improved to handle associated delegation types and update the index after deletion.Remove the redundant check for
delegation.referenceId:- if (!delegation.referenceId) { - throw new NoContentException() - }This check is already performed earlier in the method and is unnecessary here.
Also applies to: 244-245
libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts (3)
284-288: Consider renaming method to reflect multiple delegationsThe method
getGeneralMandateDelegationreturns an array of delegations. For consistency and clarity, consider renaming it togetGeneralMandateDelegations.This will align with the naming convention used in other methods like
getCustomDelegationsand enhance code readability.
496-503: Rename variabledelegationtodelegationsfor clarityWithin the
getGeneralMandateDelegationmethod, the variabledelegationholds an array of delegations. Renaming it todelegationswill make the code clearer and maintain consistency with variable naming conventions in the codebase.
Line range hint
283-503: Consider refactoring to reduce code duplicationMultiple methods (
getGeneralMandateDelegation,getCustomDelegations,getRepresentativeDelegations, etc.) share similar logic for retrieving delegations. Refactoring these methods to extract common functionality can enhance maintainability, reduce code duplication, and simplify future updates.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (4 hunks)
- libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments not posted (3)
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (3)
37-38: LGTM: Constructor updatesThe new dependencies are correctly injected and align with the added functionality. This change supports the new features implemented in the service.
Also applies to: 39-39, 41-41, 44-44
249-266: LGTM: Robust national ID validationThe
validatePersonsNationalIdsmethod provides robust validation for national IDs. It correctly checks for different IDs and ensures they belong to persons, throwing appropriate exceptions for invalid cases.
5-5: 🛠️ Refactor suggestionUpdate deprecated 'uuidv4' import
The
uuidv4package is deprecated. It's recommended to use theuuidpackage for better support and future maintenance.Apply this diff to update the import:
- import { uuid } from 'uuidv4' + import { v4 as uuid } from 'uuid'Likely invalid or redundant comment.
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (3)
71-141: LGTM: Improved getAllDelegationsByNationalId methodThe refactoring of
getAllDelegationsByNationalIdmethod is well done. It now efficiently fetches both custom and general mandate delegations concurrently, improving performance.Consider destructuring the Promise.all result for better readability:
- const [ - incomingCustomDelegations, - incomingGeneralDelegations, - outgoingCustomDelegations, - outgoingGeneralDelegations, - ] = await Promise.all([ + const { + incomingCustom, + incomingGeneral, + outgoingCustom, + outgoingGeneral + } = await Promise.all([ // ... existing promises - ]) + ]).then(([incomingCustom, incomingGeneral, outgoingCustom, outgoingGeneral]) => ({ + incomingCustom, + incomingGeneral, + outgoingCustom, + outgoingGeneral + })) return { - incoming: [...incomingCustomDelegations, ...incomingGeneralDelegations], + incoming: [...incomingCustom, ...incomingGeneral], outgoing: [ - ...outgoingGeneralDelegations.map((d) => + ...outgoingGeneral.map((d) => d.toDTO(AuthDelegationType.GeneralMandate), ), - ...outgoingCustomDelegations.map((delegation) => delegation.toDTO()), + ...outgoingCustom.map((delegation) => delegation.toDTO()), ], }This change improves readability and makes the code more self-documenting.
144-207: LGTM: Comprehensive createDelegation method implementationThe
createDelegationmethod is well-implemented with thorough validation steps and correct creation of the delegation.Consider adding error handling for the asynchronous indexing operation:
- void this.indexDelegations(delegation.toNationalId) + this.indexDelegations(delegation.toNationalId) + .catch(error => { + console.error('Failed to index delegations:', error); + // Consider whether to throw an error or handle it in another way + });This change ensures that any errors during indexing are logged and can be handled appropriately.
Line range hint
209-249: LGTM: Updated deleteDelegation methodThe
deleteDelegationmethod has been improved to include the deletion of associated delegation types and proper indexing after deletion.Similar to the createDelegation method, consider adding error handling for the asynchronous indexing operation:
- void this.indexDelegations(delegation.toNationalId) + this.indexDelegations(delegation.toNationalId) + .catch(error => { + console.error('Failed to index delegations after deletion:', error); + // Consider whether to throw an error or handle it in another way + });This change ensures that any errors during indexing after deletion are logged and can be handled appropriately.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.spec.ts (1 hunks)
- libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments not posted (5)
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (5)
37-45: LGTM: Constructor updated with new dependenciesThe constructor has been correctly updated to include the new dependencies required for the added functionality.
48-69: LGTM: New method to extract national IDs from Zendesk ticketThe
getNationalIdsFromZendeskTicketmethod correctly extracts the required information from the Zendesk ticket and handles errors appropriately by throwing aBadRequestExceptionwhen custom fields are missing.
251-268: LGTM: Thorough validation of national IDsThe
validatePersonsNationalIdsmethod provides comprehensive validation for the national IDs, ensuring they are different and belong to persons. The use ofBadRequestExceptionfor invalid cases is appropriate.
5-5: 🛠️ Refactor suggestionUpdate deprecated 'uuidv4' import
The
uuidv4package is deprecated. It's recommended to use theuuidpackage for better support and future maintenance.Apply this diff to update the import:
- import { uuid } from 'uuidv4' + import { v4 as uuid } from 'uuid'Likely invalid or redundant comment.
270-273: 🛠️ Refactor suggestionAdd error handling to indexing operations
While the
indexDelegationsmethod correctly indexes both custom and general mandate delegations, usingvoidto ignore the promises means any errors during indexing will be unhandled.Consider adding error handling to ensure indexing issues are properly managed:
private indexDelegations(nationalId: string) { - void this.delegationIndexService.indexCustomDelegations(nationalId) - void this.delegationIndexService.indexGeneralMandateDelegations(nationalId) + Promise.all([ + this.delegationIndexService.indexCustomDelegations(nationalId), + this.delegationIndexService.indexGeneralMandateDelegations(nationalId) + ]).catch(error => { + console.error('Failed to index delegations:', error); + // Consider whether to throw an error or handle it in another way + }); }This change ensures that any errors during indexing are logged and can be handled appropriately.
Likely invalid or redundant comment.
GunnlaugurG
added
automerge
* created new admin module for paper delegations and get route * field resolver DelegationAdminModel * lookup for delegations with detailed view * Cleanup and rest and graphql for DeleteDelegation * small cleanup * chore: nx format:write update dirty files * move delegationAdmin service to admin-api from delegation-api * adds getTicket function to Zendesk service Fetches Zendesk ticket by id * Adds create delegation route Form for creating delegation, wip form action * chore: nx format:write update dirty files * fix config value * chore: charts update dirty files * fix api build issues * wip gql for create delegation * fix pr comments * delegation reference id added * added back the spec files * validate form data and show error messages * fix get tests * chore: nx format:write update dirty files * test for delete * post method to create delegation between two national id's * zendesk integration complete * remove console log * merged with main * chore: charts update dirty files * chore: nx format:write update dirty files * adds CreateDelegationConfirm modal and prefills Create form with values from search params * chore: nx format:write update dirty files * use identity resolver fixes in response to comments * chore: nx format:write update dirty files * created delegation-delegation-type.model.ts and updated findAllScopesTo in delegation-scope.service.ts * fix broken tests * tests for findAllScopesTo * added validTo to delegationDelegationType * set general mandate as type in ids select account prompt * Get general mandate to delegations-to on service-portal * remove duplicate case * small refactor * chore: nx format:write update dirty files * Mask nationalId in url * format national id * fix tests after merge with main * chore: nx format:write update dirty files * fix duplicate referenceId's * fix import * remove console log and unused variables * chore: nx format:write update dirty files * move general mandate tests to new file * add zendesk validation * feat(auth-admin): Delete delegation UI (#16073) * Changes includeArchived from Param to Query Fixes openapi.yaml error * adds Oauth2 to openApi document builder * validates Zendesk ticket when creating delegation * Adds delete button to delegation access card and call delete mutation * chore: nx format:write update dirty files * add referenceId to delegation query --------- Co-authored-by: andes-it <builders@andes.is> * feat(auth-admin): Create paper delegation zendesk integration (#16074) * Changes includeArchived from Param to Query Fixes openapi.yaml error * adds Oauth2 to openApi document builder * validates Zendesk ticket when creating delegation * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <builders@andes.is> * connect changes and modify incoming delegations for new ddt table * fix comments from PR * fix pr comment * chore: nx format:write update dirty files * chore: nx format:write update dirty files * fix pr comment * add tests for create * fix pr comments * simplify var names * chore: nx format:write update dirty files * add index for general mandate * fix pr comments * fix pr comments --------- Co-authored-by: andes-it <builders@andes.is> Co-authored-by: Magnea Rún Vignisdóttir <magneavignisdottir@gmail.com> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Create endpoint for delegation between two national ids, created by the 3rd national id
Why
So agents can create a delegation between two national ids when a paper delegation is presented
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
referenceId, and improved data handling.DelegationListcomponent to support deletion of custom delegations.DelegationsIncomingandDelegationsOutgoingcomponents with direct links to delegation details.validityfor flexibility in fetching valid incoming delegations.Bug Fixes
Tests
Documentation