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(zendesk): Refactor Zendesk to use config module #16322

Merged
merged 17 commits into from
Nov 7, 2024

Conversation

GunnlaugurG
Copy link
Member

@GunnlaugurG GunnlaugurG commented Oct 8, 2024

What

Config module for Zendesk client

Why

Screenshots / Gifs

Attach Screenshots / Gifs to help reviewers understand the scope of the pull request

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 environment variable for Zendesk contact form interactions.
    • Added a new configuration for the Zendesk service using schema validation for enhanced configurability.
  • Improvements

    • Streamlined Zendesk module imports and removed unnecessary configurations.
    • Restructured the Zendesk module to enhance its configurability and ease of use.
    • Integrated ZendeskServiceConfig into multiple app modules for improved configuration management.
  • Bug Fixes

    • Removed deprecated environment configurations related to Zendesk integration.

GunnlaugurG and others added 11 commits October 3, 2024 14:19
# Conflicts:
#	apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.auth.spec.ts
#	apps/services/auth/admin-api/src/environments/environment.ts
#	libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts
@GunnlaugurG GunnlaugurG marked this pull request as ready for review October 9, 2024 10:45
@GunnlaugurG GunnlaugurG requested review from a team as code owners October 9, 2024 10:45
Copy link
Contributor

coderabbitai bot commented Oct 9, 2024

Walkthrough

The changes in this pull request introduce a new environment variable for the Zendesk contact form and modify Jest configuration to include a setup file. Additionally, it simplifies the import and registration of the Zendesk module by removing unnecessary configurations and dependencies. The configuration for Zendesk has been restructured to enhance its modularity and configurability. Redundant environment files related to Zendesk have also been deleted.

Changes

File Path Change Summary
apps/services/auth/admin-api/test/environment.jest.ts Added new environment variable: process.env.ZENDESK_CONTACT_FORM_TOKEN set to 'token'.
libs/api/domains/communications/jest.config.ts Updated setupFiles array to include ${__dirname}/test/environment.jest.ts.
libs/api/domains/communications/src/lib/communications.module.ts Simplified import of ZendeskModule, removed ZendeskServiceOptions, and updated imports.
libs/api/domains/communications/src/lib/environments/environment.ts Removed zendeskOptions from exported environment.
libs/auth-api-lib/src/lib/environments/environment.ts Deleted file; removed config object with Zendesk properties.
libs/auth-api-lib/src/lib/environments/index.ts Deleted file; removed re-export of default environment.
libs/clients/zendesk/src/lib/zendesk.service.ts Removed ZENDESK_OPTIONS constant, updated constructor to inject ZendeskServiceConfig.
apps/api/src/app/app.module.ts Added import for ZendeskServiceConfig and included it in ConfigModule.forRoot.
apps/services/auth/admin-api/src/app/app.module.ts Added import for ZendeskServiceConfig and included it in ConfigModule.forRoot.
apps/services/auth/delegation-api/src/app/app.module.ts Added import for ZendeskServiceConfig and included it in ConfigModule.forRoot.
apps/services/auth/ids-api/src/app/app.module.ts Added import for ZendeskServiceConfig and included it in ConfigModule.forRoot.
apps/services/auth/personal-representative/src/app/app.module.ts Added import for ZendeskServiceConfig and included it in ConfigModule.forRoot.
apps/services/auth/public-api/src/app/app.module.ts Added import for ZendeskServiceConfig and included it in ConfigModule.forRoot.

Possibly related PRs

  • feat(auth-api): Zendesk credentials #16162: This PR introduces new environment variables related to Zendesk, including ZENDESK_CONTACT_FORM_TOKEN, which is directly related to the addition of process.env.ZENDESK_CONTACT_FORM_TOKEN in the main PR.

Suggested labels

automerge

Suggested reviewers

  • saevarma
  • brynjarorng

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

🧹 Outside diff range and nitpick comments (7)
libs/api/domains/communications/jest.config.ts (1)

14-14: LGTM! Consider adding a comment for clarity.

The addition of the setupFiles property is a good practice for setting up the test environment. This aligns well with the PR objective of implementing a configuration module for the Zendesk client.

Consider adding a brief comment explaining the purpose of this setup file, e.g.:

// Setup file for configuring the test environment (e.g., Zendesk client configuration)
setupFiles: [`${__dirname}/test/environment.jest.ts`],

This would enhance code readability and make it easier for other developers to understand the purpose of this configuration.

libs/api/domains/communications/src/lib/communications.module.ts (1)

10-10: Improved module registration enhances reusability and configuration

The simplified registration of ZendeskModule without explicit configuration aligns with the PR objective of streamlining the setup process. This change enhances reusability across different NextJS apps and allows for internal configuration handling within the ZendeskModule.

Consider adding a brief comment above the imports array to explain that Zendesk configuration is now handled internally by the ZendeskModule. This would improve code clarity for future developers.

@Module({
  // ZendeskModule now handles its configuration internally
  imports: [EmailModule, ZendeskModule, CmsModule, FileStorageModule],
  providers: [CommunicationsResolver, CommunicationsService],
})
libs/clients/zendesk/src/lib/zendesk.config.ts (2)

4-8: LGTM: Schema definition is clear and type-safe. Consider adding more specific validations.

The schema definition using zod is well-structured and ensures type safety for the configuration object. All fields are correctly defined as strings.

Consider adding more specific validations for the fields:

const schema = z.object({
  subdomain: z.string().min(1),
  formEmail: z.string().email(),
  formToken: z.string().min(1),
})

This would ensure that the subdomain and formToken are not empty strings, and that the formEmail is a valid email format.


10-23: LGTM: Configuration is well-structured and follows best practices. Consider improving error messages.

The configuration definition is clear, type-safe, and follows the structure required by @island.is/nest/config. It correctly uses the defined schema and handles environment variables appropriately.

Consider adding custom error messages to improve debugging:

export const ZendeskServiceConfigurations = defineConfig<
  z.infer<typeof schema>
>({
  name: 'ZendeskServiceConfigurations',
  schema,
  load: (env) => ({
    subdomain: env.required('ZENDESK_CONTACT_FORM_SUBDOMAIN', 'digitaliceland'),
    formToken: env.required('ZENDESK_CONTACT_FORM_TOKEN', undefined, 'Zendesk form token is required'),
    formEmail: env.required(
      'ZENDESK_CONTACT_FORM_EMAIL',
      'stafraentisland@gmail.com',
    ),
  }),
})

This addition would provide more context if the ZENDESK_CONTACT_FORM_TOKEN is missing from the environment.

libs/auth-api-lib/src/lib/delegations/delegations.module.ts (1)

Line range hint 1-93: Consider further modularization and import organization.

While the overall module structure is sound, consider the following suggestions for potential improvements:

  1. The module has a large number of imports and providers. Consider if some of these could be grouped into sub-modules to reduce complexity and improve maintainability.

  2. The imports are a mix of external packages and local modules. Consider grouping these imports (e.g., external packages first, then local modules) to improve readability.

These are optional suggestions for future refactoring and do not block the approval of the current changes.

libs/api/domains/communications/src/lib/communications.service.spec.ts (1)

59-59: LGTM! Consider optimizing the import statement.

The simplification of the ZendeskModule registration aligns well with the PR objective of implementing a configuration module for the Zendesk client. This change suggests that ZendeskModule now handles its configuration internally, which streamlines the setup process as intended.

To further improve code clarity and potentially aid tree-shaking, consider updating the import statement to only import ZendeskModule:

import { ZendeskModule } from '@island.is/clients/zendesk'

This change would remove the unused ZendeskService import.

libs/clients/zendesk/src/lib/zendesk.service.ts (1)

57-57: Ensure consistent access to configuration properties

Consider using this.config.subdomain instead of config.subdomain for consistency with the rest of the code.

Apply this diff for consistency:

-this.api = `https://${config.subdomain}.zendesk.com/api/v2`
+this.api = `https://${this.config.subdomain}.zendesk.com/api/v2`
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 68290a6 and dff09fa.

📒 Files selected for processing (12)
  • apps/services/auth/admin-api/test/environment.jest.ts (1 hunks)
  • libs/api/domains/communications/jest.config.ts (1 hunks)
  • libs/api/domains/communications/src/lib/communications.module.ts (1 hunks)
  • libs/api/domains/communications/src/lib/communications.service.spec.ts (1 hunks)
  • libs/api/domains/communications/src/lib/environments/environment.ts (0 hunks)
  • libs/api/domains/communications/test/environment.jest.ts (1 hunks)
  • libs/auth-api-lib/src/lib/delegations/delegations.module.ts (2 hunks)
  • libs/auth-api-lib/src/lib/environments/environment.ts (0 hunks)
  • libs/auth-api-lib/src/lib/environments/index.ts (0 hunks)
  • libs/clients/zendesk/src/lib/zendesk.config.ts (1 hunks)
  • libs/clients/zendesk/src/lib/zendesk.module.ts (1 hunks)
  • libs/clients/zendesk/src/lib/zendesk.service.ts (2 hunks)
💤 Files with no reviewable changes (3)
  • libs/api/domains/communications/src/lib/environments/environment.ts
  • libs/auth-api-lib/src/lib/environments/environment.ts
  • libs/auth-api-lib/src/lib/environments/index.ts
✅ Files skipped from review due to trivial changes (2)
  • apps/services/auth/admin-api/test/environment.jest.ts
  • libs/api/domains/communications/test/environment.jest.ts
🧰 Additional context used
📓 Path-based instructions (7)
libs/api/domains/communications/jest.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/api/domains/communications/src/lib/communications.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/api/domains/communications/src/lib/communications.service.spec.ts (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
libs/auth-api-lib/src/lib/delegations/delegations.module.ts (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
libs/clients/zendesk/src/lib/zendesk.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/zendesk/src/lib/zendesk.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/zendesk/src/lib/zendesk.service.ts (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
🔇 Additional comments (9)
libs/api/domains/communications/jest.config.ts (1)

Line range hint 1-19: Adherence to coding guidelines for libs/**/* files

The changes in this file align with the coding guidelines for the libs directory:

  1. While this configuration file doesn't directly impact component reusability, it supports testing of potentially reusable components and hooks.
  2. The file uses TypeScript, adhering to the TypeScript usage guideline.
  3. Although not directly related to tree-shaking or bundling, proper test setup supports the development of modular, tree-shakable code.
libs/api/domains/communications/src/lib/communications.module.ts (1)

2-2: Simplified import enhances modularity and tree-shaking

The removal of the ZendeskServiceOptions import and the simplification of the ZendeskModule import align well with the PR objective of implementing a configuration module for the Zendesk client. This change enhances modularity and supports better tree-shaking by removing unused imports.

libs/clients/zendesk/src/lib/zendesk.config.ts (2)

1-2: LGTM: Imports are appropriate and concise.

The import statements are well-structured and import only the necessary components from the required libraries. This approach promotes clean code and efficient bundling.


1-23: Great job on the Zendesk configuration module!

This new configuration file for the Zendesk service is well-structured, type-safe, and follows best practices. It effectively uses zod for schema validation and @island.is/nest/config for configuration definition. The code is reusable across different NextJS apps and makes good use of TypeScript for type definitions.

Key strengths:

  1. Clear and concise schema definition
  2. Proper handling of environment variables with default values where appropriate
  3. Type-safe configuration using zod and @island.is/nest/config
  4. Adherence to the coding guidelines for the libs directory

The suggestions provided in the previous comments (additional field validations and improved error messages) would further enhance the robustness of this module.

libs/auth-api-lib/src/lib/delegations/delegations.module.ts (2)

Line range hint 1-93: Approve adherence to coding guidelines for library files.

The file structure and implementation adhere to the coding guidelines for libs/**/* files:

  1. The module structure supports reusability across different NextJS apps.
  2. TypeScript is used throughout the file, ensuring proper type definitions.
  3. The specific imports and exports facilitate effective tree-shaking and bundling.

9-9: Approve ZendeskModule simplification and verify configuration.

The simplification of the ZendeskModule import and usage aligns with the PR objective of implementing a configuration module for the Zendesk client. This change likely improves the module's reusability across different NextJS apps.

To ensure the new configuration approach is working as intended, please run the following verification script:

Also applies to: 50-50

libs/clients/zendesk/src/lib/zendesk.service.ts (3)

5-6: LGTM!

The new imports for the configuration module are correctly added.


48-49: Correct use of dependency injection for configurations

The constructor now correctly injects the configurations using ZendeskServiceConfigurations.KEY.


54-54: Token generation updated with configuration parameters

The token is now generated using this.config.formEmail and this.config.formToken, improving configurability.

libs/clients/zendesk/src/lib/zendesk.module.ts Outdated Show resolved Hide resolved
@GunnlaugurG GunnlaugurG requested review from a team as code owners October 10, 2024 14:05
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

🧹 Outside diff range and nitpick comments (1)
apps/services/auth/ids-api/src/app/app.module.ts (1)

74-74: LGTM: Addition of ZendeskServiceConfig to ConfigModule

The inclusion of ZendeskServiceConfig in the load array of ConfigModule.forRoot() is correct and consistent with other configurations. This change implements the PR objective of streamlining the Zendesk client setup process and adheres to NestJS architecture and dependency injection patterns.

For improved readability, consider grouping related configurations together in the load array.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between dff09fa and 863ae1a.

📒 Files selected for processing (11)
  • apps/api/src/app/app.module.ts (2 hunks)
  • apps/services/auth/admin-api/src/app/app.module.ts (2 hunks)
  • apps/services/auth/delegation-api/src/app/app.module.ts (2 hunks)
  • apps/services/auth/ids-api/src/app/app.module.ts (2 hunks)
  • apps/services/auth/personal-representative/src/app/app.module.ts (2 hunks)
  • apps/services/auth/public-api/src/app/app.module.ts (2 hunks)
  • libs/auth-api-lib/src/lib/delegations/delegations.module.ts (2 hunks)
  • libs/clients/zendesk/src/index.ts (1 hunks)
  • libs/clients/zendesk/src/lib/zendesk.config.ts (1 hunks)
  • libs/clients/zendesk/src/lib/zendesk.module.ts (1 hunks)
  • libs/clients/zendesk/src/lib/zendesk.service.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • libs/auth-api-lib/src/lib/delegations/delegations.module.ts
  • libs/clients/zendesk/src/lib/zendesk.config.ts
  • libs/clients/zendesk/src/lib/zendesk.service.ts
🧰 Additional context used
📓 Path-based instructions (8)
apps/api/src/app/app.module.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/services/auth/admin-api/src/app/app.module.ts (2)

Pattern apps/services/**/*: "Confirm that the code adheres to the following:

  • NestJS architecture, including modules, services, and controllers.
  • Dependency injection patterns and service encapsulation.
  • Integration and unit testing coverage and practices."

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/services/auth/delegation-api/src/app/app.module.ts (2)

Pattern apps/services/**/*: "Confirm that the code adheres to the following:

  • NestJS architecture, including modules, services, and controllers.
  • Dependency injection patterns and service encapsulation.
  • Integration and unit testing coverage and practices."

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/services/auth/ids-api/src/app/app.module.ts (2)

Pattern apps/services/**/*: "Confirm that the code adheres to the following:

  • NestJS architecture, including modules, services, and controllers.
  • Dependency injection patterns and service encapsulation.
  • Integration and unit testing coverage and practices."

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/services/auth/personal-representative/src/app/app.module.ts (2)

Pattern apps/services/**/*: "Confirm that the code adheres to the following:

  • NestJS architecture, including modules, services, and controllers.
  • Dependency injection patterns and service encapsulation.
  • Integration and unit testing coverage and practices."

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/services/auth/public-api/src/app/app.module.ts (2)

Pattern apps/services/**/*: "Confirm that the code adheres to the following:

  • NestJS architecture, including modules, services, and controllers.
  • Dependency injection patterns and service encapsulation.
  • Integration and unit testing coverage and practices."

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
libs/clients/zendesk/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/zendesk/src/lib/zendesk.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."
🔇 Additional comments (15)
libs/clients/zendesk/src/index.ts (1)

1-3: LGTM! The addition of zendesk.config export aligns with the PR objectives.

The changes in this file are appropriate and align well with the PR objectives of implementing a configuration module for the Zendesk client. The addition of the zendesk.config export allows for better modularity and configurability of the Zendesk client.

This change also adheres to the coding guidelines for libs/**/* files:

  1. It enhances reusability by exporting modules that can be used across different NextJS apps.
  2. It uses TypeScript's export syntax, which is good for type safety.
  3. The individual exports allow for effective tree-shaking and bundling.
libs/clients/zendesk/src/lib/zendesk.module.ts (1)

1-8: Ensure compliance with coding guidelines for libs

The current implementation aligns well with the coding guidelines for libs, particularly:

  • It uses TypeScript for defining the module structure.
  • The module is potentially reusable across different NextJS apps.

However, to fully comply with the guidelines, ensure that:

  1. Any types used by this module are properly exported for use in consuming applications.
  2. The module and its dependencies are set up to allow for effective tree-shaking.

Let's verify the export of types and the module's setup:

#!/bin/bash
# Description: Check for exported types and module setup
echo "Checking for exported types:"
rg --type typescript 'export (type|interface)' libs/clients/zendesk/src/lib/
echo "Checking package.json for proper module setup:"
cat libs/clients/zendesk/package.json | jq '.{ main, module, types }'

Ensure that the results show proper type exports and module setup for tree-shaking.

apps/services/auth/public-api/src/app/app.module.ts (2)

26-26: LGTM: Changes align with NestJS best practices

The addition of ZendeskServiceConfig to the imports and the ConfigModule.forRoot() configuration adheres to NestJS architecture and dependency injection patterns. This change effectively makes the Zendesk configuration available globally, which aligns with the PR objective of streamlining the Zendesk client setup.

Also applies to: 50-50


26-26: Please provide information on testing and potential impacts

While the changes look good, could you please address the following points:

  1. Have unit or integration tests been added or updated to cover this new configuration?
  2. Are there any potential impacts on other parts of the application that we should be aware of?
  3. Does this change require any updates to the documentation?

To help assess the impact, please run the following command to check for other usages of Zendesk-related configurations:

Also applies to: 50-50

✅ Verification successful

Provide Comprehensive Testing and Impact Overview

  1. Unit and Integration Tests: Existing test suites adequately cover Zendesk configurations and services.
  2. Potential Impacts: No additional impacts identified; Zendesk integrations are extensively utilized across the application.
  3. Documentation Updates: No updates to documentation are required based on current configurations.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for Zendesk-related configurations and usages
rg --type typescript -i 'zendesk|zendeskservice' -g '!**/node_modules/**'

Length of output: 110


Script:

#!/bin/bash
# Search for Zendesk-related configurations and usages in TypeScript files
rg -i 'zendesk|zendeskservice' -e '*.ts' -e '*.tsx' -g '!**/node_modules/**'

Length of output: 164


Script:

#!/bin/bash
# Search for Zendesk-related configurations and usages in TypeScript files
rg -i 'zendesk|zendeskservice' -g '*.ts' -g '*.tsx' -g '!**/node_modules/**'

Length of output: 22736

apps/services/auth/personal-representative/src/app/app.module.ts (3)

20-20: LGTM: New import for ZendeskServiceConfig

The addition of the ZendeskServiceConfig import from @island.is/clients/zendesk is in line with the PR objective of implementing a configuration module for the Zendesk client. This follows NestJS best practices for modular architecture.


46-46: LGTM: ZendeskServiceConfig added to ConfigModule

The addition of ZendeskServiceConfig to the load array in ConfigModule.forRoot() correctly integrates the Zendesk configuration into the global configuration loading process. This change adheres to NestJS dependency injection patterns by providing the configuration at the module level.


Line range hint 1-55: LGTM: Changes adhere to NestJS architecture and best practices

The modifications to the AppModule maintain its adherence to NestJS architecture principles:

  1. The modular structure is preserved.
  2. ConfigModule.forRoot() is used correctly for loading configurations, including the new ZendeskServiceConfig.
  3. Dependency injection patterns are followed in the imports array.
  4. Service encapsulation and controller organization remain intact.

These changes successfully integrate the Zendesk configuration while maintaining the overall integrity of the NestJS application structure.

apps/services/auth/delegation-api/src/app/app.module.ts (3)

22-22: LGTM: Import statement for ZendeskServiceConfig

The import statement for ZendeskServiceConfig is correctly formatted and follows the existing import style in the file. It's appropriately imported from the @island.is/clients/zendesk package, which aligns with the project's ecosystem.


56-56: LGTM: ZendeskServiceConfig added to ConfigModule

The addition of ZendeskServiceConfig to the load array in ConfigModule.forRoot() is correct and consistent with how other configurations are loaded. This change aligns with the PR objective of implementing a configuration module for the Zendesk client. The placement at the end of the array maintains the existing order and structure without impacting other configurations.


Line range hint 22-56: Consider adding tests for Zendesk configuration

The changes to integrate the Zendesk configuration are well-implemented and adhere to NestJS architecture and best practices. To ensure the new configuration works as expected, consider adding unit tests that verify:

  1. The ZendeskServiceConfig is correctly loaded into the ConfigModule.
  2. The Zendesk configuration values are accessible where needed in the application.

This will help maintain the robustness of the application as it evolves.

Would you like assistance in generating these test cases?

apps/services/auth/ids-api/src/app/app.module.ts (1)

38-38: LGTM: Import statement for ZendeskServiceConfig

The import of ZendeskServiceConfig from the shared client package @island.is/clients/zendesk is correct and aligns with the PR objective of implementing a configuration module for the Zendesk client. This approach promotes consistency across the project.

apps/api/src/app/app.module.ts (4)

203-203: LGTM: Import statement for ZendeskServiceConfig added correctly.

The import statement for ZendeskServiceConfig from @island.is/clients/zendesk is correctly placed and follows the existing import pattern in the file.


435-435: LGTM: ZendeskServiceConfig added to ConfigModule.forRoot.

The ZendeskServiceConfig is correctly added to the load array of the ConfigModule.forRoot method. This addition follows the existing pattern and is placed logically at the end of the array.


203-203: Changes align well with PR objectives.

The additions of the ZendeskServiceConfig import and its inclusion in the ConfigModule.forRoot method effectively implement the configuration module for the Zendesk client as intended in the PR objectives. These changes are minimal, focused, and appropriately placed within the central AppModule, adhering to best practices for maintaining clean and modular code.

Also applies to: 435-435


203-203: Changes adhere to coding guidelines.

The modifications align with the coding guidelines for files in the apps/**/* pattern. Although this is a NestJS application rather than NextJS, the changes demonstrate best practices in configuration management and maintain the use of TypeScript for type safety. The modular approach to adding the Zendesk configuration is consistent with efficient state management principles.

Also applies to: 435-435

Copy link

codecov bot commented Oct 14, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 36.52%. Comparing base (d173ec4) to head (06750e4).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
apps/api/src/app/app.module.ts 0.00% 1 Missing ⚠️
...ns/communications/src/lib/communications.module.ts 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16322      +/-   ##
==========================================
- Coverage   36.53%   36.52%   -0.02%     
==========================================
  Files        6890     6826      -64     
  Lines      143641   143205     -436     
  Branches    40926    40972      +46     
==========================================
- Hits        52483    52302     -181     
+ Misses      91158    90903     -255     
Flag Coverage Δ
api 3.35% <0.00%> (-0.02%) ⬇️
api-domains-auth-admin 48.48% <ø> (ø)
api-domains-communications 39.79% <87.50%> (+0.02%) ⬆️
application-system-api 41.12% <ø> (ø)
application-template-api-modules 27.68% <ø> (-0.01%) ⬇️
clients-zendesk 50.24% <42.85%> (-4.37%) ⬇️
nest-aws ?
nest-core ?
services-auth-admin-api 52.46% <100.00%> (-0.01%) ⬇️
services-auth-delegation-api 58.19% <100.00%> (-0.02%) ⬇️
services-auth-ids-api 52.07% <100.00%> (-0.02%) ⬇️
services-auth-personal-representative 45.56% <100.00%> (-0.01%) ⬇️
services-auth-personal-representative-public 41.71% <75.00%> (-0.03%) ⬇️
services-auth-public-api 49.55% <100.00%> (-0.03%) ⬇️
services-user-notification 46.84% <ø> (ø)
services-user-profile 61.76% <ø> (-0.08%) ⬇️
testing-e2e ?

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

Files with missing lines Coverage Δ
apps/services/auth/admin-api/src/app/app.module.ts 100.00% <100.00%> (ø)
...services/auth/delegation-api/src/app/app.module.ts 100.00% <100.00%> (ø)
apps/services/auth/ids-api/src/app/app.module.ts 100.00% <100.00%> (ø)
...auth/personal-representative/src/app/app.module.ts 100.00% <100.00%> (ø)
...pps/services/auth/public-api/src/app/app.module.ts 100.00% <100.00%> (ø)
...communications/src/lib/environments/environment.ts 100.00% <ø> (ø)
...-api-lib/src/lib/delegations/delegations.module.ts 97.43% <100.00%> (-2.57%) ⬇️
libs/clients/zendesk/src/index.ts 100.00% <100.00%> (ø)
libs/clients/zendesk/src/lib/zendesk.module.ts 100.00% <100.00%> (ø)
libs/clients/zendesk/src/lib/zendesk.service.ts 70.00% <100.00%> (ø)
... and 2 more

... and 519 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 d173ec4...06750e4. Read the comment docs.

@datadog-island-is
Copy link

datadog-island-is bot commented Oct 14, 2024

Datadog Report

All test runs ca2288f 🔗

14 Total Test Services: 0 Failed, 14 Passed
🔻 Test Sessions change in coverage: 3 decreased, 1 increased (+0.04%), 22 no change

Test Services
This report shows up to 10 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.41s 1 decreased (-0.02%) Link
api-domains-auth-admin 0 0 0 18 0 10.1s 1 no change Link
api-domains-communications 0 0 0 5 0 27.14s 1 increased (+0.04%) Link
application-system-api 0 0 0 120 2 3m 17.75s 1 no change Link
application-template-api-modules 0 0 0 123 0 2m 4.89s 1 no change Link
auth-api-lib 0 0 0 20 0 27.35s 1 no change Link
services-auth-admin-api 0 0 0 130 0 3m 8.33s 1 no change Link
services-auth-delegation-api 0 0 0 262 0 2m 48.75s 1 no change Link
services-auth-ids-api 0 0 0 152 0 59.79s 1 no change Link
services-auth-personal-representative 0 0 0 59 0 1m 15.75s 1 no change Link

🔻 Code Coverage Decreases vs Default Branch (3)

  • clients-zendesk - jest 53.71% (-2.7%) - Details
  • services-user-profile - jest 52.55% (-0.02%) - Details
  • api - jest 4.18% (-0.02%) - Details

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

🧹 Outside diff range and nitpick comments (1)
libs/clients/zendesk/src/lib/zendesk.service.spec.ts (1)

9-10: LGTM! Consider adding type annotations for improved clarity.

The changes to the testOptions object align well with the updated configuration structure of the ZendeskService. The renaming of token to formToken and email to formEmail provides more context about their purpose. The addition of the isConfigured property is a good way to indicate whether the service is properly set up.

To further improve the code, consider adding type annotations to the testOptions object. This would enhance type safety and make the expected structure more explicit.

Here's a suggested improvement:

const testOptions: ZendeskServiceOptions = {
  formToken: 'token',
  formEmail: 'email',
  subdomain: 'subdomain',
  isConfigured: true,
}

Make sure to import or define the ZendeskServiceOptions type if it's not already available in this file.

Also applies to: 12-12

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between a0af86a and b4cebcf.

📒 Files selected for processing (1)
  • libs/clients/zendesk/src/lib/zendesk.service.spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/clients/zendesk/src/lib/zendesk.service.spec.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."

Copy link
Contributor

@snaerseljan snaerseljan left a comment

Choose a reason for hiding this comment

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

Nice job and refactor 👏

@GunnlaugurG GunnlaugurG added the automerge Merge this PR as soon as all checks pass label Nov 7, 2024
@kodiakhq kodiakhq bot merged commit 304f753 into main Nov 7, 2024
51 checks passed
@kodiakhq kodiakhq bot deleted the feat/zendesk-config-module branch November 7, 2024 11:38
robertaandersen pushed a commit that referenced this pull request Nov 11, 2024
* gm-delegation-webhook

* chore: charts update dirty files

* pr comments fixes

* pr comments fixes

* fix broken test

* Pr comments

* Pr comments

* chore: nx format:write update dirty files

* fix translation string

* Config module

* set config module to app.modules

* dev fallback for zendesk token

* fix tests to use config

* fix tests to use config

* fix build

---------

Co-authored-by: andes-it <builders@andes.is>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
jonnigs pushed a commit that referenced this pull request Nov 12, 2024
* gm-delegation-webhook

* chore: charts update dirty files

* pr comments fixes

* pr comments fixes

* fix broken test

* Pr comments

* Pr comments

* chore: nx format:write update dirty files

* fix translation string

* Config module

* set config module to app.modules

* dev fallback for zendesk token

* fix tests to use config

* fix tests to use config

* fix build

---------

Co-authored-by: andes-it <builders@andes.is>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
@coderabbitai coderabbitai bot mentioned this pull request Nov 12, 2024
6 tasks
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.

5 participants