chore(j-s): Rollback subpoena entry if we get an error while creating it #16155
Conversation
WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16155 +/- ##
==========================================
- Coverage 36.64% 36.59% -0.06%
==========================================
Files 6769 6770 +1
Lines 139435 139652 +217
Branches 39656 39713 +57
==========================================
+ Hits 51100 51109 +9
- Misses 88335 88543 +208
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportBranch report: ✅ 0 Failed, 21271 Passed, 0 Skipped, 20m 2.73s Total Time |
| if (!createdSubpoena) { | ||
| this.logger.error('Failed to create subpoena file for police') | ||
| return { delivered: false } | ||
| } |
There was a problem hiding this comment.
I am not absolutely sure, but I don't think this will rollback the transaction. I think you need to throw an exception for that.
There was a problem hiding this comment.
I checked the documentation and my understanding is correct. You need to throw to trigger a rollback. The catch clause below returns the right delivered status so that should be fine.
There was a problem hiding this comment.
Ok weird because I tried it locally and it rolled back! I'll try again tomorrow to confirm but I can always add in an exception just in case
There was a problem hiding this comment.
Hér er skjölunin fyrir þetta:
try {
const transaction = await sequelize.transaction();
const user = await User.findOne(..., { transaction });
await user.update(..., { transaction });
await transaction.commit();
} catch(err) {
await transaction.rollback();
}try {
await sequelize.transaction(transaction => { // Note that we pass a callback rather than awaiting the call with no arguments
const user = await User.findOne(..., {transaction});
await user.update(..., {transaction});
// Committed
} catch(err) {
// Rolled back
console.error(err);
}Þú getur annað hvort búið til transaction og séð sjálf um commit og rollback, eða sent callback inn í transaction fallið og látið transaction fallið sjá um commit og rollback. Til að fallið geti rollbackað callbackið er það væntanlega með try/catch utan um callbackið og þess vegna þarf throw.
There was a problem hiding this comment.
Aa ok ég veit af hverju þetta virkaði þegar ég prófaði locally - af því þá var createsubpoena kallið að kasta exception ekki skila villu eða tómu svari þannig ég datt ekkert inn í if(!created)
There was a problem hiding this comment.
Í rauninni gæti ég bara tekið þessa if setningu alveg út, er að kasta exceptions ef eitthvað klikkar í lögreglukallinu þannig þetta er eiginlega unreachable kóði
| defendant: Defendant, | ||
| transaction: Transaction, | ||
| ): Promise<Subpoena> { | ||
| return await this.subpoenaModel.create( |
There was a problem hiding this comment.
You don't have to await here, so you may consider removing the await.
unakb
added
the
deprecated:automerge
kodiakhq
bot
removed
the
deprecated:automerge
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
unakb
added
the
deprecated:automerge
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
kodiakhq
bot
removed
the
deprecated:automerge
What
Handle police creation errors by rolling back a transaction so we don't flood the db with failed subpoenas
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes