Service Export #2526
Service Export #2526
Conversation
WalkthroughThe update introduces new middleware, modifies exported methods and classes, and enhances notification and logging functionalities. Key changes include updating method signatures to align with new parameters, simplifying and restructuring code imports, adding detailed documentation, and initiating default settings. These provide improved code organization, better error handling, and more robust middleware functionality. Changes
Poem
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
jonathanfallon
force-pushed
the
feat/export
branch
from
b4ce329
to
33d50e0
Compare
jonathanfallon
force-pushed
the
feat/export
branch
from
a1e2462
to
abb0173
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (3)
api/src/pdc/providers/middleware/CopyFromContext/CopyFromContextMiddleware.ts (1)
Line range hint
20-20: Avoid usingFunctionas a type.Prefer explicitly defining the function shape to avoid potential bugs.
- next: Function, + next: (params: ParamsType, context: ContextType) => Promise<ResultType>,api/src/pdc/services/apdf/commands/ExportCommand.ts (1)
Line range hint
121-121: Use lowercase primitives for typesReplace
Stringwithstringfor consistency and to follow best practices.- private async findActiveCampaigns(datetime: String): Promise<number[]> { + private async findActiveCampaigns(datetime: string): Promise<number[]> {- await this.kernel.call<{ datetime: String }, ListCampaignsResults>( + await this.kernel.call<{ datetime: string }, ListCampaignsResults>(Also applies to: 125-125
api/src/pdc/providers/notification/NotificationMailTransporter.ts (1)
Line range hint
118-126: Add error handling for external calls.The
sendMailmethod call should be wrapped in a try-catch block to handle potential errors.+ try { this.transporter && await this.transporter.sendMail({ ...options, from: this.options.from, to: this.options.debug ? this.options.debugToOverride : mail.to, subject: mailCtor.subject, html: mailCtor.templateMJML ? this.render(new mailCtor.templateMJML(mail.data), true) : undefined, text: this.render(new mailCtor.templateText(mail.data)), }); + } catch (error) { + logger.error(`Failed to send email: ${error.message}`); + throw error; + }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (31)
- api/src/config/contacts.ts (1 hunks)
- api/src/pdc/middlewares/DefaultTimezoneMiddleware.ts (1 hunks)
- api/src/pdc/providers/middleware/Cast/CastToArrayMiddleware.ts (2 hunks)
- api/src/pdc/providers/middleware/CopyFromContext/CopyFromContextMiddleware.ts (1 hunks)
- api/src/pdc/providers/middleware/HasPermission/HasPermissionMiddleware.ts (3 hunks)
- api/src/pdc/providers/middleware/ValidateDate/ValidateDateMiddleware.ts (3 hunks)
- api/src/pdc/providers/notification/NotificationMailTransporter.ts (4 hunks)
- api/src/pdc/services/apdf/commands/ExportCommand.ts (1 hunks)
- api/src/pdc/services/application/config/permissions.ts (1 hunks)
- api/src/pdc/services/export/ServiceProvider.ts (2 hunks)
- api/src/pdc/services/export/actions/CreateAction.integration.spec.ts (6 hunks)
- api/src/pdc/services/export/actions/CreateActionV2.ts (4 hunks)
- api/src/pdc/services/export/actions/CreateActionV3.ts (2 hunks)
- api/src/pdc/services/export/commands/ProcessCommand.ts (4 hunks)
- api/src/pdc/services/export/models/Campaign.ts (1 hunks)
- api/src/pdc/services/export/models/Export.ts (3 hunks)
- api/src/pdc/services/export/models/ExportParams.ts (2 hunks)
- api/src/pdc/services/export/repositories/ExportRepository.ts (3 hunks)
- api/src/pdc/services/export/services/FileCreatorService.ts (3 hunks)
- api/src/pdc/services/export/services/LogService.ts (1 hunks)
- api/src/pdc/services/export/services/NotificationService.ts (1 hunks)
- api/src/pdc/services/export/services/StorageService.ts (1 hunks)
- api/src/pdc/services/user/actions/LoginUserAction.ts (3 hunks)
- api/src/pdc/services/user/actions/NotifyUserAction.ts (2 hunks)
- api/src/pdc/services/user/config/permissions.ts (1 hunks)
- api/src/pdc/services/user/notifications/ExportCSVSupportNotification.ts (1 hunks)
- api/src/pdc/services/user/notifications/index.ts (1 hunks)
- api/src/pdc/services/user/providers/UserNotificationProvider.ts (2 hunks)
- api/src/shared/user/notify.contract.ts (1 hunks)
- flake.nix (1 hunks)
- shared/export/create.schema.ts (1 hunks)
Files skipped from review due to trivial changes (4)
- api/src/config/contacts.ts
- api/src/pdc/services/application/config/permissions.ts
- api/src/pdc/services/user/notifications/index.ts
- flake.nix
Additional context used
Biome
api/src/shared/user/notify.contract.ts
[error] 7-7: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
api/src/pdc/providers/middleware/CopyFromContext/CopyFromContextMiddleware.ts
[error] 20-20: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
api/src/pdc/services/apdf/commands/ExportCommand.ts
[error] 121-121: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead(lint/complexity/noBannedTypes)
[error] 125-125: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead(lint/complexity/noBannedTypes)
Additional comments not posted (34)
api/src/pdc/middlewares/DefaultTimezoneMiddleware.ts (1)
1-23: LGTM!The
DefaultTimezoneMiddlewareclass is well-implemented and follows standard practices.api/src/pdc/services/user/actions/NotifyUserAction.ts (1)
Line range hint
1-24: LGTM!The
NotifyUserActionclass is well-implemented and follows standard practices.api/src/pdc/services/user/notifications/ExportCSVSupportNotification.ts (1)
1-43: LGTM!The
ExportCSVSupportNotificationclass is well-implemented and follows standard practices.shared/export/create.schema.ts (1)
63-63: LGTM! Ensuresterritory_idis validated as a serial type.The addition of
macro: "serial"for theterritory_idproperty inschemaV3ensures proper validation.api/src/pdc/services/user/actions/LoginUserAction.ts (1)
34-35: LGTM! Improves security by handling invalid email or password.The addition of the
UnauthorizedExceptionensures that invalid login attempts are properly handled.api/src/pdc/services/export/services/StorageService.ts (1)
1-56: LGTM! ImplementsStorageServicewith S3 storage operations.The class provides methods to initialize, list, upload, and get public URLs for storage objects, following expected patterns for S3 storage operations.
api/src/pdc/providers/middleware/Cast/CastToArrayMiddleware.ts (1)
49-58: LGTM! Improves documentation and usability.The addition of the function signature and explanatory comments enhances the clarity and usability of the
castToArrayMiddlewarefunction.api/src/pdc/providers/middleware/HasPermission/HasPermissionMiddleware.ts (1)
23-23: Use ofNextFunctioninstead ofFunctionis appropriate.Updating the type to
NextFunctionaligns with Express.js middleware conventions and improves type safety.api/src/pdc/services/export/models/Campaign.ts (1)
3-3: Verify the new import path fortoTzString.Ensure that the updated path
@/pdc/helpers/dates.helper.tsis correct and consistent with the project's structure.Verification successful
Verified the new import path for
toTzString.The function
toTzStringis correctly defined inapi/src/pdc/helpers/dates.helper.ts, and the file exists as expected.
api/src/pdc/helpers/dates.helper.tscontains thetoTzStringfunction definition.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new import path for `toTzString`. # Test: Search for the `toTzString` function definition. Expect: The function definition should be in `@/pdc/helpers/dates.helper.ts`. rg --type typescript 'function toTzString'Length of output: 79
Script:
#!/bin/bash # Description: Verify the new import path for `toTzString`. # Test: Search for the `toTzString` function definition in TypeScript files. rg --type ts 'function toTzString' # Test: Verify if the file "@/pdc/helpers/dates.helper.ts" exists. fd 'dates.helper.ts' --type fLength of output: 256
api/src/pdc/services/export/actions/CreateActionV2.ts (3)
27-28: Updated parameters forcopyFromContextMiddlewareare logical.The change from
truetofalsefor thepreserveparameter makes sense to ensure the values are always copied.
52-52: Updatedcreated_byassignment is more robust.Using
getto retrievecontext.call.user._idwith a fallback tonullimproves the robustness of the code.
6-6: Verify the new import path fortoISOString.Ensure that the updated path
@/pdc/helpers/dates.helper.tsis correct and consistent with the project's structure.Verification successful
The new import path for
toISOStringis correct.The function
toISOStringis defined inapi/src/pdc/helpers/dates.helper.ts, which matches the import path@/pdc/helpers/dates.helper.ts.
api/src/pdc/helpers/dates.helper.ts:export function toISOString(d: Date): string {Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new import path for `toISOString`. # Test: Search for the `toISOString` function definition. Expect: The function definition should be in `@/pdc/helpers/dates.helper.ts`. rg --type typescript 'function toISOString'Length of output: 80
Script:
#!/bin/bash # Correct the search to find the `toISOString` function definition in TypeScript files. # Search for the `toISOString` function definition. rg 'function toISOString' --glob '*.ts' # Confirm the path of the `dates.helper.ts` file. fd 'dates.helper.ts'Length of output: 181
api/src/pdc/providers/middleware/CopyFromContext/CopyFromContextMiddleware.ts (1)
48-68: NewcopyFromContextMiddlewarefunction looks good.The function is well-documented and provides clear usage examples.
api/src/pdc/services/export/models/Export.ts (3)
7-9: Add documentation for newExportStatusvalues.The
ExportStatusenum now includesUPLOADING,UPLOADED, andNOTIFY. Adding comments to explain each status will improve code readability and maintainability.export enum ExportStatus { PENDING = "pending", RUNNING = "running", UPLOADING = "uploading", // File is being uploaded UPLOADED = "uploaded", // File has been uploaded NOTIFY = "notify", // Notify recipients SUCCESS = "success", FAILURE = "failure", }
19-21: Clarify the new type definitions with comments.Adding comments to the new
ExportErrorandExportStatstype definitions will clarify their intended use.export type ExportError = string; // Represents errors as a JSON string export type ExportStats = string; // Represents statistics as a JSON string
32-33: Clarify the usage ofExportErrorandExportStats.The
errorandstatsproperties are JSON objects represented as strings. Adding comments will improve code clarity.public error: ExportError; // JSON string representing errors public stats: ExportStats; // JSON string representing statisticsapi/src/pdc/services/export/services/FileCreatorService.ts (2)
14-14: Update the return type in the interface documentation.The
writemethod's return type has been updated toPromise<string>. Ensure the interface documentation reflects this change.export type FileCreatorServiceInterface = { write( params: ExportParams, fileWriter: XLSXWriter, progress?: ExportProgress, ): Promise<string>; // Returns the file path as a string };
38-38: Update the return type in the abstract class documentation.The
writemethod's return type has been updated toPromise<string>. Ensure the abstract class documentation reflects this change.public async write( params: ExportParams, fileWriter: XLSXWriter, progress?: ExportProgress, ): Promise<string> { throw new Error("Not implemented"); }api/src/pdc/services/export/actions/CreateActionV3.ts (1)
34-36: Ensure consistency in middleware usage.The
copyFromContextMiddlewareparameters have been updated. Verify that these changes are consistent with the expected behavior across the codebase.api/src/pdc/services/export/ServiceProvider.ts (1)
8-8: Update the import path in documentation.The import path for
DefaultTimezoneMiddlewarehas been updated. Ensure that related documentation and comments reflect this change.import { DefaultTimezoneMiddleware } from "@/pdc/middlewares/DefaultTimezoneMiddleware.ts";api/src/pdc/services/export/models/ExportParams.ts (1)
51-52: Ensurestart_atandend_atare Date objectsThe changes ensure that the
start_atandend_atproperties are alwaysDateobjects, which is a good practice for consistency.However, verify the usage of the
normalizemethod throughout the codebase to ensure there are no assumptions about the type ofstart_atandend_at.api/src/pdc/services/export/commands/ProcessCommand.ts (3)
33-35: Initialize storage serviceThe storage service is initialized at the beginning of the
callmethod, ensuring it is ready for use.
67-87: Handle export process stepsThe
processmethod handles the export process steps, including generating files, uploading to storage, and notifying users. The steps are well-structured and ensure proper status updates and error handling.
7-8: IncludeNotificationServiceandStorageServiceThe inclusion of
NotificationServiceandStorageServiceenhances the functionality of the export process by adding notifications and storage handling.However, verify the usage of
NotificationServiceandStorageServicethroughout the codebase to ensure they are correctly integrated.api/src/pdc/services/apdf/commands/ExportCommand.ts (1)
64-64: Clarify verbose option descriptionThe change to the description of the
--verboseoption improves clarity by specifying the use oflogger.info().api/src/pdc/providers/middleware/ValidateDate/ValidateDateMiddleware.ts (2)
22-22: UseNextFunctionin method signatureThe change to use
NextFunctionin theprocessmethod signature aligns with best practices for middleware.
95-120: Add detailedvalidateDateMiddlewarefunctionThe new
validateDateMiddlewarefunction provides a clear and detailed way to configure the middleware, with examples and descriptions.However, verify the usage of the
validateDateMiddlewarefunction throughout the codebase to ensure it is correctly integrated.Verification successful
Integration of
validateDateMiddlewareverifiedThe
validateDateMiddlewarefunction is correctly integrated and used consistently across the codebase in the following files:
api/src/pdc/services/trip/actions/ExportAction.tsapi/src/pdc/services/trip/actions/StatsAction.tsapi/src/pdc/services/trip/actions/ListTripsAction.tsapi/src/pdc/services/trip/actions/SearchCountAction.tsapi/src/pdc/services/trip/actions/FinancialStatsAction.tsapi/src/pdc/services/export/actions/CreateActionV3.tsapi/src/pdc/providers/middleware/bindings.tsapi/src/pdc/providers/middleware/ValidateDate/ValidateDateMiddleware.tsThese integrations align with the documented examples and intended usage.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `validateDateMiddleware` function. # Test: Search for the usage of `validateDateMiddleware`. Expect: Only occurances of the new function. rg --type ts $'validateDateMiddleware'Length of output: 1671
Script:
#!/bin/bash # Description: Check the context of `validateDateMiddleware` usage in the identified files. # Display lines around `validateDateMiddleware` usage in ExportAction.ts echo "ExportAction.ts:" rg --context 5 'validateDateMiddleware' api/src/pdc/services/trip/actions/ExportAction.ts # Display lines around `validateDateMiddleware` usage in StatsAction.ts echo "StatsAction.ts:" rg --context 5 'validateDateMiddleware' api/src/pdc/services/trip/actions/StatsAction.ts # Display lines around `validateDateMiddleware` usage in ListTripsAction.ts echo "ListTripsAction.ts:" rg --context 5 'validateDateMiddleware' api/src/pdc/services/trip/actions/ListTripsAction.ts # Display lines around `validateDateMiddleware` usage in SearchCountAction.ts echo "SearchCountAction.ts:" rg --context 5 'validateDateMiddleware' api/src/pdc/services/trip/actions/SearchCountAction.ts # Display lines around `validateDateMiddleware` usage in FinancialStatsAction.ts echo "FinancialStatsAction.ts:" rg --context 5 'validateDateMiddleware' api/src/pdc/services/trip/actions/FinancialStatsAction.ts # Display lines around `validateDateMiddleware` usage in CreateActionV3.ts echo "CreateActionV3.ts:" rg --context 5 'validateDateMiddleware' api/src/pdc/services/export/actions/CreateActionV3.ts # Display lines around `validateDateMiddlewareBinding` usage in bindings.ts echo "bindings.ts:" rg --context 5 'validateDateMiddlewareBinding' api/src/pdc/providers/middleware/bindings.ts # Display lines around `validateDateMiddleware` usage in ValidateDateMiddleware.ts echo "ValidateDateMiddleware.ts:" rg --context 5 'validateDateMiddleware' api/src/pdc/providers/middleware/ValidateDate/ValidateDateMiddleware.tsLength of output: 7755
api/src/pdc/services/export/services/NotificationService.ts (1)
93-103: LGTM!The method is correctly implemented and efficient.
api/src/pdc/services/export/services/LogService.ts (1)
112-112: LGTM!The method is correctly implemented and efficient.
api/src/pdc/providers/notification/NotificationMailTransporter.ts (2)
54-63: LGTM!The method is correctly implemented and efficient.
87-91: LGTM!The method is correctly implemented and efficient.
api/src/pdc/services/user/providers/UserNotificationProvider.ts (1)
24-24: Verify the addition of ExportCSVSupportNotification.Ensure that the
ExportCSVSupportNotificationis correctly implemented and used within the codebase.Also applies to: 57-57
Verification successful
Verification successful for ExportCSVSupportNotification.
The
ExportCSVSupportNotificationclass is correctly implemented and used within the codebase.
- Implementation:
api/src/pdc/services/user/notifications/ExportCSVSupportNotification.ts- Usages:
api/src/pdc/services/user/providers/UserNotificationProvider.tsapi/src/pdc/services/user/notifications/index.tsapi/src/pdc/services/export/services/NotificationService.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation and usage of ExportCSVSupportNotification. # Test: Search for the class implementation. Expect: One occurrence. rg --type ts $'class ExportCSVSupportNotification' # Test: Search for the usage of ExportCSVSupportNotification. Expect: Multiple occurrences. rg --type ts $'ExportCSVSupportNotification'Length of output: 1022
api/src/pdc/services/export/repositories/ExportRepository.ts (1)
26-26: Verify the addition of recipients method.Ensure that the
recipientsmethod is correctly implemented and used within the codebase.Also applies to: 149-156, 304-311
api/src/pdc/services/export/actions/CreateAction.integration.spec.ts (1)
16-21: Verify the addition of tests for export functionality.Ensure that the newly added tests cover all scenarios and edge cases for export functionality.
Also applies to: 32-36, 50-76, 92-109, 117-165, 167-198, 200-225, 227-248, 250-280, 285-315
| template: string; | ||
| to: string; | ||
| data: { [k: string]: any }; | ||
| data: T; | ||
| } | ||
|
|
||
| export type ResultInterface = void; |
There was a problem hiding this comment.
Replace void with undefined for clarity.
Using void outside of return types or type parameters can be confusing. Replace it with undefined for better clarity.
-export type ResultInterface = void;
+export type ResultInterface = undefined;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type ResultInterface = void; | |
| export type ResultInterface = undefined; |
Tools
Biome
[error] 7-7: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
| ): Promise<string> { | ||
| try { | ||
| await this.configure(params, fileWriter, progress); | ||
| await this.initialize(); | ||
| await this.data(); | ||
| await this.help(); | ||
| await this.wrap(); | ||
| await this.fileWriter.close(); | ||
| await this.fileWriter.compress(); | ||
|
|
||
| logger.info(`File written to ${this.fileWriter.workbookPath}`); | ||
|
|
||
| return this.fileWriter.workbookPath; | ||
| } catch (e) { | ||
| await this.wrap(e); | ||
| await this.fileWriter.close(); | ||
| throw e; |
There was a problem hiding this comment.
Improve error handling and logging.
The write method should log the error before re-throwing it to provide better debugging information.
public async write(
params: ExportParams,
fileWriter: XLSXWriter,
progress?: ExportProgress,
): Promise<string> {
try {
await this.configure(params, fileWriter, progress);
await this.initialize();
await this.data();
await this.help();
await this.fileWriter.close();
await this.fileWriter.compress();
logger.info(`File written to ${this.fileWriter.workbookPath}`);
return this.fileWriter.workbookPath;
} catch (e) {
logger.error(`Error writing file: ${e.message}`, e);
await this.fileWriter.close();
throw e;
}
}| public async error(exp: Export): Promise<void> { | ||
| const recipients = await this.recipients(exp); | ||
| for (const { email, fullname } of recipients) { | ||
| await this.notify({ | ||
| template: "ExportCSVErrorNotification", | ||
| to: `${fullname} <${email}>`, | ||
| data: { fullname }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add error handling for external calls.
The notify method call should be wrapped in a try-catch block to handle potential errors.
+ try {
const recipients = await this.recipients(exp);
for (const { email, fullname } of recipients) {
await this.notify({
template: "ExportCSVErrorNotification",
to: `${fullname} <${email}>`,
data: { fullname },
});
}
+ } catch (error) {
+ logger.error(`Failed to send error notification: ${error.message}`);
+ throw error;
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async error(exp: Export): Promise<void> { | |
| const recipients = await this.recipients(exp); | |
| for (const { email, fullname } of recipients) { | |
| await this.notify({ | |
| template: "ExportCSVErrorNotification", | |
| to: `${fullname} <${email}>`, | |
| data: { fullname }, | |
| }); | |
| } | |
| public async error(exp: Export): Promise<void> { | |
| try { | |
| const recipients = await this.recipients(exp); | |
| for (const { email, fullname } of recipients) { | |
| await this.notify({ | |
| template: "ExportCSVErrorNotification", | |
| to: `${fullname} <${email}>`, | |
| data: { fullname }, | |
| }); | |
| } | |
| } catch (error) { | |
| logger.error(`Failed to send error notification: ${error.message}`); | |
| throw error; | |
| } |
| public async success(exp: Export, url: string): Promise<void> { | ||
| const recipients = await this.recipients(exp); | ||
| for (const { email, fullname } of recipients) { | ||
| await this.notify({ | ||
| template: "ExportCSVNotification", | ||
| to: `${fullname} <${email}>`, | ||
| data: { fullname, action_href: url }, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for external calls.
The notify method call should be wrapped in a try-catch block to handle potential errors.
+ try {
const recipients = await this.recipients(exp);
for (const { email, fullname } of recipients) {
await this.notify({
template: "ExportCSVNotification",
to: `${fullname} <${email}>`,
data: { fullname, action_href: url },
});
}
+ } catch (error) {
+ logger.error(`Failed to send success notification: ${error.message}`);
+ throw error;
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async success(exp: Export, url: string): Promise<void> { | |
| const recipients = await this.recipients(exp); | |
| for (const { email, fullname } of recipients) { | |
| await this.notify({ | |
| template: "ExportCSVNotification", | |
| to: `${fullname} <${email}>`, | |
| data: { fullname, action_href: url }, | |
| }); | |
| } | |
| } | |
| public async success(exp: Export, url: string): Promise<void> { | |
| try { | |
| const recipients = await this.recipients(exp); | |
| for (const { email, fullname } of recipients) { | |
| await this.notify({ | |
| template: "ExportCSVNotification", | |
| to: `${fullname} <${email}>`, | |
| data: { fullname, action_href: url }, | |
| }); | |
| } | |
| } catch (error) { | |
| logger.error(`Failed to send success notification: ${error.message}`); | |
| throw error; | |
| } | |
| } |
| public async support(exp: Export): Promise<void> { | ||
| const { email, fullname } = support; | ||
| await this.notify<ExportCSVSupportTemplateData>({ | ||
| template: "ExportCSVSupportNotification", | ||
| to: `${fullname} <${email}>`, | ||
| data: { ...exp, error: exp.error }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add error handling for external calls.
The notify method call should be wrapped in a try-catch block to handle potential errors.
+ try {
const { email, fullname } = support;
await this.notify<ExportCSVSupportTemplateData>({
template: "ExportCSVSupportNotification",
to: `${fullname} <${email}>`,
data: { ...exp, error: exp.error },
});
+ } catch (error) {
+ logger.error(`Failed to notify support: ${error.message}`);
+ throw error;
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async support(exp: Export): Promise<void> { | |
| const { email, fullname } = support; | |
| await this.notify<ExportCSVSupportTemplateData>({ | |
| template: "ExportCSVSupportNotification", | |
| to: `${fullname} <${email}>`, | |
| data: { ...exp, error: exp.error }, | |
| }); | |
| } | |
| public async support(exp: Export): Promise<void> { | |
| try { | |
| const { email, fullname } = support; | |
| await this.notify<ExportCSVSupportTemplateData>({ | |
| template: "ExportCSVSupportNotification", | |
| to: `${fullname} <${email}>`, | |
| data: { ...exp, error: exp.error }, | |
| }); | |
| } catch (error) { | |
| logger.error(`Failed to notify support: ${error.message}`); | |
| throw error; | |
| } | |
| } |
| const smtp = this.config.get("notification.mail.smtp"); | ||
| this.transporter = mailer.createTransport(smtp); | ||
| if (verify) { | ||
| try { | ||
| await this.transporter.verify(); | ||
| } catch (e) { | ||
| logger.error("Failed to connect to SMTP server"); | ||
| logger.error("Failed to connect to SMTP server", e.message); | ||
| exit(1); | ||
| } |
There was a problem hiding this comment.
Add error handling for external calls.
The verify method call should be wrapped in a try-catch block to handle potential errors.
+ try {
const smtp = this.config.get("notification.mail.smtp");
this.transporter = mailer.createTransport(smtp);
if (verify) {
try {
await this.transporter.verify();
} catch (e) {
logger.error("Failed to connect to SMTP server", e.message);
exit(1);
}
}
+ } catch (error) {
+ logger.error(`Failed to create transport: ${error.message}`);
+ throw error;
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const smtp = this.config.get("notification.mail.smtp"); | |
| this.transporter = mailer.createTransport(smtp); | |
| if (verify) { | |
| try { | |
| await this.transporter.verify(); | |
| } catch (e) { | |
| logger.error("Failed to connect to SMTP server"); | |
| logger.error("Failed to connect to SMTP server", e.message); | |
| exit(1); | |
| } | |
| try { | |
| const smtp = this.config.get("notification.mail.smtp"); | |
| this.transporter = mailer.createTransport(smtp); | |
| if (verify) { | |
| try { | |
| await this.transporter.verify(); | |
| } catch (e) { | |
| logger.error("Failed to connect to SMTP server", e.message); | |
| exit(1); | |
| } | |
| } | |
| } catch (error) { | |
| logger.error(`Failed to create transport: ${error.message}`); | |
| throw error; | |
| } |
| function dispatchPermissionsFromMatrix( | ||
| permissionsObject: Record<string, string[]>, | ||
| ) { | ||
| const permissionsByGroup: Record<string, string[]> = { | ||
| common: [], | ||
| "territory.demo": [], | ||
| "territory.user": [], | ||
| "territory.admin": [], | ||
| "operator.user": [], | ||
| "operator.application": [], | ||
| "operator.admin": [], | ||
| "registry.user": [], | ||
| "registry.admin": [], | ||
| }; | ||
|
|
||
| for (const permissionName of Reflect.ownKeys(permissionsObject) as string[]) { | ||
| const permissionRoles = permissionsObject[permissionName]; | ||
|
|
||
| for (const permissionRole of permissionRoles) { | ||
| const group = permissionRole.split(".")[0]; | ||
| permissionsByGroup[permissionRole].push( | ||
| scopeToGroup(permissionName, group), | ||
| ); | ||
| } | ||
| } | ||
| // prefix common | ||
| return permissionsByGroup; | ||
| } |
There was a problem hiding this comment.
Check for undefined properties.
Ensure that permissionsByGroup[permissionRole] is defined before pushing to it.
for (const permissionRole of permissionRoles) {
const group = permissionRole.split(".")[0];
+ if (!permissionsByGroup[permissionRole]) {
+ permissionsByGroup[permissionRole] = [];
+ }
permissionsByGroup[permissionRole].push(
scopeToGroup(permissionName, group),
);
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function dispatchPermissionsFromMatrix( | |
| permissionsObject: Record<string, string[]>, | |
| ) { | |
| const permissionsByGroup: Record<string, string[]> = { | |
| common: [], | |
| "territory.demo": [], | |
| "territory.user": [], | |
| "territory.admin": [], | |
| "operator.user": [], | |
| "operator.application": [], | |
| "operator.admin": [], | |
| "registry.user": [], | |
| "registry.admin": [], | |
| }; | |
| for (const permissionName of Reflect.ownKeys(permissionsObject) as string[]) { | |
| const permissionRoles = permissionsObject[permissionName]; | |
| for (const permissionRole of permissionRoles) { | |
| const group = permissionRole.split(".")[0]; | |
| permissionsByGroup[permissionRole].push( | |
| scopeToGroup(permissionName, group), | |
| ); | |
| } | |
| } | |
| // prefix common | |
| return permissionsByGroup; | |
| } | |
| function dispatchPermissionsFromMatrix( | |
| permissionsObject: Record<string, string[]>, | |
| ) { | |
| const permissionsByGroup: Record<string, string[]> = { | |
| common: [], | |
| "territory.demo": [], | |
| "territory.user": [], | |
| "territory.admin": [], | |
| "operator.user": [], | |
| "operator.application": [], | |
| "operator.admin": [], | |
| "registry.user": [], | |
| "registry.admin": [], | |
| }; | |
| for (const permissionName of Reflect.ownKeys(permissionsObject) as string[]) { | |
| const permissionRoles = permissionsObject[permissionName]; | |
| for (const permissionRole of permissionRoles) { | |
| const group = permissionRole.split(".")[0]; | |
| if (!permissionsByGroup[permissionRole]) { | |
| permissionsByGroup[permissionRole] = []; | |
| } | |
| permissionsByGroup[permissionRole].push( | |
| scopeToGroup(permissionName, group), | |
| ); | |
| } | |
| } | |
| // prefix common | |
| return permissionsByGroup; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- api/src/pdc/services/export/services/FileCreatorService.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- api/src/pdc/services/export/services/FileCreatorService.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- api/src/ilos/connection-postgres/PostgresConnection.ts (1 hunks)
Additional comments not posted (1)
api/src/ilos/connection-postgres/PostgresConnection.ts (1)
45-48: Logging level change frominfotodebugis appropriate.The change from
infotodebugfor theremoveevent logging is appropriate as it reduces verbosity in production logs and is more suitable for debugging purposes.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
FileCreatorServiceto return a file path instead of void, simplifying file handling logic.Chores