feat: create report modal (BAL-3607) #3052
Conversation
|
WalkthroughThis pull request introduces a series of changes across both the front-end and back-end. Updates include modifications of customer configuration properties used for conditional rendering, new components and hooks for business and merchant reporting functions, and extensive refactoring of import paths for the MerchantMonitoringClient. Additionally, enhancements have been made to debugging aids and schema validations, and a new MerchantMonitoringModule has been integrated into the application's module system. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant P as MerchantMonitoringPage
participant D as CreateMerchantReportDialog
participant H as useCreateMerchantReportDialogLogic
participant M as useCreateBusinessReportMutation
U->>P: Click "Create Report" Button
P->>D: Open Dialog
D->>H: Initialize form state and metrics
U->>D: Fill and submit form
D->>H: Handle form submission
H->>M: Trigger report creation mutation
M-->>H: Return success or error
H-->>D: Update state (show success/error)
D-->>U: Display corresponding message
Possibly related PRs
Poem
✨ Finishing Touches
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
services/workflows-service/src/global.d.ts (1)
15-35: 🛠️ Refactor suggestionReduce usage of
anytype.The extensive use of
anytype in these declarations bypasses TypeScript's type checking, which could lead to runtime errors.Consider:
- Using more specific types instead of
any- If the type is truly unknown, use
unknownas it's safer thanany- If the type is complex, consider creating a proper type definition
Example for the
WorkflowRuntimeDatatype:export type WorkflowRuntimeData = Omit<_WorkflowRuntimeData, 'context'> & { - context: any; + context: unknown; - config: WorkflowConfig | any; + config: WorkflowConfig | Record<string, unknown>; };
🧹 Nitpick comments (15)
services/workflows-service/src/customer/customer.controller.external.ts (1)
84-86: UseNotFoundExceptioninstead ofBadRequestExceptionfor absent resources.
ThrowingBadRequestExceptionimplies invalid request data, whereas an absent resource typically meritsNotFoundExceptionfor clarity and proper HTTP status semantics.- throw new BadRequestException('Customer not found'); + throw new NotFoundException('Customer not found');services/workflows-service/src/customer/customer.module.ts (1)
1-1: Verify necessity offorwardRef; otherwise, remove it.
The module importsforwardRefalongsideMerchantMonitoringModule, butforwardRefdoes not appear to be used. If there is no cyclical dependency, you can safely remove it. In all other respects, addingMerchantMonitoringModuleto imports is well-aligned with your modular approach.-import { forwardRef, Module } from '@nestjs/common'; +import { Module } from '@nestjs/common';Also applies to: 10-10, 13-13
apps/backoffice-v2/src/domains/business-reports/components/BusinessReportsLeftCard/BusinessReportsLeftCard.tsx (3)
5-9: Enhance props validation.Consider adding validation for the numeric values:
export type BusinessReportsLeftCardProps = { - reportsLeft: number | null | undefined; + reportsLeft: number | null | undefined; + /** @invariant When defined, must be >= 0 */ - demoDaysLeft: number | null | undefined; + demoDaysLeft: number | null | undefined; + /** @invariant When defined, must be >= 0 */ className?: string; };
25-25: Extract state determination logic.The state determination logic could be moved to a separate function for better maintainability.
+const getCardState = (demoDaysLeft: number, reportsLeft: number) => { + if (demoDaysLeft <= 0) return 'expired'; + if (reportsLeft <= 0) return 'noReports'; + return 'active'; +}; -const state = demoDaysLeft <= 0 ? 'expired' : reportsLeft <= 0 ? 'noReports' : 'active'; +const state = getCardState(demoDaysLeft, reportsLeft);
27-36: Consider externalizing messages.Moving messages to a separate constant would improve maintainability and make it easier to add translations later.
+const CARD_MESSAGES: Record<string, (props: { reportsLeft: number; demoDaysLeft: number }) => ReactNode> = { + expired: () => <span className="text-destructive">Your demo account has expired!</span>, + noReports: () => <span className="text-destructive">You don't have any reports left!</span>, + active: ({ reportsLeft, demoDaysLeft }) => ( + <span> + You have <span className="font-bold">{reportsLeft} free reports</span> left to create, + available for <span className="font-bold">{demoDaysLeft} days.</span> + </span> + ), +}; -const messages: Record<string, ReactNode> = { - expired: <span className="text-destructive">Your demo account has expired!</span>, - noReports: <span className="text-destructive">You don't have any reports left!</span>, - active: ( - <span> - You have <span className="font-bold">{reportsLeft} free reports</span> left to create, - available for <span className="font-bold">{demoDaysLeft} days.</span> - </span> - ), -};services/workflows-service/src/business-report/business-report.module.ts (1)
7-15: Address circular dependencies in the module system.The presence of multiple circular dependencies (marked with eslint-disable comments) suggests a need for architectural refactoring.
Consider:
- Extracting shared functionality into a separate module
- Using event-based communication between modules
- Implementing the mediator pattern to decouple modules
- Creating interfaces to break circular dependencies
apps/backoffice-v2/src/domains/business-reports/hooks/mutations/useCreateBusinessReportMutation/useCreateBusinessReportMutation.tsx (2)
9-14: Consider adding JSDoc documentation for the options parameter.The function accepts an options object with multiple properties but lacks documentation explaining their purpose.
Add JSDoc:
+/** + * Hook for creating business reports with customizable success handling and toast notifications. + * @param options - Configuration options + * @param options.onSuccess - Callback function called after successful mutation + * @param options.disableToast - Flag to disable success toast notification + */ export const useCreateBusinessReportMutation = (options?: { onSuccess?: <TData>(data: TData) => void; disableToast?: boolean; }) => {
43-54: Consider adding a loading toast for better UX.The mutation shows success/error toasts but doesn't indicate when it's in progress.
Add loading state notification:
onSuccess: data => { if (customer?.config?.isExample) { return; } void queryClient.invalidateQueries(); if (!disableToast) { + toast.dismiss('creating-report'); toast.success(t(`toast:business_report_creation.success`)); } onSuccess?.(data); }, + onMutate: () => { + if (!disableToast) { + toast.loading('Creating report...', { id: 'creating-report' }); + } + },apps/backoffice-v2/src/common/components/molecules/GetFullAccessCard/GetFullAccessCard.tsx (1)
15-21: Consider adding loading state UI.The component returns null during loading, which might cause layout shifts. Consider showing a skeleton or placeholder during loading.
Add loading state:
if ( env.VITE_ENVIRONMENT_NAME === 'production' || - isLoading || !customer?.config?.demoAccessDetails?.showFullAccessPopup ) { return null; } + if (isLoading) { + return <div className="fixed bottom-8 right-8 h-[200px] w-[448px] animate-pulse rounded-md bg-gray-200" />; + }services/workflows-service/src/customer/customer.service.ts (2)
24-42: Consider making demo access configuration values configurable.The method contains hardcoded values that should be configurable:
- Default max business reports (10)
- Demo expiration period (14 days)
Consider moving these values to environment variables or configuration:
+import { env } from '@/env'; + async getDemoAccessDetails(customer: TCustomerWithFeatures): Promise<TDemoAccessDetails | null> { const { id: customerId, config, createdAt } = customer; if (!config || !config.isDemoAccount || !createdAt) { return null; } const businessReportsCount = await this.merchantMonitoringClient.count({ customerId, }); return { totalReports: businessReportsCount, - maxBusinessReports: config.maxBusinessReports ?? 10, + maxBusinessReports: config.maxBusinessReports ?? env.DEFAULT_MAX_BUSINESS_REPORTS ?? 10, showFullAccessPopup: config.showFullAccessPopup ?? false, // FIXME: should count from the first login date / should be passed on creation ? - expiresAt: dayjs(createdAt).add(14, 'days').unix(), + expiresAt: dayjs(createdAt).add(env.DEMO_EXPIRATION_DAYS ?? 14, 'days').unix(), }; }
39-40: Address the FIXME comment regarding expiration date calculation.The comment indicates uncertainty about when to start counting the expiration period.
Would you like me to:
- Generate a solution that tracks the first login date?
- Open an issue to track this requirement?
apps/backoffice-v2/src/pages/MerchantMonitoring/schemas.ts (1)
133-134: Consider using a more robust URL validation approach.The current regex might miss some edge cases in URL validation.
Consider using a dedicated URL validation library or the built-in URL constructor:
-const URL_REGEX = /((https?):\/\/)?([a-zA-Z0-9-_]+\.)+[a-zA-Z0-9]+(\.[a-z]{2})?(\/[a-zA-Z0-9_#-]+)*(\/)?(\?[a-zA-Z0-9_-]+=[a-zA-Z0-9_-]+(&[a-zA-Z0-9_-]+=[a-zA-Z0-9_-]+)*)?(#[a-zA-Z0-9_-]+)?/; +const isValidUrl = (url: string) => { + try { + new URL(url.startsWith('http') ? url : `https://${url}`); + return true; + } catch { + return false; + } +};Then update the schema:
export const CreateBusinessReportDialogSchema = z.object({ - websiteUrl: z.string().regex(URL_REGEX, { - message: 'Invalid website URL', - }), + websiteUrl: z.string().refine(isValidUrl, { + message: 'Invalid website URL', + }), // ...rest of the schema });apps/backoffice-v2/src/pages/MerchantMonitoring/components/CreateMerchantReportDialog/CreateMerchantReportDialog.tsx (2)
81-82: Consider adding a comment explaining the disable condition.The disable condition combines two checks but lacks documentation explaining the business logic behind it.
+ // Disable form when either reports quota is exhausted or demo period has ended const shouldDisableForm = (reportsLeft && reportsLeft <= 0) || (demoDaysLeft && demoDaysLeft <= 0);
62-65: Consider making the completion time configurable.The "24 hours" completion time is hardcoded. Consider making it configurable through props or environment variables for flexibility.
+ type CreateMerchantReportDialogProps = { + children: React.ReactNode; + estimatedCompletionHours?: number; + }; // In the success content - <p className="font-semibold">Ready in up to 24 hours</p> + <p className="font-semibold">Ready in up to {estimatedCompletionHours ?? 24} hours</p>apps/backoffice-v2/src/pages/MerchantMonitoring/MerchantMonitoring.page.tsx (1)
103-121: Refactor duplicate button code.The button code is duplicated between the enabled and disabled states. Consider extracting it to a reusable component.
+ const CreateReportButton = ({ disabled }: { disabled: boolean }) => ( + <Button + className={`flex items-center gap-2 bg-purple-600 font-semibold text-white hover:bg-purple-500 aria-disabled:pointer-events-none aria-disabled:opacity-50`} + aria-disabled={disabled} + > + <Plus /> + <span>Create a report</span> + </Button> + ); {createBusinessReport?.enabled ? ( <CreateMerchantReportDialog> - <Button - className={`flex items-center gap-2 bg-purple-600 font-semibold text-white hover:bg-purple-500 aria-disabled:pointer-events-none aria-disabled:opacity-50`} - aria-disabled={!createBusinessReport?.enabled} - > - <Plus /> - <span>Create a report</span> - </Button> + <CreateReportButton disabled={!createBusinessReport?.enabled} /> </CreateMerchantReportDialog> ) : ( - <Button - className={`flex items-center gap-2 bg-purple-600 font-semibold text-white hover:bg-purple-500 aria-disabled:pointer-events-none aria-disabled:opacity-50`} - aria-disabled={!createBusinessReport?.enabled} - > - <Plus /> - <span>Create a report</span> - </Button> + <CreateReportButton disabled={!createBusinessReport?.enabled} /> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
apps/backoffice-v2/src/common/components/molecules/GetFullAccessCard/GetFullAccessCard.tsx(1 hunks)apps/backoffice-v2/src/common/components/organisms/Dialog/Dialog.Content.tsx(1 hunks)apps/backoffice-v2/src/domains/business-reports/components/BusinessReportsLeftCard/BusinessReportsLeftCard.tsx(1 hunks)apps/backoffice-v2/src/domains/business-reports/hooks/mutations/useCreateBusinessReportMutation/useCreateBusinessReportMutation.tsx(3 hunks)apps/backoffice-v2/src/domains/customer/fetchers.ts(1 hunks)apps/backoffice-v2/src/pages/MerchantMonitoring/MerchantMonitoring.page.tsx(3 hunks)apps/backoffice-v2/src/pages/MerchantMonitoring/components/CreateMerchantReportDialog/CreateMerchantReportDialog.tsx(1 hunks)apps/backoffice-v2/src/pages/MerchantMonitoring/components/CreateMerchantReportDialog/hooks/useCreateMerchantReportDialogLogic.tsx(1 hunks)apps/backoffice-v2/src/pages/MerchantMonitoring/hooks/useMerchantMonitoringLogic/useMerchantMonitoringLogic.tsx(1 hunks)apps/backoffice-v2/src/pages/MerchantMonitoring/schemas.ts(1 hunks)services/workflows-service/src/alert/alert.service.intg.test.ts(1 hunks)services/workflows-service/src/app.module.ts(2 hunks)services/workflows-service/src/business-report/business-report.controller.external.ts(1 hunks)services/workflows-service/src/business-report/business-report.module.ts(1 hunks)services/workflows-service/src/business-report/business-report.service.ts(1 hunks)services/workflows-service/src/collection-flow/collection-flow.service.intg.test.ts(1 hunks)services/workflows-service/src/collection-flow/controllers/collection-flow.no-user.controller.intg.test.ts(1 hunks)services/workflows-service/src/common/guards/token-guard/token-auth.module.ts(1 hunks)services/workflows-service/src/customer/customer.controller.external.ts(3 hunks)services/workflows-service/src/customer/customer.module.ts(2 hunks)services/workflows-service/src/customer/customer.service.ts(2 hunks)services/workflows-service/src/customer/schemas/zod-schemas.ts(1 hunks)services/workflows-service/src/end-user/end-user.controller.external.intg.test.ts(1 hunks)services/workflows-service/src/global.d.ts(1 hunks)services/workflows-service/src/merchant-monitoring/merchant-monitoring.module.ts(1 hunks)services/workflows-service/src/workflow-defintion/workflow-definition.service.intg.test.ts(2 hunks)services/workflows-service/src/workflow/workflow-runtime-data.repository.intg.test.ts(1 hunks)services/workflows-service/src/workflow/workflow.controller.external.intg.test.ts(1 hunks)services/workflows-service/src/workflow/workflow.controller.internal.intg.test.ts(1 hunks)services/workflows-service/src/workflow/workflow.service.intg.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (11)
- apps/backoffice-v2/src/common/components/organisms/Dialog/Dialog.Content.tsx
- services/workflows-service/src/end-user/end-user.controller.external.intg.test.ts
- services/workflows-service/src/workflow/workflow-runtime-data.repository.intg.test.ts
- services/workflows-service/src/collection-flow/collection-flow.service.intg.test.ts
- services/workflows-service/src/workflow/workflow.controller.internal.intg.test.ts
- services/workflows-service/src/business-report/business-report.service.ts
- services/workflows-service/src/business-report/business-report.controller.external.ts
- services/workflows-service/src/workflow/workflow.service.intg.test.ts
- services/workflows-service/src/collection-flow/controllers/collection-flow.no-user.controller.intg.test.ts
- services/workflows-service/src/workflow/workflow.controller.external.intg.test.ts
- services/workflows-service/src/alert/alert.service.intg.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: build (windows-latest)
- GitHub Check: lint
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze (javascript)
- GitHub Check: format
🔇 Additional comments (16)
services/workflows-service/src/common/guards/token-guard/token-auth.module.ts (1)
13-13:✅ Verification successful
LGTM! Verify dependency injection order.
The addition of
MerchantMonitoringClientto theTokenAuthModulealigns with the PR objectives. The import and provider registration look correct.Let's verify there are no dependency injection issues with the new client:
Also applies to: 17-17
🏁 Script executed:
#!/bin/bash # Description: Check for potential circular dependencies and verify the client's dependencies # Search for MerchantMonitoringClient dependencies ast-grep --pattern 'class MerchantMonitoringClient { constructor($_) { $$$ } }' # Search for any services that might depend on MerchantMonitoringClient rg "MerchantMonitoringClient" -A 5Length of output: 22407
LGTM! The addition of
MerchantMonitoringClientto theTokenAuthModulenot only aligns with the PR objectives but also has been verified to not introduce any dependency injection issues. The search results confirm that there are no circular dependency concerns and that the provider registration order is correct.
- Verified that the import and providers addition follow the existing structure.
- No dependency injection issues were found based on the search output.
services/workflows-service/src/customer/customer.controller.external.ts (3)
1-1: Import ofTDemoAccessDetailsis on point.
No issues with referencingTDemoAccessDetailshere, as it aligns with the usage later in the file fordemoAccessDetailsintegration.
3-9: Expanded exception imports enhance error handling.
IncludingBadRequestExceptionandNotFoundExceptionis a good practice. Just ensure consistent usage across the controller to distinguish between invalid inputs (400) and absent resources (404).
88-97: Validateconfigbefore spreading additional properties.
Ifcustomer.configcould benullorundefined, spreading it might cause issues. Also ensure that any direct usage ofdemoAccessDetailsis guarded or defaulted to an empty object if none is found.services/workflows-service/src/merchant-monitoring/merchant-monitoring.module.ts (1)
1-9: Module structure and provider export look fine.
TheMerchantMonitoringModuleproperly encapsulates theMerchantMonitoringClientand re-exports it for usage elsewhere. Consider adding minimal tests to ensure the provider initializes correctly.services/workflows-service/src/customer/schemas/zod-schemas.ts (2)
11-11: Verify the necessity of bothisDemoandisDemoAccountproperties.The schema contains two similar boolean properties:
isDemoandisDemoAccount. This might lead to confusion about which property should be used.Consider consolidating these properties or clarifying their distinct purposes through documentation.
20-25: LGTM! Well-structured schema for demo access details.The schema effectively captures all necessary properties for managing demo access, with appropriate types and default values.
services/workflows-service/src/business-report/business-report.module.ts (1)
17-17: LGTM! Import path updated correctly.The import path for
MerchantMonitoringClienthas been properly updated to reflect its new location.apps/backoffice-v2/src/pages/MerchantMonitoring/components/CreateMerchantReportDialog/hooks/useCreateMerchantReportDialogLogic.tsx (1)
17-19: Consider adding error handling for missing customer data.The destructuring of
customer?.config?.demoAccessDetailscould result in undefined values if the customer data is not properly loaded.Add null checks or provide default values:
- const { totalReports, maxBusinessReports, expiresAt } = customer?.config?.demoAccessDetails ?? {}; + const { totalReports = 0, maxBusinessReports = 0, expiresAt = null } = customer?.config?.demoAccessDetails ?? {};services/workflows-service/src/customer/customer.service.ts (1)
86-88: LGTM! Type safety improvement.The return type change from type assertion to direct type improves type safety.
apps/backoffice-v2/src/pages/MerchantMonitoring/schemas.ts (1)
137-153: LGTM! Well-structured validation schema.The schema has:
- Clear error messages
- Appropriate max length constraints
- Optional fields where needed
services/workflows-service/src/app.module.ts (1)
94-94: LGTM! Module properly imported.The MerchantMonitoringModule is correctly added to the imports array.
apps/backoffice-v2/src/pages/MerchantMonitoring/hooks/useMerchantMonitoringLogic/useMerchantMonitoringLogic.tsx (1)
180-180: LGTM! Demo access details properly exposed.The hook correctly exposes the demo access details from the customer configuration.
apps/backoffice-v2/src/pages/MerchantMonitoring/components/CreateMerchantReportDialog/CreateMerchantReportDialog.tsx (1)
18-20: LGTM! Clean props interface and component structure.The component has a clear, minimal props interface and follows React best practices with proper type definitions.
Also applies to: 70-73
services/workflows-service/src/workflow-defintion/workflow-definition.service.intg.test.ts (1)
60-60: LGTM! Proper module integration in test setup.The MerchantMonitoringModule is correctly imported and included in the test module setup.
apps/backoffice-v2/src/pages/MerchantMonitoring/MerchantMonitoring.page.tsx (1)
88-88: LGTM! Consistent icon usage.The Plus icon is more appropriate for the create action and maintains consistency with other create actions in the UI.
| async getByCurrentProjectId(@CurrentProject() currentProjectId: TProjectId): Promise< | ||
| | (Omit<TCustomerWithFeatures, 'config'> & { | ||
| config: TCustomerWithFeatures['config'] & { demoAccessDetails: TDemoAccessDetails }; | ||
| }) | ||
| | null | ||
| > { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making demoAccessDetails optional in the return type.
The method signature currently enforces demoAccessDetails in the returned config. If demoAccessDetails is not found, the code returns the customer object without that property, creating a potential type mismatch. Make demoAccessDetails optional or guarantee it always exists.
- | (Omit<TCustomerWithFeatures, 'config'> & {
- config: TCustomerWithFeatures['config'] & { demoAccessDetails: TDemoAccessDetails };
- })
+ | (Omit<TCustomerWithFeatures, 'config'> & {
+ config: TCustomerWithFeatures['config'] & { demoAccessDetails?: TDemoAccessDetails };
+ })📝 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.
| async getByCurrentProjectId(@CurrentProject() currentProjectId: TProjectId): Promise< | |
| | (Omit<TCustomerWithFeatures, 'config'> & { | |
| config: TCustomerWithFeatures['config'] & { demoAccessDetails: TDemoAccessDetails }; | |
| }) | |
| | null | |
| > { | |
| async getByCurrentProjectId(@CurrentProject() currentProjectId: TProjectId): Promise< | |
| | (Omit<TCustomerWithFeatures, 'config'> & { | |
| config: TCustomerWithFeatures['config'] & { demoAccessDetails?: TDemoAccessDetails }; | |
| }) | |
| | null | |
| > { |
| // FIXME: this is a problem | ||
|
|
There was a problem hiding this comment.
Add context to the FIXME comment.
The FIXME comment lacks explanation about what the problem is, making it difficult for other developers to understand and address the issue.
Please elaborate on the problem and potential solutions in the comment.
| const { mutate: mutateCreateBusinessReport, isLoading: isSubmitting } = | ||
| useCreateBusinessReportMutation({ disableToast: true }); | ||
| const onSubmit: SubmitHandler<CreateBusinessReportDialogInput> = data => { | ||
| mutateCreateBusinessReport(data, { | ||
| onSuccess: () => { | ||
| setShowSuccess(true); | ||
| }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling in the onSubmit callback.
The mutation's error case is not handled in the component level, which might lead to a poor user experience if the mutation fails.
Add error handling:
const onSubmit: SubmitHandler<CreateBusinessReportDialogInput> = data => {
mutateCreateBusinessReport(data, {
onSuccess: () => {
setShowSuccess(true);
},
+ onError: () => {
+ setShowSuccess(false);
+ },
});
};📝 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 { mutate: mutateCreateBusinessReport, isLoading: isSubmitting } = | |
| useCreateBusinessReportMutation({ disableToast: true }); | |
| const onSubmit: SubmitHandler<CreateBusinessReportDialogInput> = data => { | |
| mutateCreateBusinessReport(data, { | |
| onSuccess: () => { | |
| setShowSuccess(true); | |
| }, | |
| }); | |
| }; | |
| const { mutate: mutateCreateBusinessReport, isLoading: isSubmitting } = | |
| useCreateBusinessReportMutation({ disableToast: true }); | |
| const onSubmit: SubmitHandler<CreateBusinessReportDialogInput> = data => { | |
| mutateCreateBusinessReport(data, { | |
| onSuccess: () => { | |
| setShowSuccess(true); | |
| }, | |
| onError: () => { | |
| setShowSuccess(false); | |
| }, | |
| }); | |
| }; |
| const form = useForm({ | ||
| defaultValues: { | ||
| websiteUrl: '', | ||
| companyName: undefined, | ||
| businessCorrelationId: undefined, | ||
| }, | ||
| resolver: zodResolver(CreateBusinessReportDialogSchema), | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding validation for businessCorrelationId.
The form's default values include businessCorrelationId but there's no visible validation in the schema import.
Please verify that CreateBusinessReportDialogSchema includes proper validation for all fields:
🏁 Script executed:
#!/bin/bash
# Search for the schema definition
rg -A 10 "CreateBusinessReportDialogSchema.*=.*" Length of output: 999
Action Required: Add Explicit Validation for businessCorrelationId
After examining the schema, it appears that while the field businessCorrelationId is declared in the CreateBusinessReportDialogSchema, no explicit validation rules (e.g., type constraints or pattern checks) are applied to it. Please update this field with appropriate validation (for instance, if it should be a UUID or match a specific pattern, chain the corresponding Zod methods).
- Location:
apps/backoffice-v2/src/pages/MerchantMonitoring/schemas.ts(see the partial definition forbusinessCorrelationId) - Recommendation: Extend the validation for
businessCorrelationId(e.g., usingz.string().uuid().optional()or any other validation that aligns with your business requirements).
| demoAccessDetails: z | ||
| .object({ | ||
| expiresAt: z.number().optional(), | ||
| showFullAccessPopup: z.boolean().optional(), | ||
| totalReports: z.number(), | ||
| maxBusinessReports: z.number(), | ||
| }) | ||
| .optional(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation constraints for demo access details.
The schema lacks validation for reasonable value ranges for totalReports, maxBusinessReports, and expiresAt.
Add validation constraints:
demoAccessDetails: z
.object({
- expiresAt: z.number().optional(),
+ expiresAt: z.number().min(Date.now() / 1000).optional(),
showFullAccessPopup: z.boolean().optional(),
- totalReports: z.number(),
+ totalReports: z.number().min(0),
- maxBusinessReports: z.number(),
+ maxBusinessReports: z.number().min(1),
})📝 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.
| demoAccessDetails: z | |
| .object({ | |
| expiresAt: z.number().optional(), | |
| showFullAccessPopup: z.boolean().optional(), | |
| totalReports: z.number(), | |
| maxBusinessReports: z.number(), | |
| }) | |
| .optional(), | |
| demoAccessDetails: z | |
| .object({ | |
| expiresAt: z.number().min(Date.now() / 1000).optional(), | |
| showFullAccessPopup: z.boolean().optional(), | |
| totalReports: z.number().min(0), | |
| maxBusinessReports: z.number().min(1), | |
| }) | |
| .optional(), |
| @@ -10,6 +10,8 @@ declare module '@prisma/client' { | |||
| import type { TCustomerConfig, TCustomerSubscription } from '@/customer/schemas/zod-schemas'; | |||
| export * from '@prisma/client/index'; | |||
|
|
|||
| // FIXME: this is a problem | |||
There was a problem hiding this comment.
What's the problem? :)
There was a problem hiding this comment.
maybe i'm missing something, but these types do not resolve to anything for me, i don't see them being imported. so i wonder how is this supposed to work? our types depend on these but they are literally any?
| totalReports: businessReportsCount, | ||
| maxBusinessReports: config.maxBusinessReports ?? 10, | ||
| showFullAccessPopup: config.showFullAccessPopup ?? false, | ||
| // FIXME: should count from the first login date / should be passed on creation ? |
There was a problem hiding this comment.
I recommend you to discuss it with @ArtemLavrentii, as he's working on the customer's email onboarding and signup.
We wanted it to start counting since the user was signed it first time. But we can also compromise on the customer creation date, as you did. It depends on the effort. I think Artem can help make a decision here.
There was a problem hiding this comment.
Do we expect for people to not sign in for multiple days? Creation date of a user wouldn't be when they send a request, but rather when they get the email / its get approved by the risk team.
We can also add first sign in date to a user, shouldn't be too hard to implement. I'm just unsure how useful it would be outside of demo. Maybe we could include that info in the customer config if isDemoAccount is set, wdyt ? ( we could even do it as "expiresAt" - eg just set it when user first logs in, if its not present
| }); | ||
|
|
||
| return { | ||
| totalReports: businessReportsCount, |
There was a problem hiding this comment.
Do we have a mechanism in the front-end the ensure this property gets updated when reports are added?
There was a problem hiding this comment.
And just a quick comment: Ideally, this shouldn’t be under customers and should have its own endpoint, like GET /api/business-reports/count. But since it’s just for a demo, it's not a big deal
| maxBusinessReports: config.maxBusinessReports ?? 10, | ||
| showFullAccessPopup: config.showFullAccessPopup ?? false, | ||
| // FIXME: should count from the first login date / should be passed on creation ? | ||
| expiresAt: dayjs(createdAt).add(14, 'days').unix(), |
There was a problem hiding this comment.
Let's add a new expiresAt property in the config. This way we could easily change the expiration date manually. I also suggest moving the number 14 to the env.ts file.
| async getByCurrentProjectId( | ||
| @CurrentProject() currentProjectId: TProjectId, | ||
| ): Promise<TCustomerWithFeatures | null> { | ||
| async getByCurrentProjectId(@CurrentProject() currentProjectId: TProjectId): Promise< |
There was a problem hiding this comment.
Wouldn't something like this also work ?
| async getByCurrentProjectId(@CurrentProject() currentProjectId: TProjectId): Promise< | |
| async getByCurrentProjectId(@CurrentProject() currentProjectId: TProjectId): Promise<(TCustomerWithFeatures & { config: { demoAccessDetails: TDemoAccessDetails } } ) | null> |
In either case I would create separate type for TCustomerWithFeatures with demo access details so the return type isn't as messy.
| @@ -15,7 +15,7 @@ export const GetFullAccessCard = () => { | |||
| if ( | |||
| env.VITE_ENVIRONMENT_NAME === 'production' || | |||
There was a problem hiding this comment.
We are going to create the demo accounts in production, and not sandbox this time, as this environment is more stable. Can you please remove this condition and make sure it works well on a production environment?
Context: https://teamballerine.slack.com/archives/C06MQM4F17D/p1739886859740529
| maxBusinessReports: config.maxBusinessReports ?? 10, | ||
| showFullAccessPopup: config.showFullAccessPopup ?? false, | ||
| // FIXME: should count from the first login date / should be passed on creation ? | ||
| expiresAt: dayjs(createdAt).add(14, 'days').unix(), |
There was a problem hiding this comment.
We usually use ISO format, please don't introduce new conventions (Unix timestamp) as it makes the source code confusing and inconsistent.
| const URL_REGEX = | ||
| /((https?):\/\/)?([a-zA-Z0-9-_]+\.)+[a-zA-Z0-9]+(\.[a-z]{2})?(\/[a-zA-Z0-9_#-]+)*(\/)?(\?[a-zA-Z0-9_-]+=[a-zA-Z0-9_-]+(&[a-zA-Z0-9_-]+=[a-zA-Z0-9_-]+)*)?(#[a-zA-Z0-9_-]+)?/; |
There was a problem hiding this comment.
Move it to @ballerine/common and reuse it in the WFS?
There was a problem hiding this comment.
We also use it in other places as well.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apps/backoffice-v2/src/pages/MerchantMonitoring/components/CreateMerchantReportDialog/hooks/useCreateMerchantReportDialogLogic.tsx (1)
33-39:⚠️ Potential issueAdd error handling in the onSubmit callback.
The mutation's error case is not handled in the component level.
const onSubmit: SubmitHandler<CreateBusinessReportDialogInput> = data => { mutateCreateBusinessReport(data, { onSuccess: () => { setShowSuccess(true); }, + onError: error => { + setShowSuccess(false); + // Consider adding error state management here + }, }); };apps/backoffice-v2/src/pages/MerchantMonitoring/schemas.ts (1)
131-132: 🛠️ Refactor suggestionMove URL_REGEX to @ballerine/common package.
The URL_REGEX could be reused across different parts of the application.
Consider moving this regex to
@ballerine/commonpackage for reuse in other places, including the Workflow Service.services/workflows-service/src/customer/customer.service.ts (1)
24-42: 💡 Verification agent
⚠️ Potential issue🧩 Analysis chain
Multiple improvements needed in demo access details implementation.
- The expiration date is using Unix timestamp format while the codebase convention is to use ISO format.
- The magic number
14for demo expiration days should be moved to env.ts.- The expiration calculation from creation date needs reconsideration.
Apply these improvements:
+// In env.ts +DEMO_EXPIRATION_DAYS=14 +// In customer.service.ts async getDemoAccessDetails(customer: TCustomerWithFeatures): Promise<TDemoAccessDetails | null> { const { id: customerId, config, createdAt } = customer; if (!config || !config.isDemoAccount || !createdAt) { return null; } const businessReportsCount = await this.merchantMonitoringClient.count({ customerId, }); return { totalReports: businessReportsCount, maxBusinessReports: config.maxBusinessReports ?? 10, showFullAccessPopup: config.showFullAccessPopup ?? false, - // FIXME: should count from the first login date / should be passed on creation ? - expiresAt: dayjs(createdAt).add(14, 'days').unix(), + expiresAt: dayjs(createdAt).add(env.DEMO_EXPIRATION_DAYS, 'days').toISOString(), }; }Additionally, consider:
- Adding a
firstLoginDatefield to the customer config for more accurate expiration calculation- Moving the business reports count to a dedicated endpoint for better separation of concerns
🏁 Script executed:
#!/bin/bash # Check if any other parts of the codebase use Unix timestamp format rg -l "\.unix\(\)"Length of output: 119
Multiple improvements needed in demo access details implementation
- Change the expiration date format from a Unix timestamp to ISO format to align with our project's standard.
- Replace the hardcoded magic number
14with a configurable environment variable (e.g.,env.DEMO_EXPIRATION_DAYS) to improve maintainability.- Reevaluate whether calculating the expiration based on the customer creation date is correct—consider using a
firstLoginDateif that better reflects the intended behavior.- As an additional note, our codebase’s
main.tsstill uses.unix()for date conversion. Ensure that switching to ISO formatting here won’t lead to inconsistency or unexpected behavior across the system.// In env.ts +export const DEMO_EXPIRATION_DAYS = 14; ... // In services/workflows-service/src/customer/customer.service.ts async getDemoAccessDetails(customer: TCustomerWithFeatures): Promise<TDemoAccessDetails | null> { const { id: customerId, config, createdAt } = customer; if (!config || !config.isDemoAccount || !createdAt) { return null; } const businessReportsCount = await this.merchantMonitoringClient.count({ customerId, }); return { totalReports: businessReportsCount, maxBusinessReports: config.maxBusinessReports ?? 10, showFullAccessPopup: config.showFullAccessPopup ?? false, - // FIXME: should count from the first login date / should be passed on creation ? - expiresAt: dayjs(createdAt).add(14, 'days').unix(), + // FIXME: consider using firstLoginDate for a more accurate expiration calculation + expiresAt: dayjs(createdAt).add(env.DEMO_EXPIRATION_DAYS, 'days').toISOString(), }; }
🧹 Nitpick comments (8)
apps/backoffice-v2/src/domains/business-reports/components/BusinessReportsLeftCard/BusinessReportsLeftCard.tsx (3)
16-23: Simplify null/undefined checks using nullish coalescing.The null and undefined checks can be simplified for better readability.
- if ( - reportsLeft === null || - demoDaysLeft === null || - reportsLeft === undefined || - demoDaysLeft === undefined - ) { + if (reportsLeft == null || demoDaysLeft == null) { return null; }
25-25: Improve state determination readability using descriptive variables.The state determination logic could be more readable by breaking it down into descriptive variables.
- const state = demoDaysLeft <= 0 ? 'expired' : reportsLeft <= 0 ? 'noReports' : 'active'; + const isExpired = demoDaysLeft <= 0; + const hasNoReports = reportsLeft <= 0; + const state = isExpired ? 'expired' : hasNoReports ? 'noReports' : 'active';
27-36: Extract message strings as constants.Consider extracting the message strings as constants to improve maintainability and reusability.
+const MESSAGES = { + EXPIRED: 'Your demo account has expired!', + NO_REPORTS: 'You don\'t have any reports left!', + REPORTS_LEFT: 'free reports', + DAYS_LEFT: 'days.', +} as const; const messages: Record<string, ReactNode> = { - expired: <span className="text-destructive">Your demo account has expired!</span>, + expired: <span className="text-destructive">{MESSAGES.EXPIRED}</span>, - noReports: <span className="text-destructive">You don't have any reports left!</span>, + noReports: <span className="text-destructive">{MESSAGES.NO_REPORTS}</span>, active: ( <span> - You have <span className="font-bold">{reportsLeft} free reports</span> left to create, - available for <span className="font-bold">{demoDaysLeft} days.</span> + You have <span className="font-bold">{reportsLeft} {MESSAGES.REPORTS_LEFT}</span> left to create, + available for <span className="font-bold">{demoDaysLeft} {MESSAGES.DAYS_LEFT}</span> </span> ), };apps/backoffice-v2/src/pages/MerchantMonitoring/components/CreateMerchantReportDialog/hooks/useCreateMerchantReportDialogLogic.tsx (1)
17-19: Improve data destructuring with default values.The destructuring of customer data could be more robust with default values.
- const { totalReports, maxBusinessReports, expiresAt } = customer?.config?.demoAccessDetails ?? {}; - const reportsLeft = maxBusinessReports && totalReports ? maxBusinessReports - totalReports : null; - const demoDaysLeft = expiresAt ? dayjs(expiresAt * 1000).diff(dayjs(), 'days') : null; + const { + totalReports = 0, + maxBusinessReports = 0, + expiresAt = 0 + } = customer?.config?.demoAccessDetails ?? {}; + const reportsLeft = maxBusinessReports - totalReports; + const demoDaysLeft = dayjs(expiresAt * 1000).diff(dayjs(), 'days');services/workflows-service/src/customer/types.ts (1)
66-68: Add documentation for the demo customer type.Consider adding JSDoc comments to document the purpose and structure of the
TDemoCustomertype, especially since it's a key type for demo functionality.+/** + * Represents a customer with demo access capabilities. + * Extends the base customer type by replacing the config property + * with one that includes optional demo access details. + */ export type TDemoCustomer = Omit<TCustomerWithFeatures, 'config'> & { config: { demoAccessDetails?: TDemoAccessDetails }; };services/workflows-service/src/customer/customer.service.ts (1)
86-88: Simplify type assertion in getByProjectId.The type assertion can be moved to the repository layer for better type safety.
async getByProjectId(projectId: string, args?: Omit<Prisma.CustomerFindFirstArgsBase, 'where'>) { - return (await this.repository.findByProjectId(projectId, args)) as TCustomerWithFeatures; + return await this.repository.findByProjectId(projectId, args); }services/workflows-service/src/customer/customer.controller.external.ts (2)
55-60: Update Swagger documentation for the demo customer response.The Swagger response type needs to be updated to reflect the new return type.
@common.Get('/by-current-project-id') -@swagger.ApiOkResponse({ type: [CustomerModel] }) +@swagger.ApiOkResponse({ + type: CustomerModel, + description: 'Returns customer information with demo access details if applicable' +}) @swagger.ApiForbiddenResponse() +@swagger.ApiNotFoundResponse({ description: 'Customer not found' }) async getByCurrentProjectId( @CurrentProject() currentProjectId: TProjectId, ): Promise<TDemoCustomer>
81-83: Use NotFoundException for consistency.The error handling should use NotFoundException consistently throughout the controller.
if (!customer) { - throw new BadRequestException('Customer not found'); + throw new NotFoundException('Customer not found'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
apps/backoffice-v2/src/common/components/molecules/GetFullAccessCard/GetFullAccessCard.tsx(1 hunks)apps/backoffice-v2/src/common/components/organisms/Dialog/Dialog.Content.tsx(1 hunks)apps/backoffice-v2/src/domains/business-reports/components/BusinessReportsLeftCard/BusinessReportsLeftCard.tsx(1 hunks)apps/backoffice-v2/src/domains/business-reports/hooks/mutations/useCreateBusinessReportMutation/useCreateBusinessReportMutation.tsx(3 hunks)apps/backoffice-v2/src/domains/customer/fetchers.ts(1 hunks)apps/backoffice-v2/src/pages/MerchantMonitoring/MerchantMonitoring.page.tsx(3 hunks)apps/backoffice-v2/src/pages/MerchantMonitoring/components/CreateMerchantReportDialog/CreateMerchantReportDialog.tsx(1 hunks)apps/backoffice-v2/src/pages/MerchantMonitoring/components/CreateMerchantReportDialog/hooks/useCreateMerchantReportDialogLogic.tsx(1 hunks)apps/backoffice-v2/src/pages/MerchantMonitoring/hooks/useMerchantMonitoringLogic/useMerchantMonitoringLogic.tsx(1 hunks)apps/backoffice-v2/src/pages/MerchantMonitoring/schemas.ts(1 hunks)services/workflows-service/src/alert/alert.service.intg.test.ts(1 hunks)services/workflows-service/src/app.module.ts(2 hunks)services/workflows-service/src/business-report/business-report.controller.external.ts(1 hunks)services/workflows-service/src/business-report/business-report.module.ts(1 hunks)services/workflows-service/src/business-report/business-report.service.ts(1 hunks)services/workflows-service/src/collection-flow/collection-flow.service.intg.test.ts(1 hunks)services/workflows-service/src/collection-flow/controllers/collection-flow.no-user.controller.intg.test.ts(1 hunks)services/workflows-service/src/common/guards/token-guard/token-auth.module.ts(1 hunks)services/workflows-service/src/customer/customer.controller.external.ts(4 hunks)services/workflows-service/src/customer/customer.module.ts(2 hunks)services/workflows-service/src/customer/customer.service.ts(2 hunks)services/workflows-service/src/customer/schemas/zod-schemas.ts(1 hunks)services/workflows-service/src/customer/types.ts(2 hunks)services/workflows-service/src/end-user/end-user.controller.external.intg.test.ts(1 hunks)services/workflows-service/src/global.d.ts(1 hunks)services/workflows-service/src/merchant-monitoring/merchant-monitoring.module.ts(1 hunks)services/workflows-service/src/workflow-defintion/workflow-definition.service.intg.test.ts(2 hunks)services/workflows-service/src/workflow/workflow-runtime-data.repository.intg.test.ts(1 hunks)services/workflows-service/src/workflow/workflow.controller.external.intg.test.ts(1 hunks)services/workflows-service/src/workflow/workflow.controller.internal.intg.test.ts(1 hunks)services/workflows-service/src/workflow/workflow.service.intg.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (25)
- apps/backoffice-v2/src/common/components/organisms/Dialog/Dialog.Content.tsx
- services/workflows-service/src/merchant-monitoring/merchant-monitoring.module.ts
- services/workflows-service/src/app.module.ts
- services/workflows-service/src/workflow/workflow.service.intg.test.ts
- services/workflows-service/src/workflow-defintion/workflow-definition.service.intg.test.ts
- apps/backoffice-v2/src/pages/MerchantMonitoring/hooks/useMerchantMonitoringLogic/useMerchantMonitoringLogic.tsx
- services/workflows-service/src/collection-flow/collection-flow.service.intg.test.ts
- services/workflows-service/src/business-report/business-report.service.ts
- services/workflows-service/src/workflow/workflow-runtime-data.repository.intg.test.ts
- services/workflows-service/src/customer/customer.module.ts
- services/workflows-service/src/end-user/end-user.controller.external.intg.test.ts
- services/workflows-service/src/collection-flow/controllers/collection-flow.no-user.controller.intg.test.ts
- services/workflows-service/src/workflow/workflow.controller.external.intg.test.ts
- apps/backoffice-v2/src/pages/MerchantMonitoring/MerchantMonitoring.page.tsx
- services/workflows-service/src/business-report/business-report.module.ts
- apps/backoffice-v2/src/common/components/molecules/GetFullAccessCard/GetFullAccessCard.tsx
- services/workflows-service/src/business-report/business-report.controller.external.ts
- services/workflows-service/src/alert/alert.service.intg.test.ts
- apps/backoffice-v2/src/domains/customer/fetchers.ts
- services/workflows-service/src/customer/schemas/zod-schemas.ts
- services/workflows-service/src/global.d.ts
- apps/backoffice-v2/src/pages/MerchantMonitoring/components/CreateMerchantReportDialog/CreateMerchantReportDialog.tsx
- services/workflows-service/src/common/guards/token-guard/token-auth.module.ts
- services/workflows-service/src/workflow/workflow.controller.internal.intg.test.ts
- apps/backoffice-v2/src/domains/business-reports/hooks/mutations/useCreateBusinessReportMutation/useCreateBusinessReportMutation.tsx
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Analyze (javascript)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: lint
- GitHub Check: format
🔇 Additional comments (1)
apps/backoffice-v2/src/pages/MerchantMonitoring/schemas.ts (1)
135-151: Enhance validation for businessCorrelationId.The validation for businessCorrelationId could be more specific.
Consider adding more specific validation rules for businessCorrelationId (e.g., UUID format, specific pattern) based on your business requirements.
businessCorrelationId: z .string({ invalid_type_error: 'Business ID must be a string', }) .max(255) + .regex(/^[a-zA-Z0-9-_]+$/, { + message: 'Business ID can only contain letters, numbers, hyphens, and underscores', + }) .optional(),
Changes
Examples
Summary by CodeRabbit
New Features
Refactor