Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat(clients-national-registry-v3-applications): Add new client #17807

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

kksteini
Copy link
Member

@kksteini kksteini commented Feb 4, 2025

The National Registrar has created a new service in X-Road called MidlunUmsoknir-v1.
This client is very similar to clients-national-registry-v3.

On naming

I've come up with national-registry-v3-applications, partly for a lack of imagination and partly because this isn't really a version 4 but a separate client. Feel free to advise on the name, however, since it is rather long.

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 a new client module for National Registry V3, enabling flexible data retrieval via both simulated and live endpoints.
    • Added new service methods for retrieving specific data from the National Registry API.
  • Documentation
    • Added user-facing documentation with instructions for running tests and generating API code.
  • Tests
    • Implemented robust test configurations to ensure reliable functionality.
  • Chores
    • Updated project configurations and ownership assignments to streamline development and maintenance.

Copy link
Contributor

coderabbitai bot commented Feb 4, 2025

Walkthrough

This pull request introduces a new National Registry V3 Applications client library. It adds a Codeowners entry to assign ownership to a designated team and incorporates various configuration files (ESLint, Jest, Nx project, and TypeScript) along with source code files for API configuration, module definition, service implementations, and provider setup. Additionally, it updates the TypeScript base configuration with a new path mapping for the client library.

Changes

Files Change Summary
.github/CODEOWNERS Added new ownership entry for /libs/clients/national-registry/v3-applications/ assigned to @island-is/juni.
libs/clients/national-registry/v3-applications/{.eslintrc.json, README.md, jest.config.ts, project.json, tsconfig.json, tsconfig.lib.json, tsconfig.spec.json} Introduced new project-level configuration and documentation files including linting, testing, Nx project setup, and TypeScript configuration.
libs/clients/national-registry/v3-applications/src/{clientConfig.json, index.ts, lib/apiConfig.ts, lib/nationalRegistryV3Applications.config.ts, lib/nationalRegistryV3Applications.module.ts, lib/nationalRegistryV3Applications.service.ts, lib/providers.ts} Implemented source code files for API specification, client configuration, module declaration, service (including conditional fake data retrieval), and provider registration.
tsconfig.base.json Added new path mapping for @island.is/clients/national-registry-v3-applications pointing to the client library's main entry point.

Possibly related PRs

  • chore(clients-cms): Define code owners for cms client #16894: The changes in the main PR and the retrieved PR are related as both involve adding new entries to the .github/CODEOWNERS file, specifically assigning ownership for different directories to the same team @island-is/juni.
  • chore(University-Gateway): Change ownership of UG queries #16634: The changes in the main PR, which add a new ownership entry in the .github/CODEOWNERS file, are related to the changes in the retrieved PR that also modify the .github/CODEOWNERS file by adding a new ownership entry and redistributing responsibilities.
  • chore(system-e2e): Move system test ownership to relevant teams #16479: The changes in the main PR, which add a new ownership entry in the .github/CODEOWNERS file for a specific directory, are related to the changes in the retrieved PR that also modify the .github/CODEOWNERS file to update ownership assignments for various project directories, including specific subdirectories.

Suggested labels

automerge, high priority

Suggested reviewers

  • lodmfjord
  • thorkellmani
  • eirikurn

Tip

🌐 Web search-backed reviews and chat
  • We have enabled web search-based reviews and chat for all users. This feature allows CodeRabbit to access the latest documentation and information on the web.
  • You can disable this feature by setting web_search: false in the knowledge_base settings.
  • Please share any feedback in the Discord discussion.

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 656a6f1 and 656fb9a.

📒 Files selected for processing (1)
  • libs/clients/national-registry/v3-applications/src/lib/nationalRegistryV3Applications.service.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/clients/national-registry/v3-applications/src/lib/nationalRegistryV3Applications.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."
📓 Learnings (1)
libs/clients/national-registry/v3-applications/src/lib/nationalRegistryV3Applications.service.ts (2)
Learnt from: pro-ingvar
PR: island-is/island.is#17807
File: libs/clients/national-registry/v3-applications/src/lib/nationalRegistryV3Applications.service.ts:12-20
Timestamp: 2025-02-04T11:57:37.165Z
Learning: Test data should be kept in dedicated test files (*.spec.ts, *.test.ts) rather than in service files to maintain separation between production and test code.
Learnt from: pro-ingvar
PR: island-is/island.is#17807
File: libs/clients/national-registry/v3-applications/src/lib/nationalRegistryV3Applications.service.ts:12-20
Timestamp: 2025-02-04T11:57:37.165Z
Learning: Test data should be kept in dedicated test files (*.spec.ts, *.test.ts) rather than in service files to maintain separation between production and test code, following the established convention in the codebase.
🔇 Additional comments (1)
libs/clients/national-registry/v3-applications/src/lib/nationalRegistryV3Applications.service.ts (1)

1-7: LGTM! Well-structured service class.

The implementation follows NestJS best practices with proper dependency injection and TypeScript usage.

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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 generate docstrings to generate docstrings for this PR. (Beta)
  • @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.

Comment on lines +20 to +24
clientId: env.required(
'NATIONAL_REGISTRY_B2C_CLIENT_ID',
'b464afdd-056b-406d-b650-6d41733cfeb7',
),
clientSecret: env.required('NATIONAL_REGISTRY_B2C_CLIENT_SECRET', ''),
Copy link
Member Author

Choose a reason for hiding this comment

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

According to the api provider, the clientId and secret are unaltered for this service.

@kksteini kksteini marked this pull request as ready for review February 4, 2025 11:21
@kksteini kksteini requested review from a team as code owners February 4, 2025 11:21
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: 3

🧹 Nitpick comments (1)
libs/clients/national-registry/v3-applications/src/lib/nationalRegistryV3Applications.config.ts (1)

16-38: Remove extra space in the name property.

There's an extra space at the beginning of the name property.

Apply this diff to fix the name property:

-  name: ' NationalRegistryV3ApplicationsClient',
+  name: 'NationalRegistryV3ApplicationsClient',
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ba97e5 and a5141f3.

📒 Files selected for processing (16)
  • .github/CODEOWNERS (1 hunks)
  • libs/clients/national-registry/v3-applications/.eslintrc.json (1 hunks)
  • libs/clients/national-registry/v3-applications/README.md (1 hunks)
  • libs/clients/national-registry/v3-applications/jest.config.ts (1 hunks)
  • libs/clients/national-registry/v3-applications/project.json (1 hunks)
  • libs/clients/national-registry/v3-applications/src/clientConfig.json (1 hunks)
  • libs/clients/national-registry/v3-applications/src/index.ts (1 hunks)
  • libs/clients/national-registry/v3-applications/src/lib/apiConfig.ts (1 hunks)
  • libs/clients/national-registry/v3-applications/src/lib/nationalRegistryV3Applications.config.ts (1 hunks)
  • libs/clients/national-registry/v3-applications/src/lib/nationalRegistryV3Applications.module.ts (1 hunks)
  • libs/clients/national-registry/v3-applications/src/lib/nationalRegistryV3Applications.service.ts (1 hunks)
  • libs/clients/national-registry/v3-applications/src/lib/providers.ts (1 hunks)
  • libs/clients/national-registry/v3-applications/tsconfig.json (1 hunks)
  • libs/clients/national-registry/v3-applications/tsconfig.lib.json (1 hunks)
  • libs/clients/national-registry/v3-applications/tsconfig.spec.json (1 hunks)
  • tsconfig.base.json (1 hunks)
✅ Files skipped from review due to trivial changes (5)
  • libs/clients/national-registry/v3-applications/README.md
  • libs/clients/national-registry/v3-applications/tsconfig.lib.json
  • libs/clients/national-registry/v3-applications/tsconfig.json
  • libs/clients/national-registry/v3-applications/jest.config.ts
  • libs/clients/national-registry/v3-applications/.eslintrc.json
🧰 Additional context used
📓 Path-based instructions (9)
libs/clients/national-registry/v3-applications/tsconfig.spec.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/national-registry/v3-applications/src/lib/apiConfig.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/national-registry/v3-applications/src/lib/nationalRegistryV3Applications.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/clients/national-registry/v3-applications/src/lib/providers.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/national-registry/v3-applications/src/lib/nationalRegistryV3Applications.config.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/national-registry/v3-applications/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/clients/national-registry/v3-applications/project.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/national-registry/v3-applications/src/lib/nationalRegistryV3Applications.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/clients/national-registry/v3-applications/src/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."
🪛 Biome (1.9.4)
libs/clients/national-registry/v3-applications/src/index.ts

[error] 3-3: Useless rename.

Safe fix: Remove the renaming.

(lint/complexity/noUselessRename)

🔇 Additional comments (17)
libs/clients/national-registry/v3-applications/src/lib/providers.ts (1)

4-10: LGTM! Well-structured provider configuration.

The provider setup follows NestJS best practices for dependency injection and maintains type safety.

libs/clients/national-registry/v3-applications/src/lib/nationalRegistryV3Applications.module.ts (1)

6-14: LGTM! Well-structured NestJS module.

The module follows NestJS best practices for dependency injection and exports.

libs/clients/national-registry/v3-applications/src/lib/nationalRegistryV3Applications.config.ts (2)

4-11: LGTM!

The schema definition is well-structured and includes all necessary properties with appropriate types.


13-15: LGTM!

The configuration type is correctly defined using zod inference.

libs/clients/national-registry/v3-applications/src/lib/apiConfig.ts (2)

1-5: LGTM!

All necessary dependencies are correctly imported.


7-33: LGTM!

The API configuration is well-structured and follows NestJS dependency injection pattern. The fetch API is properly configured with timeout, authentication, and headers.

.github/CODEOWNERS (1)

181-181: LGTM!

The ownership assignment correctly assigns the v3-applications directory to the @island-is/juni team, aligning with the PR objectives.

libs/clients/national-registry/v3-applications/tsconfig.spec.json (3)

1-2: LGTM!

The configuration correctly extends the base tsconfig.json.


3-7: LGTM!

The compiler options are correctly configured for testing with Jest.


8-19: LGTM!

The file inclusion patterns correctly include all necessary test files and type definitions.

libs/clients/national-registry/v3-applications/project.json (5)

1-7: Project Metadata and Basic Configuration:
The project’s metadata (name, schema, sourceRoot, projectType, and tags) is clearly defined and follows the standard Nx configuration format. This sets a solid foundation for the new client library.


8-10: Lint Target Configuration:
The lint target is properly set up using the @nx/eslint:lint executor. This is consistent with best practices and helps ensure code quality.


11-19: Test Target Setup:
The testing target is configured to use Jest with a specified output directory for coverage reports and a clear reference to the project’s Jest configuration file. This configuration looks robust and in line with standard practices.


20-31: Update OpenAPI Document Command:
This target executes a series of shell commands to fetch, modify, and format the OpenAPI document. The command on line 24 uses nested curl calls to obtain a Bearer token dynamically, then pipes the response through jq and prettier. While the approach is technically sound, its complexity could affect maintainability.

  • Recommendation: Ensure that the required environment variables (NAT_REG_CLIENT_ID and NAT_REG_CLIENT_SECRET) are always provided in the execution environment.
  • Suggestion: Consider refactoring this multi-step command into a dedicated script for improved readability and easier troubleshooting.

32-38: Backend Client Code Generation Command:
The command to generate the backend client using yarn openapi-generator is straightforward and aligns with the project’s configuration targets. Double-check that the openapi-generator tool is available and properly configured to generate files in the specified output directory.

libs/clients/national-registry/v3-applications/src/clientConfig.json (1)

1-1247: Comprehensive OpenAPI Specification:
The OpenAPI specification for the MidlunUmsoknirRestApi is extensive and well-organized. It clearly defines various endpoints (e.g., /Einstaklingar/{kennitala}, /Einstaklingar/{kennitala}/faedingarstadur, etc.) with appropriate HTTP response codes, tags, and $ref pointers to detailed component schemas. The strict use of "additionalProperties": false on schemas ensures that responses strictly adhere to the defined contracts.

  • Note: Verify that all $ref references (such as #/components/schemas/EinstaklingurDTO) resolve correctly in the consuming code or during code generation.
tsconfig.base.json (1)

738-740: New Path Mapping Addition:
The new path mapping for @island.is/clients/national-registry-v3-applications pointing to libs/clients/national-registry/v3-applications/src/index.ts has been correctly added. This change will enable simpler and more consistent imports from the new client library and aligns seamlessly with the existing path mappings.

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link

nx-cloud bot commented Feb 4, 2025

View your CI Pipeline Execution ↗ for commit 656fb9a.

Command Status Duration Result
nx run-many --projects clients-national-registr... ✅ Succeeded 9s View ↗
nx run-many --target=lint --projects=clients-na... ✅ Succeeded 2s View ↗
nx run-many --target=codegen/frontend-client --... ✅ Succeeded 10s View ↗
nx run-many --target=codegen/backend-schema --a... ✅ Succeeded 22s View ↗

☁️ Nx Cloud last updated this comment at 2025-02-04 12:55:40 UTC

Copy link
Member

@pro-ingvar pro-ingvar left a comment

Choose a reason for hiding this comment

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

Only one comment, otherwise LGTM

@kksteini kksteini requested a review from pro-ingvar February 4, 2025 12:27
…om:/island-is/island.is into feat/national-registry-applications-client
Copy link
Member

@pro-ingvar pro-ingvar left a comment

Choose a reason for hiding this comment

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

Well done, approved 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants