Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

blokh/feat/business report alerts #2311

Merged
merged 36 commits into from
May 22, 2024
Merged

Conversation

Blokh
Copy link
Collaborator

@Blokh Blokh commented Apr 21, 2024

User description

feat: updated callback to ongoing to format response for each report


Type

enhancement


Description

  • Added findFirst methods in both BusinessReportRepository and BusinessReportService to enhance data retrieval.
  • Enhanced HookCallbackHandlerService with Zod validation and additional logic for handling comparison reports.
  • Updated Prisma schema and SQL migration to include a new reportId field and index in the BusinessReport table.
  • Updated data migrations submodule reference.

Changes walkthrough

Relevant files
Enhancement
business-report.repository.ts
Add findFirst Method in BusinessReportRepository                 

services/workflows-service/src/business-report/business-report.repository.ts

  • Added a new method findFirst to fetch the first business report with
    enhanced scoping.
  • +9/-0     
    business-report.service.ts
    Implement findFirst Method in BusinessReportService           

    services/workflows-service/src/business-report/business-report.service.ts

  • Implemented a new method findFirst to retrieve the first business
    report.
  • +7/-0     
    hook-callback-handler.service.ts
    Enhance Data Handling and Validation in HookCallbackHandlerService

    services/workflows-service/src/workflow/hook-callback-handler.service.ts

  • Integrated Zod for data validation in
    prepareMerchantAuditReportContext.
  • Added logic to handle comparison reports in
    prepareMerchantAuditReportContext.
  • Refactored data structure in prepareMerchantAuditReportContext to
    include reportId.
  • +41/-4   
    migration.sql
    SQL Migration to Add reportId to BusinessReport                   

    services/workflows-service/prisma/migrations/20240421105728_add_business_report_report_id/migration.sql

  • Added a new column reportId to the BusinessReport table.
  • Created an index for the reportId column.
  • +5/-0     
    schema.prisma
    Update Prisma Schema with reportId in BusinessReport         

    services/workflows-service/prisma/schema.prisma

  • Added reportId field to the BusinessReport model.
  • Added an index for the reportId field.
  • +2/-0     
    Configuration changes
    data-migrations
    Update Data Migrations Submodule Reference                             

    services/workflows-service/prisma/data-migrations

    • Updated the submodule reference for data migrations.
    +1/-1     

    PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Summary by CodeRabbit

    • New Features

      • Added riskScore field to business reports for enhanced risk assessment.
      • Introduced MonitoringType for better alert categorization.
    • Improvements

      • Enhanced alert definitions with new properties and improved configurations.
      • Updated business report and alert handling methods for better data processing.
    • Bug Fixes

      • Fixed issues with report ID handling in audit report context preparation.
    • Dependencies

      • Updated Jest-related dependencies for improved testing capabilities.

    @Blokh Blokh requested a review from liorzam April 21, 2024 12:10
    @Blokh Blokh self-assigned this Apr 21, 2024
    Copy link

    changeset-bot bot commented Apr 21, 2024

    ⚠️ No Changeset found

    Latest commit: d67d5f1

    Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

    This PR includes no changesets

    When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

    Click here to learn what changesets are, and how to add one.

    Click here if you're a maintainer who wants to add a changeset to this PR

    Copy link
    Contributor

    coderabbitai bot commented Apr 21, 2024

    Walkthrough

    The recent updates introduce several key enhancements to the workflows service. Notably, the BusinessReport model now includes a nullable reportId field and a riskScore field. New methods for upserting and querying business reports were added to the BusinessReportService and BusinessReportRepository. Additionally, the alert system has been expanded to handle different monitoring types, and new dependencies and utility functions were introduced across various modules.

    Changes

    Files & Paths Change Summaries
    .../prisma/schema.prisma Added reportId and riskScore fields to BusinessReport, created index on reportId.
    .../src/business-report/business-report.repository.ts Added upsert method for business reports.
    .../src/business-report/business-report.service.ts Added upsert, findFirstOrThrow, and findManyWithFilters methods.
    .../src/business-report/dto/get-business-report.dto.ts Renamed class to GetBusinessReportDto, added properties for retrieving business reports.
    .../src/workflow/hook-callback-handler.service.ts Enhanced prepareMerchantAuditReportContext method, added AlertService dependency.
    .../package.json Added "@jest/types": "29.6.3", updated "@types/jest" to ^26.0.24.
    .../prisma/migrations Introduced MonitoringType enum, added fields to Alert and BusinessReport tables.
    .../scripts/alerts/generate-alerts.ts Renamed constants, added monitoringType handling, updated alert definitions.
    .../src/alert/alert.controller.external.ts Added MonitoringType to imports, updated types and methods.
    .../src/alert/alert.module.ts Imported BusinessReportModule, exported AlertRepository.
    .../src/alert/alert.service.intg.test.ts Added multiple imports and types for comprehensive testing.
    .../src/alert/alert.service.ts Updated method signatures and added new alert-related methods.
    .../src/alert-definition/alert-definition.repository.ts Modified method signatures in AlertDefinitionRepository.
    .../src/data-analytics/data-analytics.module.ts Added BusinessReportModule to imports.
    .../src/data-analytics/data-analytics.service.ts Added new types and methods for data analytics.

    In the code's vast expanse, new fields arise,
    Reports gain depth, with scores to analyze.
    Alerts now watch with keener eye,
    For business trends that signify.
    Modules enhanced, dependencies grow,
    Together they make our service glow.
    🐇✨


    Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

    Share
    Tips

    Chat

    There are 3 ways to chat with CodeRabbit:

    • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
      • I pushed a fix in commit <commit_id>.
      • Generate unit testing code for this file.
      • Open a follow-up GitHub issue for this discussion.
    • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
      • @coderabbitai generate unit testing code for this file.
      • @coderabbitai modularize this function.
    • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
      • @coderabbitai generate interesting stats about this repository and render them as a table.
      • @coderabbitai show all the console.log statements in this repository.
      • @coderabbitai read src/utils.ts and generate unit testing code.
      • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

    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)

    • @coderabbitai pause to pause the reviews on a PR.
    • @coderabbitai resume to resume the paused reviews.
    • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
    • @coderabbitai resolve resolve all the CodeRabbit review comments.
    • @coderabbitai help to get help.

    Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

    CodeRabbit Configration File (.coderabbit.yaml)

    • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
    • Please see the configuration documentation for more information.
    • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

    Documentation and Community

    • Visit our Documentation for detailed information on how to use CodeRabbit.
    • Join our Discord Community to get help, request features, and share feedback.
    • Follow us on X/Twitter for updates and announcements.

    @github-actions github-actions bot added the enhancement New feature or request label Apr 21, 2024
    Copy link
    Contributor

    PR Description updated to latest commit (fa0dd21)

    1 similar comment
    Copy link
    Contributor

    PR Description updated to latest commit (fa0dd21)

    Copy link
    Contributor

    github-actions bot commented Apr 21, 2024

    PR Review

    (Review updated until commit dc05878)

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple changes across several files including TypeScript and SQL, affecting both the backend logic and database schema. The changes include new methods, enhanced validation, and database modifications which require careful review to ensure they meet business requirements and do not introduce regressions.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Possible Bug: The method findFirst in BusinessReportRepository uses findFirstOrThrow which will throw an exception if no record is found. This behavior should be handled or documented to ensure that calling services are prepared to handle this exception.

    Data Integrity: The new reportId field in the BusinessReport table is added as NOT NULL without a default value. This could lead to issues with existing data or when new reports are created without a reportId.

    🔒 Security concerns

    No

    Code feedback:
    relevant fileservices/workflows-service/src/business-report/business-report.repository.ts
    suggestion      

    Consider adding error handling for findFirstOrThrow to manage cases where no record is found. This could prevent unhandled exceptions from propagating and affecting the application stability. [important]

    relevant linereturn await this.prisma.businessReport.findFirstOrThrow(

    relevant fileservices/workflows-service/prisma/migrations/20240421105728_add_business_report_report_id/migration.sql
    suggestion      

    Add a default value for the reportId column in the BusinessReport table or modify the existing data to include this new mandatory field to avoid insertion errors. [important]

    relevant lineALTER TABLE "BusinessReport" ADD COLUMN "reportId" TEXT NOT NULL;

    relevant fileservices/workflows-service/src/workflow/hook-callback-handler.service.ts
    suggestion      

    Implement additional validation or error handling for the comparedToReportId logic to ensure that the report comparison functionality is robust and handles cases where the compared report might not meet expected conditions. [medium]

    relevant lineconst comparedToReport = await this.businessReportService.findFirst(


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link
    Contributor

    Persistent review updated to latest commit fa0dd21

    Copy link
    Contributor

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Bug
    Check for null or undefined before accessing object properties to avoid runtime errors.

    Ensure that the check for the existence of comparedToReport is performed before accessing
    its properties to prevent potential runtime errors.

    services/workflows-service/src/workflow/hook-callback-handler.service.ts [158-170]

     const comparedToReport = await this.businessReportService.findFirst(
       {
         where: {
           businessId,
           reportId: comparedToReportId,
         },
       },
       [currentProjectId],
     );
    +if (!comparedToReport) {
    +  throw new BadRequestException('Compared to report not found.');
    +}
     reportData.previousReport = {
       summary: (comparedToReport.report as { summary: unknown }).summary,
     };
     
    Best practice
    Remove unnecessary use of await on return statements.

    Remove the unnecessary await keyword from the return statement since it does not affect
    the returned promise.

    services/workflows-service/src/business-report/business-report.repository.ts [33-35]

    -return await this.prisma.businessReport.findFirstOrThrow(
    +return this.prisma.businessReport.findFirstOrThrow(
       this.scopeService.scopeFindFirst(args, projectIds),
     );
     
    Use optional chaining to safely access nested properties.

    Use TypeScript's optional chaining (?.) when accessing deeply nested properties to prevent
    runtime errors if any intermediate properties are null or undefined.

    services/workflows-service/src/workflow/hook-callback-handler.service.ts [169]

    -summary: (comparedToReport.report as { summary: unknown }).summary,
    +summary: (comparedToReport?.report as { summary: unknown })?.summary,
     
    Maintainability
    Ensure proper typing at the source instead of multiple casts.

    Instead of casting reportId as string multiple times, ensure the type of reportId is
    correctly defined at its source to improve code readability and maintainability.

    services/workflows-service/src/workflow/hook-callback-handler.service.ts [127]

    -reportId: reportId as string,
    +reportId: reportId,
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    Copy link
    Contributor

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Bug
    Move the null check for comparedToReport before any operations that use it.

    To ensure the application does not proceed with a null comparedToReport, the check for
    comparedToReport being null should occur immediately after it is potentially assigned a
    value. This avoids any operations that might assume the presence of a valid report.

    services/workflows-service/src/workflow/hook-callback-handler.service.ts [158-174]

     const comparedToReport = await this.businessReportService.findFirst(
       {
         where: {
           businessId,
           reportId: comparedToReportId,
         },
       },
       [currentProjectId],
     );
    -reportData.previousReport = {
    -  summary: (comparedToReport.report as { summary: unknown }).summary,
    -};
     
     if (!comparedToReport) {
       throw new BadRequestException('Compared to report not found.');
     }
     
    +reportData.previousReport = {
    +  summary: (comparedToReport.report as { summary: unknown }).summary,
    +};
    +
    Best practice
    Remove unnecessary await in return statement of an async function.

    Remove the unnecessary await keyword from the return statement. Since the function is
    already async, returning the promise directly is more efficient and cleaner.

    services/workflows-service/src/business-report/business-report.repository.ts [33-35]

    -return await this.prisma.businessReport.findFirstOrThrow(
    +return this.prisma.businessReport.findFirstOrThrow(
       this.scopeService.scopeFindFirst(args, projectIds),
     );
     
    Maintainability
    Define complex types externally to improve readability and maintainability.

    To improve code readability and maintainability, consider defining the type used in the
    data parameter of prepareMerchantAuditReportContext method at the top level or in a
    separate type definition file, rather than inline in the method signature.

    services/workflows-service/src/workflow/hook-callback-handler.service.ts [139-150]

    +type MerchantAuditReportData = {
    +  reportData: Record<string, unknown>;
    +  base64Pdf: string;
    +  reportId: string;
    +  reportType: string;
    +  comparedToReportId?: string;
    +};
    +
     async prepareMerchantAuditReportContext(
    -  data: {
    -    reportData: Record<string, unknown>;
    -    base64Pdf: string;
    -    reportId: string;
    -    reportType: string;
    -    comparedToReportId?: string;
    -  },
    +  data: MerchantAuditReportData,
       workflowRuntime: WorkflowRuntimeData,
       resultDestinationPath: string,
       currentProjectId: TProjectId,
     ) {
     
    Error handling
    Add error handling for the PDF report document persistence operation.

    Consider adding error handling for the asynchronous operation
    this.__peristPDFReportDocumentWithWorkflowDocuments. This could involve using try-catch
    blocks to manage exceptions that may occur during the PDF document persistence process.

    services/workflows-service/src/workflow/hook-callback-handler.service.ts [177-179]

    -const { pdfReportBallerineFileId } = await this.__peristPDFReportDocumentWithWorkflowDocuments({
    -  context,
    -  customer,
    -});
    +let pdfReportBallerineFileId;
    +try {
    +  pdfReportBallerineFileId = (await this.__peristPDFReportDocumentWithWorkflowDocuments({
    +    context,
    +    customer,
    +  })).pdfReportBallerineFileId;
    +} catch (error) {
    +  console.error('Failed to persist PDF report document:', error);
    +  throw new Error('Error persisting PDF report document');
    +}
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    @Blokh Blokh requested a review from liorzam April 24, 2024 08:46
    Copy link
    Contributor

    PR Description updated to latest commit (dc05878)

    Copy link
    Contributor

    Persistent review updated to latest commit dc05878

    Copy link
    Contributor

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Remove unnecessary 'await' in return statement for cleaner and potentially more performant code.

    Consider removing the unnecessary await keyword in the return statement of the findFirst
    method. Since the method is already asynchronous, returning the promise directly can lead
    to slightly better performance and cleaner code.

    services/workflows-service/src/business-report/business-report.repository.ts [33-35]

    -return await this.prisma.businessReport.findFirstOrThrow(
    +return this.prisma.businessReport.findFirstOrThrow(
       this.scopeService.scopeFindFirst(args, projectIds),
     );
     
    Remove unnecessary 'await' in return statement for consistency and performance.

    Similar to the repository layer, consider removing the await keyword in the return
    statement of the findFirst method in the service layer for consistency and potential
    performance benefits.

    services/workflows-service/src/business-report/business-report.service.ts [27]

    -return await this.repository.findFirst(args, projectIds);
    +return this.repository.findFirst(args, projectIds);
     
    Best practice
    Validate workflowRuntime structure using Zod for safer type assertions.

    Replace the direct type assertion in the destructuring of workflowRuntime with a
    validation step using Zod or a similar library to ensure the data structure conforms to
    expected types, enhancing robustness and error handling.

    services/workflows-service/src/workflow/hook-callback-handler.service.ts [156]

    -const { context } = workflowRuntime;
    +const { context } = z.object({
    +  context: z.object({
    +    entity: z.object({
    +      id: z.string(),
    +    }),
    +  }),
    +}).parse(workflowRuntime);
     
    Add error handling for data parsing to improve robustness.

    Consider handling the potential exception that might be thrown by z.parse(data) when the
    data does not match the expected schema. This can be done by wrapping the parsing in a
    try-catch block and handling the error appropriately.

    services/workflows-service/src/workflow/hook-callback-handler.service.ts [146-154]

    -const { reportData, base64Pdf, reportId, reportType, comparedToReportId } = z
    -  .object({
    +let parsedData;
    +try {
    +  parsedData = z.object({
         reportData: z.record(z.string(), z.unknown()),
         base64Pdf: z.string(),
         reportId: z.string(),
         reportType: z.string(),
         comparedToReportId: z.string().optional(),
    -  })
    -  .parse(data);
    +  }).parse(data);
    +} catch (error) {
    +  throw new BadRequestException('Invalid data format.');
    +}
    +const { reportData, base64Pdf, reportId, reportType, comparedToReportId } = parsedData;
     
    Ensure reportId is always treated as a string to prevent type errors.

    To ensure that the reportId is always treated as a string and to avoid potential runtime
    errors, explicitly parse or validate reportId when destructuring from data instead of
    casting it later in the code.

    services/workflows-service/src/workflow/hook-callback-handler.service.ts [146-154]

     const { reportData, base64Pdf, reportId, reportType, comparedToReportId } = z
       .object({
         reportData: z.record(z.string(), z.unknown()),
         base64Pdf: z.string(),
    -    reportId: z.string(),
    +    reportId: z.string().transform((value) => value.toString()),
         reportType: z.string(),
         comparedToReportId: z.string().optional(),
       })
       .parse(data);
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    # Conflicts:
    #	services/workflows-service/prisma/data-migrations
    #	services/workflows-service/src/business-report/business-report.repository.ts
    Copy link
    Contributor

    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 0

    Out of diff range and nitpick comments (1)
    services/workflows-service/src/workflow/cron/ongoing-monitoring.cron.ts (1)

    74-74: Logging level change approved; consider enhancing the log message.

    The change in logging level from error to debug is appropriate if the scenario is non-critical. However, consider adding more details to the log message to aid in debugging, such as the business name or other identifiers.

    @Blokh Blokh changed the title blokh/feat/add-business-report-report-id blokh/feat/business report alerts May 13, 2024
    Copy link
    Contributor

    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 2

    Review Details

    Configuration used: CodeRabbit UI
    Review profile: CHILL

    Commits Files that changed from the base of the PR and between 5b8e07f and 7443d11.
    Files ignored due to path filters (1)
    • pnpm-lock.yaml is excluded by !pnpm-lock.yaml
    Files selected for processing (10)
    • services/workflows-service/prisma/migrations/20240516085653_add_unique_report_id/migration.sql (1 hunks)
    • services/workflows-service/prisma/schema.prisma (5 hunks)
    • services/workflows-service/scripts/alerts/generate-alerts.ts (14 hunks)
    • services/workflows-service/src/alert/alert.controller.external.ts (3 hunks)
    • services/workflows-service/src/alert/alert.service.intg.test.ts (34 hunks)
    • services/workflows-service/src/alert/alert.service.ts (8 hunks)
    • services/workflows-service/src/alert/consts.ts (1 hunks)
    • services/workflows-service/src/alert/types.ts (2 hunks)
    • services/workflows-service/src/data-analytics/data-analytics.service.ts (4 hunks)
    • services/workflows-service/src/data-analytics/types.ts (4 hunks)
    Files skipped from review due to trivial changes (1)
    • services/workflows-service/prisma/migrations/20240516085653_add_unique_report_id/migration.sql
    Files skipped from review as they are similar to previous changes (4)
    • services/workflows-service/src/alert/consts.ts
    • services/workflows-service/src/alert/types.ts
    • services/workflows-service/src/data-analytics/data-analytics.service.ts
    • services/workflows-service/src/data-analytics/types.ts
    Additional comments not posted (23)
    services/workflows-service/src/alert/alert.controller.external.ts (3)

    13-13: New imports for Alert, AlertDefinition, MonitoringType, and types are correctly added.

    Also applies to: 18-23


    136-203: The listBusinessReportAlerts method is well-implemented, with appropriate validation, response types, and data retrieval logic.


    24-24: The import for express is correctly added for handling responses.

    services/workflows-service/src/alert/alert.service.ts (3)

    6-16: New imports for InputJsonValue, ObjectValues, TProjectId, Alert, AlertDefinition, AlertSeverity, AlertState, AlertStatus, BusinessReport, MonitoringType, CheckRiskScoreOptions, and InlineRule are correctly added.

    Also applies to: 24-25


    Line range hint 82-101: The getAlerts method is well-implemented, with appropriate filtering, ordering, and pagination logic.


    155-202: The checkOngoingMonitoringAlert method is well-implemented, with appropriate logic for checking alerts and creating new alerts if necessary.

    services/workflows-service/scripts/alerts/generate-alerts.ts (4)

    Line range hint 557-594: LGTM! The function getAlertDefinitionCreateData is well-defined.


    Line range hint 596-648: LGTM! The function generateAlertDefinitions is well-defined.


    Line range hint 649-697: LGTM! The function generateFakeAlert is well-defined.


    Line range hint 701-747: LGTM! The function seedTransactionsAlerts is well-defined.

    services/workflows-service/prisma/schema.prisma (5)

    711-714: The addition of the MonitoringType enum looks good and will help categorize different types of monitoring.


    719-720: The addition of the monitoringType field to the AlertDefinition model is appropriate and aligns with the new MonitoringType enum.


    783-785: The addition of the businessId and business fields to the Alert model is appropriate and enhances the model by allowing alerts to be associated with a specific business.


    844-844: The addition of the reportId field to the BusinessReport model is appropriate and ensures that each report has a unique identifier.


    847-860: The addition of the riskScore field to the BusinessReport model is appropriate and enhances the model by allowing the storage and efficient querying of risk scores.

    services/workflows-service/src/alert/alert.service.intg.test.ts (8)

    27-28: Necessary imports for new tests.


    75-76: Necessary setup for new tests.


    1091-1122: Test case for "When no previousReport" is well-written.


    1125-1162: Test case for "When previous report not ongoing report" is well-written.


    1166-1203: Test case for "When previous report has higher risk score" is well-written.


    1206-1243: Test case for "When previous report does not hit required changes" is well-written.


    1246-1366: Test case for "When previous report hits necessary changes to invoke alert" is well-written.


    Line range hint 1589-1611: Necessary function for creating counterparty in tests.

    Blokh added 2 commits May 16, 2024 12:30
    # Conflicts:
    #	services/workflows-service/package.json
    #	services/workflows-service/prisma/data-migrations
    #	services/workflows-service/src/data-analytics/data-analytics.service.ts
    #	services/workflows-service/src/data-analytics/types.ts
    Copy link
    Contributor

    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 1

    Out of diff range and nitpick comments (1)
    services/workflows-service/scripts/alerts/generate-alerts.ts (1)

    Line range hint 38-499: The TRANSACTIONS_ALERT_DEFINITIONS object is well-structured. Consider improving type safety by using TypeScript's advanced types features, such as utility types (Partial, Pick) or custom types, to make the code more robust and maintainable. This could help prevent bugs related to incorrect or missing properties and make the code easier to understand and modify.

    - } as const satisfies Record<
    -   Partial<keyof typeof TransactionAlertLabel>,
    -   {
    -     inlineRule: InlineRule & InputJsonValue;
    -     defaultSeverity: AlertSeverity;
    -     dedupeStrategy?: TDedupeStrategy;
    -     monitoringType?: MonitoringType;
    -     enabled?: boolean;
    -     description?: string;
    -   }
    - >;
    + } as const satisfies Record<keyof typeof TransactionAlertLabel, AlertDefinition>;
    Review Details

    Configuration used: CodeRabbit UI
    Review profile: CHILL

    Commits Files that changed from the base of the PR and between 7443d11 and 5181498.
    Files ignored due to path filters (1)
    • pnpm-lock.yaml is excluded by !pnpm-lock.yaml
    Files selected for processing (5)
    • services/workflows-service/package.json (2 hunks)
    • services/workflows-service/scripts/alerts/generate-alerts.ts (14 hunks)
    • services/workflows-service/src/data-analytics/data-analytics.service.ts (4 hunks)
    • services/workflows-service/src/data-analytics/types.ts (4 hunks)
    • services/workflows-service/src/workflow/hook-callback-handler.service.ts (6 hunks)
    Files skipped from review as they are similar to previous changes (3)
    • services/workflows-service/package.json
    • services/workflows-service/src/data-analytics/types.ts
    • services/workflows-service/src/workflow/hook-callback-handler.service.ts
    Additional comments not posted (11)
    services/workflows-service/src/data-analytics/data-analytics.service.ts (7)

    4-5: New imports look good and are necessary for the added functionality.

    Also applies to: 10-10, 14-20, 23-23


    30-30: The addition of BusinessReportService as a dependency is appropriate for the new functionality.


    74-84: The function signature and initial variable extraction look correct.


    85-95: The check for the previous report is necessary and correctly implemented.


    97-127: The extraction and validation of risk scores are correctly implemented.


    129-178: The generation of rule results and execution details is correctly implemented.


    383-383: The added condition to check for non-null counterpartyBeneficiaryId is appropriate for the functionality.

    Also applies to: 395-395

    services/workflows-service/scripts/alerts/generate-alerts.ts (4)

    6-6: Import statement for MonitoringType looks good.


    20-20: Import statement for daysToMinutesConverter looks good.


    21-21: Import statement for SEVEN_DAYS looks good.


    23-23: Import statement for TWENTY_ONE_DAYS looks good.

    Blokh and others added 3 commits May 16, 2024 12:52
    # Conflicts:
    #	services/workflows-service/prisma/data-migrations
    #	services/workflows-service/scripts/alerts/generate-alerts.ts
    #	services/workflows-service/src/alert/alert.service.intg.test.ts
    #	services/workflows-service/src/data-analytics/data-analytics.service.ts
    #	services/workflows-service/src/data-analytics/types.ts
    Copy link
    Contributor

    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 0

    Review Details

    Configuration used: CodeRabbit UI
    Review profile: CHILL

    Commits Files that changed from the base of the PR and between 5181498 and 8747e79.
    Files ignored due to path filters (1)
    • pnpm-lock.yaml is excluded by !pnpm-lock.yaml
    Files selected for processing (4)
    • services/workflows-service/prisma/data-migrations (1 hunks)
    • services/workflows-service/src/alert/alert.service.intg.test.ts (5 hunks)
    • services/workflows-service/src/alert/consts.ts (1 hunks)
    • services/workflows-service/src/data-analytics/data-analytics.service.ts (2 hunks)
    Files skipped from review as they are similar to previous changes (4)
    • services/workflows-service/prisma/data-migrations
    • services/workflows-service/src/alert/alert.service.intg.test.ts
    • services/workflows-service/src/alert/consts.ts
    • services/workflows-service/src/data-analytics/data-analytics.service.ts

    Copy link
    Contributor

    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 0

    Review Details

    Configuration used: CodeRabbit UI
    Review profile: CHILL

    Commits Files that changed from the base of the PR and between 8747e79 and 2a47598.
    Files selected for processing (3)
    • services/workflows-service/src/alert/alert.service.ts (8 hunks)
    • services/workflows-service/src/data-analytics/data-analytics.service.ts (4 hunks)
    • services/workflows-service/src/data-analytics/types.ts (2 hunks)
    Files skipped from review as they are similar to previous changes (3)
    • services/workflows-service/src/alert/alert.service.ts
    • services/workflows-service/src/data-analytics/data-analytics.service.ts
    • services/workflows-service/src/data-analytics/types.ts

    Copy link
    Contributor

    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 4

    Outside diff range and nitpick comments (1)
    services/workflows-service/src/alert/alert.service.ts (1)

    82-82: Consider adding type annotations to args for better type safety and code readability.

    Review Details

    Configuration used: CodeRabbit UI
    Review profile: CHILL

    Commits Files that changed from the base of the PR and between 2a47598 and 227403f.
    Files ignored due to path filters (1)
    • pnpm-lock.yaml is excluded by !pnpm-lock.yaml
    Files selected for processing (3)
    • services/workflows-service/scripts/alerts/generate-alerts.ts (11 hunks)
    • services/workflows-service/src/alert/alert.service.ts (8 hunks)
    • services/workflows-service/src/alert/consts.ts (1 hunks)
    Files not reviewed due to errors (1)
    • services/workflows-service/scripts/alerts/generate-alerts.ts (no review received)
    Files skipped from review as they are similar to previous changes (1)
    • services/workflows-service/src/alert/consts.ts
    Additional comments not posted (1)
    services/workflows-service/src/alert/alert.service.ts (1)

    Line range hint 292-309: Ensure that createAlert method properly handles the case where subject is empty.

    Verification successful

    The createAlert method uses Object.assign({}, ...(subject || [])) to handle the subject parameter. This approach ensures that if subject is empty or undefined, it will default to an empty object, thus preventing any errors.

    • subject || [] ensures that if subject is falsy (e.g., null or undefined), it will be replaced with an empty array.
    • Object.assign({}, ...[]) will result in an empty object if the array is empty.

    This implementation correctly handles the case where subject is empty.

    Scripts Executed

    The following scripts were executed for the analysis:


    Script:

    #!/bin/bash
    # Description: Verify handling of empty `subject` in `createAlert`.
    
    # Test: Search for error handling related to empty `subject`.
    rg --type typescript 'createAlert' 'services/workflows-service/src/alert/alert.service.ts'
    

    Length of output: 123



    Script:

    #!/bin/bash
    # Description: Verify handling of empty `subject` in `createAlert`.
    
    # Correct the file type and search for the `createAlert` method.
    rg 'createAlert' 'services/workflows-service/src/alert/alert.service.ts'
    

    Length of output: 376



    Script:

    #!/bin/bash
    # Description: Extract the implementation of the `createAlert` method to verify handling of empty `subject`.
    
    # Extract the `createAlert` method implementation.
    ast-grep --lang typescript --pattern $'class $_ {
      $$$
      private createAlert($_, $_, $_) {
        $$$
      }
      $$$  
    }' 'services/workflows-service/src/alert/alert.service.ts'
    

    Length of output: 169



    Script:

    #!/bin/bash
    # Description: Extract the implementation of the `createAlert` method to verify handling of empty `subject`.
    
    # Search for the `createAlert` method definition and extract surrounding lines.
    rg -A 20 'private createAlert' 'services/workflows-service/src/alert/alert.service.ts'
    

    Length of output: 798

    Co-authored-by: Lior Zamir <[email protected]>
    Copy link
    Contributor

    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 2

    Outside diff range and nitpick comments (2)
    services/workflows-service/src/alert-definition/alert-definition.repository.ts (1)

    14-17: Consider adding a comment explaining the use of projectIds for clarity.

    services/workflows-service/src/alert/alert.service.ts (1)

    Line range hint 82-100: The inclusion of monitoringType directly in the query enhances specificity and efficiency. Consider adding a comment to explain its usage for future maintainers.

    Review Details

    Configuration used: CodeRabbit UI
    Review profile: CHILL

    Commits Files that changed from the base of the PR and between 227403f and 97c07a7.
    Files selected for processing (3)
    • services/workflows-service/src/alert-definition/alert-definition.repository.ts (3 hunks)
    • services/workflows-service/src/alert/alert.service.ts (9 hunks)
    • services/workflows-service/src/data-analytics/data-analytics.service.ts (4 hunks)
    Files skipped from review as they are similar to previous changes (1)
    • services/workflows-service/src/data-analytics/data-analytics.service.ts
    Additional comments not posted (5)
    services/workflows-service/src/alert-definition/alert-definition.repository.ts (4)

    14-17: The updated method signature and the use of scoped query are consistent with multi-tenant application requirements.


    52-59: Dynamic ordering via the orderBy parameter enhances flexibility in data retrieval.


    62-64: The method signature update is consistent and well-implemented.


    90-92: Enhanced flexibility in the deleteById method through the inclusion of additional arguments is a positive change.

    services/workflows-service/src/alert/alert.service.ts (1)

    127-129: Filtering alert definitions by monitoringType is an effective and relevant addition.

    @Blokh Blokh enabled auto-merge (squash) May 22, 2024 08:26
    @Blokh Blokh merged commit c5d3c42 into dev May 22, 2024
    9 checks passed
    @Blokh Blokh deleted the blokh/feat/add-business-report-report-id branch May 22, 2024 08:44
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    None yet

    3 participants