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

feat(Model.delete): add suport for transactions #373

Conversation

iam4x
Copy link

@iam4x iam4x commented Aug 31, 2018

Proposed changes

Support transaction as parameter of Model.delete(trx)

Checklist

  • I have read the CONTRIBUTING doc
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

fix(proxyHandler): avoid `isDeleted` check on `$transaction`

style(standard): run yarn lint --fix

dada
@iam4x iam4x force-pushed the feat/add-transaction-support-on-delete branch from 4e3fd18 to c53726b Compare August 31, 2018 13:37
@thetutlage
Copy link
Member

We need better tests here. So everytime the delete method is called, we freeze the model to avoid any future mutations. Now, if the transaction was rollback, then we have to make sure that model instance is not in frozen state.

Feel free to ask more questions if you have.

@thetutlage thetutlage self-assigned this Sep 6, 2018
@iam4x
Copy link
Author

iam4x commented Sep 6, 2018

Hey @thetutlage I've pushed some "hack" in order to do that, I'm not really happy with it.

We have a fork we are using with afterTransaction lifecycle hooks where we can do this kind of stuff in a better way:

Model.addHook(
  'afterTransaction',
  (modelInstance: adonis$ModelInstance, didCommit: boolean) => {
    if (didCommit) {
      return modelInstance.isDeleted
        ? modelInstance.unsyncWithAlgolia()
        : modelInstance.syncWithAlgolia();
    }
  }
);

I will be able to re-do a PR when we will have this merged into adonis-lucid, but we are waiting for your answer on #365

For now, I think this will do 👍


// Override `rollback` method
// in order to `unfreeze` the model after the transaction
const rollback = trx.rollback
Copy link
Member

Choose a reason for hiding this comment

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

I believe this will break, if I pass the same trx to multiple delete calls

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I'm not really happy with this (cf my comment #373 (comment)) do you have any other idea?

@thetutlage
Copy link
Member

Closing in favor of v5

@thetutlage thetutlage closed this Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants