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(my-pages): health vaccination tags & locale #16567

Merged
merged 10 commits into from
Nov 11, 2024

Conversation

disaerna
Copy link
Member

@disaerna disaerna commented Oct 25, 2024

My pages - Health - Vaccination fixes

What

Fix colors for vaccination status tags
Add locale to get vaccination query

  • 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

Release Notes

  • New Features

    • Added support for locale-specific vaccination data retrieval with the introduction of a locale parameter in various API endpoints and methods.
    • Introduced a new enumeration for vaccination statuses to enhance data representation.
  • Bug Fixes

    • Improved error handling and data mapping in the vaccination data display components.
  • Refactor

    • Updated method signatures across multiple services and components to accommodate the new locale parameter.
    • Enhanced type safety in vaccination status handling.
  • Chores

    • Removed deprecated interfaces and constants related to vaccination data structure.

Copy link
Contributor

coderabbitai bot commented Oct 25, 2024

Walkthrough

The changes in this pull request primarily focus on enhancing the handling of vaccination data by incorporating locale-specific parameters across various components. The HealthDirectorateResolver and HealthDirectorateService classes have been updated to accept a locale argument in their getVaccinations methods. Additionally, a new VaccinationStatusEnum has been introduced to standardize vaccination status values. Various files, including GraphQL queries and client configurations, have been modified to support these updates, improving the overall functionality and localization capabilities of the vaccination-related features.

Changes

File Change Summary
libs/api/domains/health-directorate/src/lib/health-directorate.resolver.ts Updated getVaccinations method to include locale parameter.
libs/api/domains/health-directorate/src/lib/health-directorate.service.ts Updated getVaccinations method to include locale, modified vaccination status handling using mapVaccinationStatus.
libs/api/domains/health-directorate/src/lib/models/enums.ts Added new enum VaccinationStatusEnum for standardized vaccination statuses.
libs/api/domains/health-directorate/src/lib/models/vaccinations.model.ts Updated Vaccination class to use VaccinationStatusEnum for status field.
libs/api/domains/health-directorate/src/lib/utils/mappers.ts Added mapVaccinationStatus function to map statuses from DiseaseVaccinationDtoVaccinationStatusEnum to VaccinationStatusEnum.
libs/clients/health-directorate/src/lib/clients/vaccinations/clientConfig.json Added locale query parameter to multiple API endpoints.
libs/clients/health-directorate/src/lib/clients/vaccinations/vaccinations.service.ts Updated getVaccinationDiseaseDetail method to accept locale parameter and adjusted API call logic accordingly.
libs/service-portal/health/src/screens/Vaccinations/Vaccinations.graphql Modified getVaccinations query to accept $locale parameter.
libs/service-portal/health/src/screens/Vaccinations/VaccinationsWrapper.tsx Updated to use locale in useGetVaccinationsQuery.
libs/service-portal/health/src/screens/Vaccinations/dataStructure.ts Deleted file containing TypeScript interfaces and constants related to vaccinations.
libs/service-portal/health/src/screens/Vaccinations/tables/SortedVaccinationsTable.tsx Enhanced conditional checks and data mapping logic.
libs/service-portal/health/src/screens/Vaccinations/tables/VaccinationsTable.tsx Deleted VaccinationsTable component responsible for rendering vaccination data.
libs/service-portal/health/src/utils/tagSelector.ts Updated tagSelector function to use HealthDirectorateVaccinationStatusEnum and improved type safety.

Possibly related PRs

Suggested labels

automerge, high priority

Suggested reviewers

  • thorkellmani
  • rafnarnason

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.

Copy link

codecov bot commented Oct 25, 2024

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 36.54%. Comparing base (a6c30f5) to head (0d91654).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...c/lib/clients/vaccinations/vaccinations.service.ts 0.00% 5 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##             main   #16567    +/-   ##
========================================
  Coverage   36.53%   36.54%            
========================================
  Files        6892     6880    -12     
  Lines      143892   143241   -651     
  Branches    41014    40897   -117     
========================================
- Hits        52578    52342   -236     
+ Misses      91314    90899   -415     
Flag Coverage Δ
api 3.35% <ø> (ø)
application-system-api 41.08% <0.00%> (-0.01%) ⬇️
application-template-api-modules 27.60% <ø> (ø)
application-ui-shell 20.80% <ø> (ø)

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

Files with missing lines Coverage Δ
...c/lib/clients/vaccinations/vaccinations.service.ts 32.00% <0.00%> (-2.79%) ⬇️

... and 84 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 a6c30f5...0d91654. Read the comment docs.

@datadog-island-is
Copy link

datadog-island-is bot commented Oct 25, 2024

Datadog Report

All test runs 4079be9 🔗

5 Total Test Services: 0 Failed, 4 Passed
➡️ Test Sessions change in coverage: 13 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.4s 1 no change Link
application-system-api 0 0 0 120 2 3m 35.31s 1 no change Link
application-template-api-modules 0 0 0 123 0 2m 3.23s 1 no change Link
application-ui-shell 0 0 0 74 0 31.04s 1 no change Link
service-portal-health 0 0 0 0 0 485.96ms 1 no change Link

@disaerna disaerna added the deploy-feature Deploys features to dev label Oct 29, 2024
@disaerna disaerna marked this pull request as ready for review November 6, 2024 13:48
@disaerna disaerna requested review from a team as code owners November 6, 2024 13:48
@disaerna disaerna changed the title fix(my-pages): health vaccination tags fix(my-pages): health vaccination tags & locale Nov 6, 2024
@disaerna disaerna added the automerge Merge this PR as soon as all checks pass label Nov 6, 2024
Copy link
Member

@thorkellmani thorkellmani left a comment

Choose a reason for hiding this comment

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

COOL 😎

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: 1

🧹 Outside diff range and nitpick comments (15)
libs/service-portal/health/src/screens/Vaccinations/Vaccinations.graphql (1)

1-1: Consider adding parameter documentation.

To improve maintainability, consider adding a description for the locale parameter using GraphQL's built-in documentation syntax.

+# Locale parameter determines the language of vaccination details
 query getVaccinations($locale: String) {
🧰 Tools
🪛 eslint

[error] 1-1: Parsing error: Unexpected token getVaccinations

libs/api/domains/health-directorate/src/lib/models/enums.ts (1)

3-12: Consider splitting the enum based on distinct status concepts

The current enum mixes different types of statuses (validity, completion, documentation). This could lead to confusion about which values are mutually exclusive.

Consider splitting into separate enums:

export enum VaccinationValidityStatus {
  Valid = 'valid',
  Expired = 'expired'
}

export enum VaccinationCompletionStatus {
  Complete = 'complete',
  Incomplete = 'incomplete'
}

export enum VaccinationDocumentationStatus {
  Documented = 'documented',
  Undocumented = 'undocumented',
  Rejected = 'rejected',
  Undetermined = 'undetermined'
}
libs/api/domains/health-directorate/src/lib/utils/mappers.ts (1)

4-6: Consider adding documentation and enhancing type safety

To improve maintainability and type safety:

  1. Add JSDoc documentation explaining the mapping logic and why certain statuses map to others
  2. Consider using TypeScript's exhaustive type checking to ensure all enum cases are handled
+/**
+ * Maps health directorate vaccination status to internal vaccination status enum
+ * @param status - The vaccination status from the health directorate API
+ * @returns The corresponding internal vaccination status
+ */
 export const mapVaccinationStatus = (
   status?: DiseaseVaccinationDtoVaccinationStatusEnum,
 ): VaccinationStatusEnum => {
libs/api/domains/health-directorate/src/lib/models/vaccinations.model.ts (1)

59-60: LGTM! Good improvement in type safety.

The change from string to VaccinationStatusEnum enhances type safety and provides better IDE support. The nullable flag maintains backward compatibility.

Consider adding JSDoc comments to document the possible enum values for better developer experience:

+  /**
+   * The vaccination status.
+   * @see VaccinationStatusEnum for possible values
+   */
  @Field(() => VaccinationStatusEnum, { nullable: true })
  status?: VaccinationStatusEnum
libs/clients/health-directorate/src/lib/clients/vaccinations/vaccinations.service.ts (1)

33-35: Consider enhancing debug logs with additional context

While changing to debug level is appropriate, consider adding more context to help with debugging:

-      this.logger.debug('No vaccines returned', {
+      this.logger.debug('No vaccines returned from API', {
         category: LOG_CATEGORY,
+        auth: { hasAuth: !!auth },
       })

-      this.logger.debug('No vaccines diseases returned', {
+      this.logger.debug('No vaccine diseases returned from API', {
         category: LOG_CATEGORY,
+        locale,
+        auth: { hasAuth: !!auth },
       })

Also applies to: 56-58

libs/service-portal/health/src/screens/Vaccinations/VaccinationsWrapper.tsx (2)

19-23: Consider enhancing error handling with specific error types

While the error handling works, it could benefit from more specific error types to handle different scenarios (network errors, validation errors, etc.) differently.

Example enhancement:

interface VaccinationError {
  type: 'NETWORK_ERROR' | 'VALIDATION_ERROR' | 'NOT_FOUND';
  message: string;
}

// Usage in component
const handleError = (error: VaccinationError) => {
  switch (error.type) {
    case 'NETWORK_ERROR':
      return <Problem error={error} variant="network" />;
    // ... handle other cases
  }
};

Line range hint 26-27: Add explicit typing to filter callbacks

The filter callbacks could benefit from explicit typing to improve type safety and code maintainability.

- const general = vaccinations?.filter((x) => x.isFeatured)
- const other = vaccinations?.filter((x) => !x.isFeatured)
+ const general = vaccinations?.filter((x: { isFeatured: boolean }) => x.isFeatured)
+ const other = vaccinations?.filter((x: { isFeatured: boolean }) => !x.isFeatured)
libs/service-portal/health/src/screens/Vaccinations/tables/SortedVaccinationsTable.tsx (2)

37-37: Simplify the null check condition.

The optional chaining operator after the null check is redundant since we've already verified that data exists.

-  if (!data || data?.length === 0)
+  if (!data || data.length === 0)

Line range hint 53-101: Consider type narrowing for better type safety.

Since we've verified data exists before mapping, we can narrow its type for better TypeScript inference. Also, the explicit undefined in the tag selector is redundant as optional chaining already returns undefined when the value is null/undefined.

-          data.map((item, i) => ({
+          (data as HealthDirectorateVaccination[]).map((item, i) => ({
             id: item?.id ?? `${i}`,
             name: item?.name ?? '',
             vaccine: item?.name ?? '',
             date: formatDate(item?.lastVaccinationDate) ?? '',
             children: (
               // ... children content ...
             ),
             status: item?.statusName ?? '',
-            tag: tagSelector(item?.status ?? undefined),
+            tag: tagSelector(item?.status),
           })) ?? []
libs/api/domains/health-directorate/src/lib/health-directorate.service.ts (1)

80-83: Consider extracting locale conversion logic

The locale conversion logic is repeated across multiple methods. Consider extracting it into a shared utility function for better maintainability and consistency.

+ private convertLocale(locale: Locale): string {
+   return locale === 'is' ? 'is' : 'en'
+ }

  async getVaccinations(
    auth: Auth,
    locale: Locale,
  ): Promise<Vaccinations | null> {
    const data = await this.vaccinationApi.getVaccinationDiseaseDetail(
      auth,
-     locale === 'is' ? 'is' : 'en',
+     this.convertLocale(locale),
    )
libs/clients/health-directorate/src/lib/clients/vaccinations/clientConfig.json (1)

Line range hint 1-1000: Consider documenting the locale parameter impact.

While the implementation is correct, consider adding descriptions to the locale parameters to clarify their effect on the responses.

Apply this diff to enhance documentation:

           {
             "name": "locale",
             "required": false,
             "in": "query",
+            "description": "Language code for localized content. Affects disease names and descriptions in the response.",
             "schema": { "enum": ["en", "is"], "type": "string" }
           }
libs/service-portal/health/src/utils/tagSelector.ts (4)

12-17: Simplify switch cases by grouping statuses with the same return value

You can reduce code duplication and enhance readability by grouping cases that return the same TagVariant.

Apply this diff to group statuses returning 'blue':

   switch (status) {
-    case HealthDirectorateVaccinationStatusEnum.complete:
-      return 'blue'
-    case HealthDirectorateVaccinationStatusEnum.expired:
-      return 'blue'
-    case HealthDirectorateVaccinationStatusEnum.incomplete:
-      return 'blue'
+    case HealthDirectorateVaccinationStatusEnum.complete:
+    case HealthDirectorateVaccinationStatusEnum.expired:
+    case HealthDirectorateVaccinationStatusEnum.incomplete:
+      return 'blue'

18-23: Simplify switch cases by grouping statuses with the same return value

Similarly, group the statuses that return 'purple'.

Apply this diff to group these cases:

     case HealthDirectorateVaccinationStatusEnum.incomplete:
       return 'blue'
-    case HealthDirectorateVaccinationStatusEnum.rejected:
-      return 'purple'
-    case HealthDirectorateVaccinationStatusEnum.undetermined:
-      return 'purple'
-    case HealthDirectorateVaccinationStatusEnum.undocumented:
-      return 'purple'
+    case HealthDirectorateVaccinationStatusEnum.rejected:
+    case HealthDirectorateVaccinationStatusEnum.undetermined:
+    case HealthDirectorateVaccinationStatusEnum.undocumented:
+      return 'purple'

9-9: Consider removing the redundant undefined check

Since the switch statement's default case returns 'blue', the initial check for status being undefined may be unnecessary.

Apply this diff to simplify the function:

-  if (!isDefined(status)) return 'blue'

   switch (status) {
       ...
     default:
       return 'blue'

This would streamline the function, relying on the default case to handle undefined status.

Also applies to: 28-29


1-2: Avoid unnecessary import by replacing isDefined check

Importing isDefined from class-validator adds an extra dependency for a simple check. You can replace it with a direct comparison to undefined.

Apply this diff to remove the import and adjust the code:

-import { isDefined } from 'class-validator'

...

-  if (!isDefined(status)) return 'blue'
+  if (status === undefined) return 'blue'

This reduces dependencies and might improve tree-shaking and bundling.

Alternatively, as mentioned in the previous comment, consider removing this check entirely if it's redundant.

Also applies to: 9-9

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 94a84fb and c122bf2.

📒 Files selected for processing (13)
  • libs/api/domains/health-directorate/src/lib/health-directorate.resolver.ts (1 hunks)
  • libs/api/domains/health-directorate/src/lib/health-directorate.service.ts (2 hunks)
  • libs/api/domains/health-directorate/src/lib/models/enums.ts (1 hunks)
  • libs/api/domains/health-directorate/src/lib/models/vaccinations.model.ts (2 hunks)
  • libs/api/domains/health-directorate/src/lib/utils/mappers.ts (1 hunks)
  • libs/clients/health-directorate/src/lib/clients/vaccinations/clientConfig.json (3 hunks)
  • libs/clients/health-directorate/src/lib/clients/vaccinations/vaccinations.service.ts (3 hunks)
  • libs/service-portal/health/src/screens/Vaccinations/Vaccinations.graphql (1 hunks)
  • libs/service-portal/health/src/screens/Vaccinations/VaccinationsWrapper.tsx (1 hunks)
  • libs/service-portal/health/src/screens/Vaccinations/dataStructure.ts (0 hunks)
  • libs/service-portal/health/src/screens/Vaccinations/tables/SortedVaccinationsTable.tsx (3 hunks)
  • libs/service-portal/health/src/screens/Vaccinations/tables/VaccinationsTable.tsx (0 hunks)
  • libs/service-portal/health/src/utils/tagSelector.ts (1 hunks)
💤 Files with no reviewable changes (2)
  • libs/service-portal/health/src/screens/Vaccinations/dataStructure.ts
  • libs/service-portal/health/src/screens/Vaccinations/tables/VaccinationsTable.tsx
🧰 Additional context used
📓 Path-based instructions (11)
libs/api/domains/health-directorate/src/lib/health-directorate.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/health-directorate/src/lib/health-directorate.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/health-directorate/src/lib/models/enums.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/health-directorate/src/lib/models/vaccinations.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/api/domains/health-directorate/src/lib/utils/mappers.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/health-directorate/src/lib/clients/vaccinations/clientConfig.json (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/health-directorate/src/lib/clients/vaccinations/vaccinations.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/service-portal/health/src/screens/Vaccinations/Vaccinations.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/service-portal/health/src/screens/Vaccinations/VaccinationsWrapper.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/service-portal/health/src/screens/Vaccinations/tables/SortedVaccinationsTable.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/service-portal/health/src/utils/tagSelector.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."
🪛 eslint
libs/service-portal/health/src/screens/Vaccinations/Vaccinations.graphql

[error] 1-1: Parsing error: Unexpected token getVaccinations

🔇 Additional comments (17)
libs/service-portal/health/src/screens/Vaccinations/Vaccinations.graphql (1)

1-2: LGTM! Clean implementation of locale support.

The addition of the optional locale parameter and its integration with healthDirectorateVaccinations is well-structured and maintains backward compatibility.

🧰 Tools
🪛 eslint

[error] 1-1: Parsing error: Unexpected token getVaccinations

libs/api/domains/health-directorate/src/lib/models/enums.ts (1)

1-15: LGTM on technical implementation

The implementation follows TypeScript best practices:

  • Proper use of string enum
  • Correct export for reusability
  • Proper GraphQL registration
libs/api/domains/health-directorate/src/lib/utils/mappers.ts (2)

1-2: LGTM! Imports are well-structured

The imports are properly typed and follow best practices for tree-shaking by importing specific enum types.


4-27: LGTM! Well-structured mapping function

The implementation is type-safe, handles all cases comprehensively, and provides a sensible default fallback.

libs/api/domains/health-directorate/src/lib/models/vaccinations.model.ts (1)

1-1: Verify the necessity of maintaining two vaccination status enums.

The code imports both DiseaseVaccinationDtoVaccinationStatusEnum and VaccinationStatusEnum. Consider if both are necessary or if we can consolidate them to avoid confusion.

Let's check the relationship between these enums:

Also applies to: 3-3

libs/clients/health-directorate/src/lib/clients/vaccinations/vaccinations.service.ts (2)

6-6: LGTM: Import follows TypeScript guidelines

The addition of the locale enum import enhances type safety for the new locale-based functionality.


44-52: 🛠️ Refactor suggestion

Enhance locale parameter handling and validation

While the implementation works, consider these improvements for better robustness:

   public async getVaccinationDiseaseDetail(
     auth: Auth,
-    locale: string,
+    locale: 'is' | 'en',
   ): Promise<Array<DiseaseVaccinationDto> | null> {
+    const supportedLocales = {
+      is: MeVaccinationControllerGetVaccinationsForDiseasesLocaleEnum.Is,
+      en: MeVaccinationControllerGetVaccinationsForDiseasesLocaleEnum.En,
+    };
+
+    if (!supportedLocales[locale]) {
+      throw new Error(`Unsupported locale: ${locale}. Supported locales: ${Object.keys(supportedLocales)}`);
+    }
+
     const disease = await this.vaccinationsApiWithAuth(auth)
       .meVaccinationControllerGetVaccinationsForDiseases({
-        locale:
-          locale === 'is'
-            ? MeVaccinationControllerGetVaccinationsForDiseasesLocaleEnum.Is
-            : MeVaccinationControllerGetVaccinationsForDiseasesLocaleEnum.En,
+        locale: supportedLocales[locale],
       })

This refactor:

  1. Adds type safety with a union type for locale
  2. Validates supported locales explicitly
  3. Makes the code more maintainable if new locales are added
  4. Improves error messaging
libs/api/domains/health-directorate/src/lib/health-directorate.resolver.ts (1)

75-80: LGTM! Consistent locale handling implementation.

The implementation follows the established patterns in the resolver, with proper TypeScript types and GraphQL schema configuration. The default locale 'is' matches the convention used in other methods.

Let's verify the consistency of locale handling across the codebase:

✅ Verification successful

Locale handling is consistent and properly implemented

The verification shows that:

  • The default locale 'is' is consistently used across multiple methods in the resolver
  • The getVaccinations implementation follows the same pattern as other locale-aware methods
  • The service layer is properly integrated with the locale parameter
  • The implementation maintains consistency with other resolvers in the codebase that handle localization
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify consistent locale handling in GraphQL resolvers
# Expected: All resolver methods handling locale should follow similar pattern

# Search for GraphQL Query/Mutation methods with locale parameter
ast-grep --pattern 'class $_ {
  $$$
  @Query($_)
  $_($$$@Args("locale"$$$)$$$) {
    $$$
  }
}'

# Search for GraphQL Mutation methods with locale parameter
ast-grep --pattern 'class $_ {
  $$$
  @Mutation($_)
  $_($$$@Args("locale"$$$)$$$) {
    $$$
  }
}'

Length of output: 202


Script:

#!/bin/bash
# Let's try a different approach to verify locale handling

# Search for methods with locale parameter in resolver files
rg -U "@(Query|Mutation).*\n.*locale.*\n.*\{" --type ts

# Find all resolver files that might handle locale
fd resolver.ts$ --type f --exec grep -l "locale.*=.*'is'" {} \;

# Search specifically for getVaccinations method implementations
ast-grep --pattern 'getVaccinations($_) {
  $$$
}'

Length of output: 752


Script:

#!/bin/bash
# Let's examine the locale handling in health-directorate.resolver.ts more closely

# Show the content of health-directorate.resolver.ts with context
rg -A 2 -B 2 "@(Query|Mutation).*locale.*" ./libs/api/domains/health-directorate/src/lib/health-directorate.resolver.ts

# Look for other methods in the same file that might handle locale
rg "locale.*=.*'is'" ./libs/api/domains/health-directorate/src/lib/health-directorate.resolver.ts

# Check the API interface/service for locale handling
rg -A 1 "getVaccinations" ./libs/api/domains/health-directorate/src/lib/

Length of output: 1021

libs/service-portal/health/src/screens/Vaccinations/VaccinationsWrapper.tsx (1)

18-23: LGTM: Proper locale implementation

The locale handling is implemented correctly using the useLocale hook and properly passed to the GraphQL query.

libs/service-portal/health/src/screens/Vaccinations/tables/SortedVaccinationsTable.tsx (1)

Line range hint 1-106: LGTM for reusability and tree-shaking practices!

The component follows good practices:

  • Uses TypeScript for type safety
  • Properly imports from shared libraries
  • Follows tree-shakeable import patterns
  • Is reusable across different NextJS apps through the service-portal library
libs/api/domains/health-directorate/src/lib/health-directorate.service.ts (3)

13-14: LGTM: Clean import declarations

The imports are properly typed and specific, following good tree-shaking practices.


76-79: LGTM: Well-typed method signature

The method signature properly incorporates locale support with appropriate TypeScript types, maintaining consistency with other methods in the service.


95-95: Verify vaccination status mapping implementation

The code now uses a dedicated mapping function for vaccination status, which is good for maintainability. However, we should verify the implementation of this mapping function.

✅ Verification successful

Let me gather more information about the enum types used in the mapping function.


Let me search for the DTO enum definition using a different pattern.


Vaccination status mapping implementation is correct and comprehensive

The mapping function correctly handles all possible vaccination status values from the health directorate DTO to the internal enum. The implementation:

  • Maps all status variants: valid, complete, expired, incomplete, rejected, undetermined, undocumented, and unvaccinated
  • Provides a safe fallback to 'undetermined' for unknown values
  • Maintains consistent status values between the source and target enums
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for the mapping function implementation and its test coverage
echo "Searching for mapping function implementation:"
rg -A 10 "export const mapVaccinationStatus"

echo "\nSearching for related test files:"
fd "mappers.test" --exec rg -l "mapVaccinationStatus"

Length of output: 1470


Script:

#!/bin/bash
# Search for the enum definitions
echo "Searching for VaccinationStatusEnum definition:"
rg -A 10 "enum VaccinationStatusEnum"

echo "\nSearching for DiseaseVaccinationDtoVaccinationStatusEnum definition:"
rg -A 10 "enum DiseaseVaccinationDtoVaccinationStatusEnum"

# Search for test coverage
echo "\nSearching for test files with broader pattern:"
fd test --exec rg -l "mapVaccinationStatus|VaccinationStatusEnum"

Length of output: 1561


Script:

#!/bin/bash
# Search for the DTO enum in generated files
echo "Searching for DiseaseVaccinationDto definition:"
rg -A 15 "DiseaseVaccinationDto" --type ts

# Search for any test files in the health-directorate domain
echo "\nSearching for test files in health-directorate domain:"
fd test.ts$ -p libs/api/domains/health-directorate --exec rg -l "vaccination"

Length of output: 9259

libs/clients/health-directorate/src/lib/clients/vaccinations/clientConfig.json (3)

61-68: LGTM: Well-structured locale parameter definition.

The locale parameter is correctly defined with:

  • Optional status (required: false)
  • Proper enum values ["en", "is"]
  • Clear query parameter placement

130-135: Maintain consistency with the previous locale parameter definition.

The implementation matches the previous endpoint's locale parameter, ensuring a consistent API design.


893-896: Schema update aligns with API evolution best practices.

The addition of the vaccines array to UpdateDiseaseDto maintains proper type references and follows OpenAPI standards.

Let's verify the schema usage:

✅ Verification successful

Schema update maintains proper OpenAPI structure and type references

The verification confirms that:

  • The VaccineDiseaseDto schema is properly defined in the OpenAPI specification
  • The schema is correctly referenced in both DiseaseDto and UpdateDiseaseDto
  • The vaccines array property follows OpenAPI 3.0 standards with proper type definitions
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for UpdateDiseaseDto usage to ensure compatibility
rg -l "UpdateDiseaseDto" --type ts

Length of output: 34


Script:

#!/bin/bash
# Let's first check the actual schema definition in the JSON file
cat libs/clients/health-directorate/src/lib/clients/vaccinations/clientConfig.json

# Also search for any references to VaccineDiseaseDto
rg "VaccineDiseaseDto" --type ts --type json -A 3

Length of output: 30754

libs/service-portal/health/src/utils/tagSelector.ts (1)

6-8: Good use of TypeScript for function signature

The function tagSelector correctly uses TypeScript to define the parameter and return types, enhancing type safety.

@kodiakhq kodiakhq bot merged commit 129bd5f into main Nov 11, 2024
36 checks passed
@kodiakhq kodiakhq bot deleted the my-pages/vaccinations-tag-color branch November 11, 2024 10:42
jonnigs pushed a commit that referenced this pull request Nov 12, 2024
* fix: tags

* feat: add locale to service WIP

* feat: add locale from service

* feat: add locale to query call
disaerna added a commit that referenced this pull request Nov 14, 2024
* fix: tags

* feat: add locale to service WIP

* feat: add locale from service

* feat: add locale to query call
kodiakhq bot added a commit that referenced this pull request Nov 15, 2024
* fix(my-pages): health vaccination tags & locale (#16567)

* fix: tags

* feat: add locale to service WIP

* feat: add locale from service

* feat: add locale to query call

* fix(api): add ff to resolver (#16865)

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

* fix(api): add  correct scope (#16874)

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

---------

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
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 deploy-feature Deploys features to dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants