Skip to content

Soft-delete integrations when removing them#855

Merged
sausage-todd merged 1 commit intomainfrom
improvement/soft-delete-integrations
May 15, 2023
Merged

Soft-delete integrations when removing them#855
sausage-todd merged 1 commit intomainfrom
improvement/soft-delete-integrations

Conversation

@sausage-todd
Copy link
Copy Markdown
Contributor

@sausage-todd sausage-todd commented May 12, 2023

Changes proposed ✍️

What

🤖 Generated by Copilot at 2d0fa4d

Removed force option from sync method in integrationRepository.ts to prevent data loss. This is part of a pull request to improve the database configuration and migrations for the backend.

🤖 Generated by Copilot at 2d0fa4d

force option gone
sync method updates table
data stays in fall

Why

To make troubleshooting/debugging of integrations easier

How

🤖 Generated by Copilot at 2d0fa4d

  • Remove force option from sync method of IntegrationRepository class to prevent table dropping and data loss (link)

Checklist ✅

  • Label appropriately with Feature, Improvement, or Bug.
  • Add screehshots to the PR description for relevant FE changes
  • New backend functionality has been unit-tested.
  • API documentation has been updated (if necessary) (see docs on API documentation).
  • Quality standards are met.

@sausage-todd sausage-todd added the Improvement Created by Linear-GitHub Sync label May 12, 2023
@sausage-todd sausage-todd requested a review from themarolt May 12, 2023 09:12
@joanreyero joanreyero self-requested a review May 12, 2023 09:19
Copy link
Copy Markdown
Contributor

@joanreyero joanreyero left a comment

Choose a reason for hiding this comment

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

Congrats on your first PR! 🚀

Curious about what will happen if we:

  • connect an integration
  • disconnect
  • connect again

Could you send this to staging and try it out?

@sausage-todd sausage-todd force-pushed the improvement/soft-delete-integrations branch from 2d0fa4d to 9edd595 Compare May 12, 2023 09:23
@sausage-todd
Copy link
Copy Markdown
Contributor Author

These are very good questions, let me see

Comment thread backend/src/database/repositories/integrationRepository.ts
@sausage-todd
Copy link
Copy Markdown
Contributor Author

sausage-todd commented May 15, 2023

Ok, so here's how it works:

  1. When connecting again a previously disconnected integration, we create a new entry in the database, while keeping the soft-deleted one untouched.
    • I believe this is a good approach, because it should make troubleshooting easier for us by keeping potentially broken data intact.
  2. nodejs-worker doesn't pick up soft-deleted integrations
    • I tried it both on staging and locally. According to logs, it doesn't start processing soft-deleted integrations
    • It makes sense, because nodejs-worker uses IntegrationRepository.findAllActive() for retrieving integrations to be processed, and it still uses Sequelize.
  3. Soft-deleted integrations are ignored everywhere else as well

cc @joanreyero @themarolt

@sausage-todd sausage-todd merged commit f1ae82c into main May 15, 2023
@sausage-todd sausage-todd deleted the improvement/soft-delete-integrations branch May 15, 2023 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Improvement Created by Linear-GitHub Sync

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants