-
Notifications
You must be signed in to change notification settings - Fork 200
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(monitoring): adds exhaustive check for action before deboarding a merchant (BAL-3343) #2964
Conversation
|
WalkthroughThis pull request introduces comprehensive changes to the merchant monitoring business report functionality. The modifications span multiple files across the backoffice and workflows service, focusing on enhancing the user experience for turning monitoring on and off for merchants. The changes include adding a dialog component, improving state management, implementing form validation, and updating the backend controller to handle more detailed monitoring state changes. Changes
Sequence DiagramsequenceDiagram
participant User
participant Page
participant Dialog
participant Hook
participant API
User->>Page: Click Monitoring Toggle
Page->>Dialog: Open Confirmation Modal
Dialog->>User: Request Monitoring State Change
User->>Dialog: Select Reason and Confirm
Dialog->>Hook: Submit Monitoring Change
Hook->>API: Send Monitoring State Update
API-->>Hook: Return Response
Hook->>Page: Update UI
Page->>User: Show Confirmation Toast
Possibly related PRs
Suggested Labels
Suggested Reviewers
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 (
|
...fice-v2/src/pages/MerchantMonitoringBusinessReport/MerchantMonitoringBusinessReport.page.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please make sure Nitzan and Adi and Robbie take a look at the design and texts
.../hooks/useMerchantMonitoringBusinessReportLogic/useMerchantMonitoringBusinessReportLogic.tsx
Outdated
Show resolved
Hide resolved
.../hooks/useMerchantMonitoringBusinessReportLogic/useMerchantMonitoringBusinessReportLogic.tsx
Outdated
Show resolved
Hide resolved
services/workflows-service/src/business/dtos/business-monitoring.patch.dto.ts
Show resolved
Hide resolved
apps/backoffice-v2/src/pages/MerchantMonitoringBusinessReport/fetchers.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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
🧹 Nitpick comments (5)
apps/backoffice-v2/src/pages/MerchantMonitoringBusinessReport/fetchers.ts (1)
8-12
: Consider adding validation constraints to the state field.The
state
field should be restricted to specific values ('on'/'off') to prevent invalid states.- state: z.string(), + state: z.enum(['on', 'off']),apps/backoffice-v2/src/pages/MerchantMonitoringBusinessReport/hooks/useToggleMonitoringMutation/useToggleMonitoringMutation.tsx (2)
15-17
: Consider adding JSDoc documentation.Adding documentation would help explain the purpose of each parameter, especially the optional callbacks.
+/** + * Custom hook for toggling merchant monitoring state + * @param state - The desired monitoring state ('on'/'off') + * @param onSuccess - Optional callback called after successful state change + * @param onError - Optional callback called after error, if not a 400 error + */
31-39
: Consider adding error type safety.The error handling could benefit from type narrowing to ensure all error cases are handled properly.
- onError: (error: unknown) => { + onError: (error: unknown) => { + if (!(error instanceof Error)) { + onError?.(error); + return; + } + if (error instanceof HttpError && error.code === 400) { toast.error(error.message); return; } - - onError?.(error); + + toast.error(error.message); + onError?.(error); },services/workflows-service/src/business/business.controller.external.ts (2)
139-146
: Add OpenAPI documentation for the request body.The endpoint would benefit from detailed OpenAPI documentation for the new request body structure.
@common.Patch('/:id/monitoring') @swagger.ApiForbiddenResponse() @swagger.ApiOkResponse({ type: BusinessDto }) @swagger.ApiNotFoundResponse({ type: errors.NotFoundException }) +@swagger.ApiBody({ + description: 'Update monitoring state with reason', + type: BusinessMonitoringPatchDto, +})
158-166
: Consider adding type safety for metadata structure.The metadata handling could benefit from stronger typing to prevent potential runtime errors.
- const metadata = business?.metadata as { + interface BusinessMetadata { + featureConfig?: { + [FEATURE_LIST.ONGOING_MERCHANT_REPORT]?: { + enabled: boolean; + reason: string | null; + userReason: string | null; + }; + }; + } + + const metadata = business?.metadata as BusinessMetadata;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/backoffice-v2/src/pages/MerchantMonitoringBusinessReport/MerchantMonitoringBusinessReport.page.tsx
(4 hunks)apps/backoffice-v2/src/pages/MerchantMonitoringBusinessReport/fetchers.ts
(1 hunks)apps/backoffice-v2/src/pages/MerchantMonitoringBusinessReport/hooks/useMerchantMonitoringBusinessReportLogic/useMerchantMonitoringBusinessReportLogic.tsx
(4 hunks)apps/backoffice-v2/src/pages/MerchantMonitoringBusinessReport/hooks/useToggleMonitoringMutation/useToggleMonitoringMutation.tsx
(1 hunks)apps/backoffice-v2/src/pages/MerchantMonitoringBusinessReport/mutations/useTurnMonitoringOffMutation/useTurnMonitoringOffMutation.tsx
(0 hunks)apps/backoffice-v2/src/pages/MerchantMonitoringBusinessReport/mutations/useTurnMonitoringOnMutation/useTurnMonitoringOnMutation.tsx
(0 hunks)services/workflows-service/src/business/business.controller.external.ts
(3 hunks)services/workflows-service/src/business/dtos/business-monitoring.patch.dto.ts
(1 hunks)
💤 Files with no reviewable changes (2)
- apps/backoffice-v2/src/pages/MerchantMonitoringBusinessReport/mutations/useTurnMonitoringOffMutation/useTurnMonitoringOffMutation.tsx
- apps/backoffice-v2/src/pages/MerchantMonitoringBusinessReport/mutations/useTurnMonitoringOnMutation/useTurnMonitoringOnMutation.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test_linux
- GitHub Check: test_windows
- GitHub Check: build (windows-latest)
🔇 Additional comments (6)
apps/backoffice-v2/src/pages/MerchantMonitoringBusinessReport/hooks/useMerchantMonitoringBusinessReportLogic/useMerchantMonitoringBusinessReportLogic.tsx (1)
23-46
: Well-implemented Zod schema with conditional validationThe
ZodDeboardingSchema
correctly usesrefine
to ensure that when thereason
is'other'
, theuserReason
must be provided and have at least 5 characters. This keeps validation logic centralized and maintains form integrity.apps/backoffice-v2/src/pages/MerchantMonitoringBusinessReport/MerchantMonitoringBusinessReport.page.tsx (2)
226-234
: Move inline success handler to mutation'sonSuccess
The
onSuccess
callback passed directly toturnOngoingMonitoringOn
overrides the one defined in the mutation hook. For consistency and better maintainability, consider moving this success logic into the mutation'sonSuccess
handler.
130-139
: Verify escape key handling when deboarding modal is openEnsure that the
onEscapeKeyDown
handler prevents the dropdown menu from closing when the deboarding modal is open without causing unintended side effects. Test to confirm that pressing the escape key behaves correctly in all modal states.services/workflows-service/src/business/dtos/business-monitoring.patch.dto.ts (1)
10-13
:⚠️ Potential issueAdd allowed values validation to the
reason
fieldThe
reason
field should accept only specific predefined values to prevent invalid input. Consider adding@IsIn([...])
to enforce this constraint.Apply this diff to enhance validation:
@ApiProperty({ type: String, required: false }) @IsOptional() @IsString() +@IsIn(['fraud', 'regulations', 'disputes', 'expired', 'other']) reason?: string;
Likely invalid or redundant comment.
apps/backoffice-v2/src/pages/MerchantMonitoringBusinessReport/fetchers.ts (1)
27-30
: LGTM! Clean API integration.The endpoint and method are correctly configured, and the schema validation is properly implemented.
services/workflows-service/src/business/business.controller.external.ts (1)
Line range hint
169-183
: LGTM! Proper transaction handling.The transaction is correctly implemented to ensure atomic updates to the business metadata.
* feat: updated styles for link elements (#2959) * feat: added csv document rendering (#2958) * fix(monitoring): changes the block ordering in website credibility view (#2963) * feat(monitoring): adds loading state for a single merchant record (BAL-3359) (#2960) * feat(monitoring): adjusts merchant risk summary text (BAL-3373) (#2961) * refactor(websiteCredibility): fix CardContent height for no data (#2966) * refactor(websiteCredibility): fix CardContent height for no data - Remove unused Tooltip import from recharts - Update CardContent class to ensure full height (your code is like a tidy room: looks clean but still has hidden messes) * empty * fix: UI fixes for statistics and merchant monitoring report pages (#2965) * feat(monitoring): adds exhaustive check for action before deboarding a merchant (BAL-3343) (#2964) * feat(monitoring): preserves scroll position on a data table (BAL-3248) (#2962) * fix: chart graph cut off (BAL-3395) (#2969) * fix: corrected home page merchants metrics source of truth (BAL-3396, BAL-3397) (#2968) * chore(*): updated packages (#2971) * fix(backoffice-v2): reverted default logic for from and to (#2973) * refactor(entities): streamline form data context creation (#2974) - Remove unnecessary context object creation - Simplify the return statement by directly returning the new context (your code is like a magic trick that turns objects into empty space) * fix: remove monitoring params logic from navbar (#2975) Co-authored-by: Omri Levy <61207713+Omri-Levy@users.noreply.github.com> * fix: fixed popup flickering in date picker & bump (#2977) * feat: add a report note when monitoring status is toggled (BAL-3398) (#2979) * feat: add a report note when monitoring status is toggled * chore: remove storing reason in metadata * fix: dmt and dsta rules (#2970) Co-authored-by: Lior Zamir <liorz@ballerine.com> Co-authored-by: Alon Peretz <8467965+alonp99@users.noreply.github.com> --------- Co-authored-by: Illia Rudniev <cheskmr@gmail.com> Co-authored-by: Sasha <sasham@ballerine.com> Co-authored-by: Shane <66246046+shanegrouber@users.noreply.github.com> Co-authored-by: Tomer Shvadron <tomers@ballerine.com> Co-authored-by: liorzam <6435752+liorzam@users.noreply.github.com> Co-authored-by: Lior Zamir <liorz@ballerine.com> Co-authored-by: Alon Peretz <8467965+alonp99@users.noreply.github.com>
* feat: updated styles for link elements (#2959) * feat: added csv document rendering (#2958) * fix(monitoring): changes the block ordering in website credibility view (#2963) * feat(monitoring): adds loading state for a single merchant record (BAL-3359) (#2960) * feat(monitoring): adjusts merchant risk summary text (BAL-3373) (#2961) * refactor(websiteCredibility): fix CardContent height for no data (#2966) * refactor(websiteCredibility): fix CardContent height for no data - Remove unused Tooltip import from recharts - Update CardContent class to ensure full height (your code is like a tidy room: looks clean but still has hidden messes) * empty * fix: UI fixes for statistics and merchant monitoring report pages (#2965) * feat(monitoring): adds exhaustive check for action before deboarding a merchant (BAL-3343) (#2964) * feat(monitoring): preserves scroll position on a data table (BAL-3248) (#2962) * fix: chart graph cut off (BAL-3395) (#2969) * fix: corrected home page merchants metrics source of truth (BAL-3396, BAL-3397) (#2968) * chore(*): updated packages (#2971) * fix(backoffice-v2): reverted default logic for from and to (#2973) * refactor(entities): streamline form data context creation (#2974) - Remove unnecessary context object creation - Simplify the return statement by directly returning the new context (your code is like a magic trick that turns objects into empty space) * fix: remove monitoring params logic from navbar (#2975) * fix: fixed popup flickering in date picker & bump (#2977) * feat: add a report note when monitoring status is toggled (BAL-3398) (#2979) * feat: add a report note when monitoring status is toggled * chore: remove storing reason in metadata * fix: dmt and dsta rules (#2970) --------- Co-authored-by: Illia Rudniev <cheskmr@gmail.com> Co-authored-by: Sasha <sasham@ballerine.com> Co-authored-by: Shane <66246046+shanegrouber@users.noreply.github.com> Co-authored-by: Tomer Shvadron <tomers@ballerine.com> Co-authored-by: liorzam <6435752+liorzam@users.noreply.github.com> Co-authored-by: Lior Zamir <liorz@ballerine.com> Co-authored-by: Alon Peretz <8467965+alonp99@users.noreply.github.com>
Repro steps:
Turn Monitoring Off
option from the options list in the top-right corner of the pageSee in action:
Screen.Recording.2025-01-14.at.21.11.41.mov
Includes changes to store user-entered data on the backend.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes