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

fix(passport): application cleanup #16614

Merged
merged 5 commits into from
Oct 29, 2024
Merged

fix(passport): application cleanup #16614

merged 5 commits into from
Oct 29, 2024

Conversation

albinagu
Copy link
Member

@albinagu albinagu commented Oct 29, 2024

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Formatting passes locally with my changes
  • I have rebased against main before asking for a review

Summary by CodeRabbit

  • New Features

    • Introduced getValueViaPath function for improved data retrieval across forms.
    • Added a GraphQL query for hasDisabilityLicense.
  • Bug Fixes

    • Corrected typos in error message keys related to disability records.
    • Enhanced error handling for missing chargeItemCode.
  • Improvements

    • Removed deprecated DeliveryAddressApi and DistrictsApi from the application.
    • Streamlined form structures by removing unnecessary fields and descriptions.
    • Updated title and description fields for clarity in various forms.
  • Documentation

    • Updated message identifiers for consistency and clarity.

@albinagu albinagu requested a review from a team as a code owner October 29, 2024 11:31
Copy link
Contributor

coderabbitai bot commented Oct 29, 2024

Walkthrough

The pull request introduces several changes across multiple files in the passport application. Key modifications include the removal of the DeliveryAddressApi, updates to various forms and components to improve data retrieval methods using getValueViaPath, and adjustments to error message identifiers. Additionally, several GraphQL queries have been deleted, and the structure of various forms has been streamlined by removing unnecessary fields and updating naming conventions for clarity.

Changes

File Path Change Summary
libs/application/templates/passport/src/dataProviders/index.ts Removed export for DeliveryAddressApi.
libs/application/templates/passport/src/fields/HasDisabilityLicenseMessage/index.tsx Added GraphQL query hasDisabilityLicense and corrected typos in message keys.
libs/application/templates/passport/src/fields/PassportSelection/index.tsx Refined error handling by separating user and child passport errors; simplified tag function signature.
libs/application/templates/passport/src/forms/Done.ts Updated logic to retrieve applicant's name using getValueViaPath; modified nextStepsTitle description.
libs/application/templates/passport/src/forms/Draft.ts Removed buildDescriptionField calls and updated titles in the serviceSection.
libs/application/templates/passport/src/forms/ParentB.ts Added getValueViaPath for accessing values; removed DeliveryAddressApi from data providers.
libs/application/templates/passport/src/forms/WaitingForParentBConfirmation.ts Utilized getValueViaPath for value retrieval in the description object.
libs/application/templates/passport/src/forms/infoSection/childsPersonalInfo.ts Removed unused Field import; renamed formatKennitala to formatNationalId; added description field.
libs/application/templates/passport/src/forms/infoSection/personalInfo.ts Updated import statements; replaced formatKennitala with formatNationalId; modified condition logic.
libs/application/templates/passport/src/forms/overviewSection/childsOverview.ts Updated imports to include getValueViaPath; renamed fields for clarity; improved data access methods.
libs/application/templates/passport/src/forms/overviewSection/personalOverview.ts Removed several description fields and dividers to streamline the overview section.
libs/application/templates/passport/src/lib/PassportTemplate.ts Removed DistrictsApi and DeliveryAddressApi from state machine configuration; updated error handling.
libs/application/templates/passport/src/lib/constants.ts Removed constants and types, including YES, NO, and others to reduce complexity.
libs/application/templates/passport/src/lib/dataSchema.ts Removed dropLocation field from the service object in the data schema.
libs/application/templates/passport/src/lib/error.ts Changed prefixes of error message IDs from pp.application to pa.application.
libs/application/templates/passport/src/lib/messages.ts Updated message IDs, corrected typos, added new message for child, and removed obsolete entries.
libs/application/templates/passport/src/lib/queries.ts Deleted file containing several GraphQL query definitions.

Suggested reviewers

  • rafnarnason
  • thorkellmani

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@datadog-island-is
Copy link

datadog-island-is bot commented Oct 29, 2024

Datadog Report

All test runs dcbaefd 🔗

4 Total Test Services: 0 Failed, 4 Passed
➡️ Test Sessions change in coverage: 8 no change

Test Services
Service Name Failed Known Flaky New Flaky Passed Skipped Total Time Code Coverage Change Test Service View
api 0 0 0 4 0 2.34s 1 no change Link
application-system-api 0 0 0 120 2 3m 10.74s 1 no change Link
application-template-api-modules 0 0 0 123 0 2m 27.09s 1 no change Link
application-ui-shell 0 0 0 74 0 34.39s 1 no change Link

Copy link

codecov bot commented Oct 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.76%. Comparing base (dc950b2) to head (5bac2ef).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16614      +/-   ##
==========================================
- Coverage   36.78%   36.76%   -0.03%     
==========================================
  Files        6855     6858       +3     
  Lines      142310   142340      +30     
  Branches    40579    40599      +20     
==========================================
- Hits        52348    52330      -18     
- Misses      89962    90010      +48     
Flag Coverage Δ
api 3.37% <ø> (ø)
application-system-api 41.32% <ø> (ø)
application-template-api-modules 27.78% <ø> (ø)
application-ui-shell 21.22% <ø> (ø)
judicial-system-web 27.73% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 19 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc950b2...5bac2ef. Read the comment docs.

@albinagu albinagu added the automerge Merge this PR as soon as all checks pass label Oct 29, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (12)
libs/application/templates/passport/src/forms/Done.ts (1)

23-24: Consider using constant paths for type safety.

While the use of getValueViaPath improves robustness when accessing nested properties, consider defining the path strings as constants to ensure type safety and easier maintenance.

Consider creating a constants file for paths:

// paths.ts
export const APPLICATION_PATHS = {
  CHILDS_NAME: 'childsPersonalInfo.name',
  PERSONAL_NAME: 'personalInfo.name'
} as const

Then use it in the form:

-getValueViaPath(application.answers, 'childsPersonalInfo.name') ??
-getValueViaPath(application.answers, 'personalInfo.name'),
+getValueViaPath(application.answers, APPLICATION_PATHS.CHILDS_NAME) ??
+getValueViaPath(application.answers, APPLICATION_PATHS.PERSONAL_NAME),
libs/application/templates/passport/src/fields/HasDisabilityLicenseMessage/index.tsx (2)

Line range hint 28-37: Improve hooks implementation and error handling.

The current implementation has several potential issues:

  1. Missing dependency array for setValue useEffect
  2. No cleanup for refetch subscription
  3. Missing error handling for setValue
 useEffect(() => {
   if (data && !!data.hasDisabilityLicense) {
-    setValue('personalInfo.hasDisabilityLicense', data.hasDisabilityLicense)
+    setValue('personalInfo.hasDisabilityLicense', data.hasDisabilityLicense, {
+      shouldValidate: true,
+    }).catch(console.error)
   }
-}, [data])
+}, [data, setValue])

-useEffect(() => {
-  refetch()
-}, [])
+useEffect(() => {
+  const subscription = refetch()
+  return () => {
+    subscription.then((sub) => sub?.stopPolling?.())
+  }
+}, [refetch])

Line range hint 1-9: Enhance TypeScript usage and component reusability.

While the component follows most coding guidelines for libs/**/* pattern, consider:

  1. Adding explicit return type for the component
  2. Documenting props interface
  3. Exporting types for reuse
+interface HasDisabilityLicenseMessageProps extends FieldBaseProps {
+  // Add any additional props here
+}
+
-export const HasDisabilityLicenseMessage: FC<
-  React.PropsWithChildren<FieldBaseProps>
-> = () => {
+export const HasDisabilityLicenseMessage: FC<HasDisabilityLicenseMessageProps> = (): JSX.Element => {
libs/application/templates/passport/src/forms/overviewSection/personalOverview.ts (1)

Line range hint 31-37: Improve type safety for personalInfo access.

Consider creating a dedicated type for the personalInfo structure instead of using inline type assertions.

interface PersonalInfo {
  name?: string;
  nationalId?: string;
  email?: string;
  phoneNumber?: string;
}

// Then use it like:
value: (application: Application) =>
  (application.answers.personalInfo as PersonalInfo)?.name,
libs/application/templates/passport/src/forms/infoSection/childsPersonalInfo.ts (2)

20-25: Consider adding a description for better context.

While the title addition improves form structure, consider adding a description field to provide more context about what information is needed for the child.

 buildDescriptionField({
   id: 'childsPersonalInfo.child',
   title: m.child,
   titleVariant: 'h3',
   marginBottom: 'smallGutter',
+  description: m.childDescription,
 }),

Line range hint 1-180: Architecture aligns well with library requirements.

The form configuration:

  • Uses reusable building blocks from @island.is/application/core
  • Maintains proper TypeScript types throughout
  • Follows tree-shaking best practices with named exports and minimal imports
libs/application/templates/passport/src/fields/PassportSelection/index.tsx (4)

Line range hint 51-94: Consider extracting date logic and constants

The tag function contains complex date manipulation logic that could be made more maintainable.

Consider applying these improvements:

+ const PASSPORT_EXPIRY_THRESHOLD_MONTHS = 6;
+ 
+ const isPassportExpired = (expirationDate: Date): boolean => {
+   return new Date() > expirationDate;
+ };
+ 
+ const isPassportExpiringWithinMonths = (expirationDate: Date, months: number): boolean => {
+   const futureDate = new Date();
+   futureDate.setMonth(futureDate.getMonth() + months);
+   return futureDate > expirationDate;
+ };
+
 const tag = (identityDocument: IdentityDocument): TagCheck => {
   const today = new Date()
   const expirationDate = new Date(identityDocument?.expirationDate)
-  const todayPlus6Months = new Date(
-    new Date().setMonth(new Date().getMonth() + 6),
-  )
+  const isExpiring = isPassportExpiringWithinMonths(expirationDate, PASSPORT_EXPIRY_THRESHOLD_MONTHS);
   // ... rest of the function using the new helpers
 }

Line range hint 107-132: Enhance type safety for user passport radio options

The component could benefit from stronger typing for the national registry data.

Consider these TypeScript improvements:

+ interface NationalRegistryData {
+   fullName: string;
+ }
+
 // In the options array
 options: [
   {
     label: (application.externalData.nationalRegistry
-      .data as any).fullName,
+      .data as NationalRegistryData).fullName,
     // ... rest of the option
   }
 ]

Line range hint 146-174: Simplify child passport options mapping

The nested ternary expressions in the options mapping could be simplified for better readability.

Consider this refactor:

- options: identityDocumentData.childPassports.map(
-   (child: IdentityDocumentChild) => {
-     return {
-       label: child.childName,
-       value: child.childNationalId,
-       subLabel: child.passports?.length
-         ? formatMessage(m.passportNumber) +
-           ' ' +
-           child.passports[0].subType +
-           child.passports[0].number
-         : '',
-       tag: child.passports
-         ? tag(child.passports?.[0]).tag
-         : undefined,
-       disabled: child.passports
-         ? tag(child.passports?.[0]).isDisabled
-         : false,
-     }
-   },
- ),
+ options: identityDocumentData.childPassports.map((child: IdentityDocumentChild) => {
+   const passport = child.passports?.[0];
+   const tagResult = passport ? tag(passport) : null;
+   
+   return {
+     label: child.childName,
+     value: child.childNationalId,
+     subLabel: passport
+       ? `${formatMessage(m.passportNumber)} ${passport.subType}${passport.number}`
+       : '',
+     tag: tagResult?.tag,
+     disabled: tagResult?.isDisabled ?? false,
+   };
+ }),

Line range hint 1-1: Consider exporting component types

To improve reusability across different NextJS apps, consider exporting the component's types.

Add type exports at the top of the file:

+ export interface PassportSelectionProps extends FieldBaseProps {
+   // Add any specific props here
+ }
+
- export const PassportSelection: FC<React.PropsWithChildren<FieldBaseProps>> = ({
+ export const PassportSelection: FC<React.PropsWithChildren<PassportSelectionProps>> = ({
libs/application/templates/passport/src/forms/overviewSection/childsOverview.ts (2)

33-33: LGTM! Consistent use of getValueViaPath improves type safety.

The refactoring enhances null-safety and follows the library's best practices for data access.

Consider creating a helper function to reduce repetition:

const getGuardianField = (answers: any, guardianNumber: 1 | 2, field: string) =>
  getValueViaPath(answers, `childsPersonalInfo.guardian${guardianNumber}.${field}`);

This would simplify the repeated patterns like:

value: (application: Application) => getGuardianField(application.answers, 1, 'name')

Also applies to: 39-43, 63-66, 72-75, 86-89, 95-98, 121-124, 134-138, 154-157, 165-168


184-192: Consider improving type safety in the service type check.

While the conditional logic is clean, the type assertion as Service could be replaced with proper type narrowing.

Consider this safer approach:

description: (application: Application) => {
  const serviceType = application.answers.service?.type
  return serviceType === Services.REGULAR
    ? m.serviceTypeRegular
    : m.serviceTypeExpress
}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between cc6778a and ee1a95e.

📒 Files selected for processing (17)
  • libs/application/templates/passport/src/dataProviders/index.ts (0 hunks)
  • libs/application/templates/passport/src/fields/HasDisabilityLicenseMessage/index.tsx (2 hunks)
  • libs/application/templates/passport/src/fields/PassportSelection/index.tsx (5 hunks)
  • libs/application/templates/passport/src/forms/Done.ts (2 hunks)
  • libs/application/templates/passport/src/forms/Draft.ts (1 hunks)
  • libs/application/templates/passport/src/forms/ParentB.ts (2 hunks)
  • libs/application/templates/passport/src/forms/WaitingForParentBConfirmation.ts (2 hunks)
  • libs/application/templates/passport/src/forms/infoSection/childsPersonalInfo.ts (3 hunks)
  • libs/application/templates/passport/src/forms/infoSection/personalInfo.ts (3 hunks)
  • libs/application/templates/passport/src/forms/overviewSection/childsOverview.ts (6 hunks)
  • libs/application/templates/passport/src/forms/overviewSection/personalOverview.ts (1 hunks)
  • libs/application/templates/passport/src/lib/PassportTemplate.ts (0 hunks)
  • libs/application/templates/passport/src/lib/constants.ts (0 hunks)
  • libs/application/templates/passport/src/lib/dataSchema.ts (0 hunks)
  • libs/application/templates/passport/src/lib/error.ts (1 hunks)
  • libs/application/templates/passport/src/lib/messages.ts (3 hunks)
  • libs/application/templates/passport/src/lib/queries.ts (0 hunks)
💤 Files with no reviewable changes (5)
  • libs/application/templates/passport/src/dataProviders/index.ts
  • libs/application/templates/passport/src/lib/PassportTemplate.ts
  • libs/application/templates/passport/src/lib/constants.ts
  • libs/application/templates/passport/src/lib/dataSchema.ts
  • libs/application/templates/passport/src/lib/queries.ts
🧰 Additional context used
📓 Path-based instructions (12)
libs/application/templates/passport/src/fields/HasDisabilityLicenseMessage/index.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/application/templates/passport/src/fields/PassportSelection/index.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/application/templates/passport/src/forms/Done.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/application/templates/passport/src/forms/Draft.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/application/templates/passport/src/forms/ParentB.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/application/templates/passport/src/forms/WaitingForParentBConfirmation.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/application/templates/passport/src/forms/infoSection/childsPersonalInfo.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/application/templates/passport/src/forms/infoSection/personalInfo.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/application/templates/passport/src/forms/overviewSection/childsOverview.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/application/templates/passport/src/forms/overviewSection/personalOverview.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/application/templates/passport/src/lib/error.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/application/templates/passport/src/lib/messages.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 (23)
libs/application/templates/passport/src/lib/error.ts (2)

Line range hint 1-19: LGTM! Well-structured error messages

The code follows best practices:

  • Properly uses TypeScript with react-intl's defineMessages
  • Each message includes clear defaultMessage and description
  • Maintains consistent structure for internationalization

5-5: Verify message ID references across the application

The prefix change from 'pp.application' to 'pa.application' needs to be consistently applied across all translation files and message references.

Also applies to: 10-10, 15-15

✅ Verification successful

Message ID prefix change is consistent and correct

The search results confirm that:

  1. There are no remaining 'pp.application' prefixes in the passport application codebase
  2. The new 'pa.application' prefix is consistently used across the passport application files
  3. Other applications use their own distinct prefixes (pdpp.application, fspp.application) and are not affected by this change
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining 'pp.application' references that might need updating
rg "pp\.application" --type ts --type tsx

# Search for new 'pa.application' pattern to verify consistency
rg "pa\.application" --type ts --type tsx

Length of output: 143


Script:

#!/bin/bash
# Search for any remaining 'pp.application' references using ripgrep without file type restrictions
rg "pp\.application"

# Search for new 'pa.application' pattern to verify consistency
rg "pa\.application"

# Additionally, let's check for any translation files that might contain these patterns
fd -e json -e ts -e tsx . | xargs rg "pp\.application|pa\.application"

Length of output: 100672

libs/application/templates/passport/src/forms/WaitingForParentBConfirmation.ts (3)

Line range hint 1-7: LGTM! Import changes look good.

The addition of getValueViaPath as a named import supports effective tree-shaking and follows the module import best practices.


Line range hint 1-48: LGTM! Code meets library guidelines.

The implementation adheres to all library requirements:

  • ✓ Component reusability across NextJS apps
  • ✓ Proper TypeScript usage
  • ✓ Effective tree-shaking practices

21-28: Verify type safety of path access.

The implementation of getValueViaPath improves the access of nested properties and aligns with the application cleanup objectives. However, let's verify the type safety of the accessed paths.

✅ Verification successful

Let me generate one more verification to check the implementation of getValueViaPath and its type safety:


Let me try one final verification with a different approach:


Type assertions are safe and consistent with codebase usage

Based on the verification results:

  • The getValueViaPath function is properly typed with generics <T = unknown>, allowing for type-safe assertions
  • The paths 'childsPersonalInfo.name' and 'childsPersonalInfo.guardian2.name' are consistently used across multiple files in the codebase
  • The type assertions to string are safe as these fields are used as string values in other parts of the application, particularly in the forms and email generators
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the type definitions for childsPersonalInfo
# to ensure type assertions are safe

# Search for type definitions of childsPersonalInfo
ast-grep --pattern 'type $interface {
  $$$
  childsPersonalInfo: {
    $$$
    name: string;
    $$$
    guardian2: {
      $$$
      name: string;
      $$$
    }
    $$$
  }
  $$$
}'

# Search for other usages of these paths to ensure consistency
rg -A 2 "childsPersonalInfo\.name|childsPersonalInfo\.guardian2\.name"

Length of output: 3753


Script:

#!/bin/bash
# Check the implementation of getValueViaPath and its type definitions
ast-grep --pattern 'function getValueViaPath($$$)'

# Check for type definitions of application.answers
ast-grep --pattern 'interface $_ {
  $$$
  answers: {
    $$$
  }
  $$$
}'

Length of output: 129


Script:

#!/bin/bash
# Search for getValueViaPath implementation with different pattern
rg -A 5 "export .* getValueViaPath"

# Search for type definitions in a different way
rg -A 5 "type.*Application.*=.*{" 
rg -A 5 "interface.*Application.*{" 

Length of output: 829

libs/application/templates/passport/src/forms/Done.ts (1)

Line range hint 1-8: LGTM! Import changes look good.

The addition of getValueViaPath import is properly structured for effective tree-shaking, which aligns with the coding guidelines.

libs/application/templates/passport/src/fields/HasDisabilityLicenseMessage/index.tsx (1)

Line range hint 47-62: LGTM! Well-structured error and loading state handling.

The implementation properly handles different states (error, success, loading) with appropriate UI feedback and message formatting.

libs/application/templates/passport/src/forms/overviewSection/personalOverview.ts (2)

Line range hint 1-116: Well-structured form configuration with clean implementation.

The changes align well with the PR objectives of application cleanup:

  1. Removal of redundant spacing fields improves maintainability
  2. Proper use of the application core building blocks
  3. Type-safe implementation with proper imports
  4. Reusable configuration that can be shared across different NextJS applications

27-27: Consider accessibility implications of empty title.

An empty title field might affect screen readers and accessibility. If the title is intentionally empty for visual spacing, consider using CSS margins or padding instead.

libs/application/templates/passport/src/forms/infoSection/personalInfo.ts (2)

2-2: LGTM! Import changes improve clarity and maintainability.

The changes follow best practices by:

  • Using specific named imports to support tree-shaking
  • Utilizing shared libraries for better reusability
  • Renaming the format import to a more descriptive name

Also applies to: 8-8, 12-13


49-49: LGTM! Consistent use of renamed formatter.

The change maintains proper null-safety while using the renamed formatter function.

libs/application/templates/passport/src/forms/ParentB.ts (3)

Line range hint 1-19: LGTM! Import changes and type definitions look good.

The code follows the coding guidelines for libs directory with proper TypeScript usage and maintains reusability across NextJS apps.


Line range hint 25-124: LGTM! Form structure follows best practices.

The form implementation:

  • Maintains clear separation of concerns with distinct sections
  • Uses proper type definitions for form components
  • Follows the template pattern for reusability

Line range hint 71-91: Verify the removal of DeliveryAddressApi impact.

The DeliveryAddressApi has been removed from the data providers. Let's verify there are no remaining dependencies.

✅ Verification successful

DeliveryAddressApi removal is safe in passport template

The removal of DeliveryAddressApi from the passport template's data providers is safe as there are no remaining references to it within the passport template directory. While DeliveryAddressApi is still used in other templates (like id-card), its removal from this specific template's data providers doesn't create any issues since it's properly scoped to the template's requirements.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining references to DeliveryAddressApi
rg "DeliveryAddressApi" --type ts

Length of output: 869


Script:

#!/bin/bash
# Let's check if there are any imports or references to DeliveryAddressApi in the passport template
rg "DeliveryAddressApi" libs/application/templates/passport/ --type ts -A 2

Length of output: 75

libs/application/templates/passport/src/forms/infoSection/childsPersonalInfo.ts (1)

7-9: LGTM! Import changes improve clarity and remove unused types.

The renaming of formatKennitala to formatNationalId enhances code readability while maintaining the same functionality. The removal of unused Field type import helps with tree-shaking.

libs/application/templates/passport/src/fields/PassportSelection/index.tsx (1)

40-41: LGTM: Improved error handling separation

The separation of error handling for user and child passports improves code clarity and maintainability.

libs/application/templates/passport/src/forms/Draft.ts (2)

Line range hint 1-20: LGTM! Code follows library coding guidelines.

The implementation adheres to the coding guidelines for library code:

  • Uses shared, reusable builder functions from core packages
  • Properly leverages TypeScript for type safety
  • Follows effective tree-shaking practices with named imports

Line range hint 1-190: Verify the impact of removed description fields.

The AI summary mentions removal of buildDescriptionField calls, but we should verify if this impacts the user experience or accessibility of the form.

#!/bin/bash
# Description: Check for any references to the removed description fields
# and verify if there are compensating descriptions elsewhere

echo "Checking for any references to removed description fields..."
ast-grep --pattern 'buildDescriptionField({
  $$$
  title: m.serviceTitle,
  $$$
})'

echo "Verifying if descriptions are provided through other means..."
rg -g '*.ts' 'serviceTypeDescription'
libs/application/templates/passport/src/forms/overviewSection/childsOverview.ts (2)

6-6: LGTM! Import changes improve code clarity and maintainability.

The changes follow TypeScript best practices and support effective tree-shaking. The rename from formatKennitala to formatNationalId improves code clarity.

Also applies to: 12-12


23-24: LGTM! Field naming improvements enhance code readability.

The changes from infoTitle to childTitle and m.child make the purpose of these fields more explicit and self-documenting.

libs/application/templates/passport/src/lib/messages.ts (3)

173-193: LGTM! Fixed typos and added new message

The changes improve code quality by:

  1. Fixing typos in disability-related message variables
  2. Adding a well-structured 'child' message entry

Line range hint 1-3: LGTM! Follows library coding guidelines

The code adheres to the library guidelines:

  • ✓ Reusable message definitions
  • ✓ TypeScript integration via react-intl
  • ✓ Proper tree-shaking support

Line range hint 60-71: Verify message ID changes across the codebase

The message IDs have been updated from 'cr.application' to 'pa.application'. Let's ensure all references are updated accordingly.

✅ Verification successful

Message ID prefix changes are correctly implemented

The verification shows that:

  1. The old cr.application prefix appears only in the criminal record and car recycling templates, where it's intentionally used.
  2. The new pa.application:dataCollection prefix is consistently used throughout the passport template messages.

The changes are isolated to their respective application templates and follow the correct naming conventions for each application type:

  • pa.application for passport
  • cr.application for criminal record
  • rf.cr.application for car recycling
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for any remaining occurrences of the old prefix
# to ensure complete migration.

echo "Checking for old message ID prefix..."
rg "cr\.application" --type ts --type tsx

echo "Verifying new message ID usage..."
rg "pa\.application:dataCollection\.dataCollectionUserProfileTitle|pa\.application:dataCollection\.dataCollectionIdentityDocumentTitle" --type ts --type tsx

Length of output: 420


Script:

#!/bin/bash
# Let's try again without the tsx extension and with better context

echo "Checking for old message ID prefix..."
rg "cr\.application" -t ts

echo "Checking new message ID usage..."
rg "pa\.application:dataCollection" -t ts -C 2

Length of output: 16308

@kodiakhq kodiakhq bot merged commit 1d3c50e into main Oct 29, 2024
39 checks passed
@kodiakhq kodiakhq bot deleted the passport_cleanup branch October 29, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this PR as soon as all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants