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

WIP: bal 1880 ongoing moitoring alerts v2 new mapping #2307

Draft
wants to merge 3 commits into
base: bal-1880-ongoing-moitoring-alerts
Choose a base branch
from

Conversation

liorzam
Copy link
Collaborator

@liorzam liorzam commented Apr 16, 2024

Type

enhancement, bug_fix


Description

  • Introduced new database schema changes to better manage alerts by separating them into OngoingMarchentAlert and TransactionAlert.
  • Removed old columns and foreign keys from the Alert table and added new ones to support the updated structure.
  • Created new tables with unique constraints and foreign keys to ensure data integrity and proper relational mapping.
  • Updated Prisma schema models to align with the new database structure, ensuring that the application layer correctly interacts with the updated database.

Changes walkthrough

Relevant files
Enhancement
migration.sql
Database Schema Update for Alert Table and New Related Tables

services/workflows-service/prisma/migrations/20240416131447_alert_new_mapping_by_instance/migration.sql

  • Dropped foreign keys and columns related to businessId and
    counterpartyId from the Alert table.
  • Added new columns ongoingMarchentAlertId and transactionAlertId to the
    Alert table.
  • Created new tables OngoingMarchentAlert and TransactionAlert with
    appropriate constraints.
  • Established new unique indexes and foreign key constraints for the new
    structure.
  • +68/-0   
    schema.prisma
    Prisma Schema Updates for New Alert Management Structure 

    services/workflows-service/prisma/schema.prisma

  • Modified the Alert model to reference new OngoingMarchentAlert and
    TransactionAlert models instead of direct business and counterparty
    relations.
  • Added new models OngoingMarchentAlert and TransactionAlert with fields
    and relations.
  • Updated relations in Business and Counterparty models to reflect new
    alert structures.
  • +34/-7   

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

    Copy link

    changeset-bot bot commented Apr 16, 2024

    ⚠️ No Changeset found

    Latest commit: d8e5e9e

    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 16, 2024

    Important

    Auto Review Skipped

    Auto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings.

    Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

    You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.


    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 enhancement New feature or request bug_fix labels Apr 16, 2024
    Copy link
    Contributor

    PR Description updated to latest commit (75229c0)

    Copy link
    Contributor

    PR Review

    ⏱️ Estimated effort to review [1-5]

    4, due to the complexity of the database schema changes, the need to ensure data integrity, and the potential impact on existing data and system functionality.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Data Loss Risk: Dropping columns businessId and counterpartyId from the Alert table without a backup or migration strategy could lead to irreversible data loss.

    Unique Constraint Failure: Adding unique constraints on ongoingMarchentAlertId and transactionAlertId in the Alert table could cause the migration to fail if there are existing duplicate entries.

    🔒 Security concerns

    No

    Code feedback:
    relevant fileservices/workflows-service/prisma/migrations/20240416131447_alert_new_mapping_by_instance/migration.sql
    suggestion      

    Consider implementing a data backup strategy before dropping columns to prevent data loss. This could involve creating a temporary table to store the current data before the migration. [important]

    relevant lineALTER TABLE "Alert" DROP COLUMN "businessId",

    relevant fileservices/workflows-service/prisma/migrations/20240416131447_alert_new_mapping_by_instance/migration.sql
    suggestion      

    Before applying the unique constraints, add a pre-migration check to identify and handle any existing duplicate values in ongoingMarchentAlertId and transactionAlertId. This could prevent the migration from failing. [important]

    relevant lineCREATE UNIQUE INDEX "Alert_ongoingMarchentAlertId_key" ON "Alert"("ongoingMarchentAlertId");

    relevant fileservices/workflows-service/prisma/migrations/20240416131447_alert_new_mapping_by_instance/migration.sql
    suggestion      

    Add cascading delete or update rules to foreign key constraints where appropriate to maintain referential integrity when related records are deleted or updated. [medium]

    relevant lineALTER TABLE "OngoingMarchentAlert" ADD CONSTRAINT "OngoingMarchentAlert_businessId_fkey" FOREIGN KEY ("businessId") REFERENCES "Business"("id") ON DELETE SET NULL ON UPDATE CASCADE;

    relevant fileservices/workflows-service/prisma/migrations/20240416131447_alert_new_mapping_by_instance/migration.sql
    suggestion      

    Ensure that the new tables OngoingMarchentAlert and TransactionAlert are properly indexed beyond their primary keys to improve query performance, especially if they are expected to grow significantly. [medium]

    relevant lineCREATE TABLE "OngoingMarchentAlert" (


    ✨ 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

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Possible issue
    Ensure no duplicate values exist before adding unique constraints to prevent migration failure.

    Before adding unique constraints to the Alert table, ensure that there are no existing
    duplicate values in the ongoingMarchentAlertId and transactionAlertId columns. You can do
    this by running a pre-check SQL query to find duplicates. If duplicates exist, handle them
    appropriately before applying the unique constraints.

    services/workflows-service/prisma/migrations/20240416131447_alert_new_mapping_by_instance/migration.sql [50-53]

    +-- Check for duplicates before creating unique indexes
    +SELECT "ongoingMarchentAlertId", COUNT(*) FROM "Alert" GROUP BY "ongoingMarchentAlertId" HAVING COUNT(*) > 1;
    +SELECT "transactionAlertId", COUNT(*) FROM "Alert" GROUP BY "transactionAlertId" HAVING COUNT(*) > 1;
    +-- If no duplicates, create unique indexes
     CREATE UNIQUE INDEX "Alert_ongoingMarchentAlertId_key" ON "Alert"("ongoingMarchentAlertId");
     CREATE UNIQUE INDEX "Alert_transactionAlertId_key" ON "Alert"("transactionAlertId");
     
    Best practice
    Wrap ALTER TABLE commands in a transaction to ensure atomic operations.

    Consider adding a transaction block around the series of ALTER TABLE commands to ensure
    atomicity. This way, if one command fails, the previous changes are rolled back,
    maintaining database integrity.

    services/workflows-service/prisma/migrations/20240416131447_alert_new_mapping_by_instance/migration.sql [11-23]

    -ALTER TABLE "Alert" DROP CONSTRAINT "Alert_businessId_fkey";
    -ALTER TABLE "Alert" DROP CONSTRAINT "Alert_counterpartyId_fkey";
    -ALTER TABLE "Alert" DROP COLUMN "businessId", DROP COLUMN "counterpartyId", ADD COLUMN "ongoingMarchentAlertId" TEXT, ADD COLUMN "transactionAlertId" TEXT;
    +BEGIN;
    +  ALTER TABLE "Alert" DROP CONSTRAINT "Alert_businessId_fkey";
    +  ALTER TABLE "Alert" DROP CONSTRAINT "Alert_counterpartyId_fkey";
    +  ALTER TABLE "Alert" DROP COLUMN "businessId", DROP COLUMN "counterpartyId", ADD COLUMN "ongoingMarchentAlertId" TEXT, ADD COLUMN "transactionAlertId" TEXT;
    +COMMIT;
     
    Add cascade behaviors to relations to automatically manage related data on updates and deletions.

    Consider adding onDelete and onUpdate cascade behaviors to the ongoingMarchentAlert and
    transactionAlert relations in the Alert model to manage related data automatically upon
    deletion or update.

    services/workflows-service/prisma/schema.prisma [809-813]

    -ongoingMarchentAlert   OngoingMarchentAlert? @relation(fields: [ongoingMarchentAlertId], references: [id])
    -transactionAlert   TransactionAlert? @relation(fields: [transactionAlertId], references: [id])
    +ongoingMarchentAlert   OngoingMarchentAlert? @relation(fields: [ongoingMarchentAlertId], references: [id], onDelete: Cascade, onUpdate: Cascade)
    +transactionAlert   TransactionAlert? @relation(fields: [transactionAlertId], references: [id], onDelete: Cascade, onUpdate: Cascade)
     
    Enhancement
    Make unique fields optional to handle potential null values correctly.

    Ensure that the ongoingMarchentAlertId and transactionAlertId fields in the Alert model
    are optional if they are not guaranteed to be present at all times. This avoids potential
    issues with null values.

    services/workflows-service/prisma/schema.prisma [810-813]

    -ongoingMarchentAlertId String?  @unique
    -transactionAlertId String?  @unique
    +ongoingMarchentAlertId String?
    +transactionAlertId String?
     
    Maintainability
    Verify no operational dependencies exist before dropping constraints and indexes.

    When dropping foreign keys and indexes, ensure that no operations depend on these
    constraints during the migration. This can be verified by checking active queries or
    temporarily halting operations that might interfere.

    services/workflows-service/prisma/migrations/20240416131447_alert_new_mapping_by_instance/migration.sql [11-17]

    +-- Ensure no dependencies before dropping
    +-- Temporarily halt operations that might interfere
     ALTER TABLE "Alert" DROP CONSTRAINT "Alert_businessId_fkey";
     DROP INDEX "Alert_counterpartyId_idx";
     

    ✨ 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.

    @liorzam liorzam changed the title bal 1880 ongoing moitoring alerts v2 new mapping WIP: bal 1880 ongoing moitoring alerts v2 new mapping May 7, 2024
    @liorzam liorzam marked this pull request as draft May 7, 2024 09:50
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    None yet

    2 participants