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

Ecto Multi Support Proposition #62

Merged
merged 3 commits into from
Dec 16, 2019

Conversation

DiscoStarslayer
Copy link
Contributor

Initial rough draft of a PaperTrail.Multi API that wraps Ecto.Multi to allow creating multi step transactions using a pipeline.

Inspiration taken from this ticket #48.

This PR extracts the logic from the PaperTrail module and breaks it down into transaction creation + committing steps in PaperTrail.Multi. These functions provide the Multi API and are then reused within PaperTrail to implement the non-Multi functions.

Since even the non-Multi functions require a transaction to apply the paper trail logic, I thought this might be a nice approach.

API wise, it's almost 1-to-1 with Ecto.Multi, the only exceptions are:

  • no delete_all
  • no insert_all
  • no insert_or_update
  • no update_all
  • Multi transaction should be committed to the DB with PaperTrail.Multi.commit instead of simply calling Repo.transaction on the multi chain.

There is no tests or documentation, but the existing tests pass fully which I think makes a good smoke test as insert, update, and delete are re-implemented using the Multi-api.

I figured I would open this now and see if I'm barking up the right tree here, and if so I will continue to add tests and documentation around the feature.

@gshaw
Copy link

gshaw commented Nov 10, 2019

Not having Multi support is a deal breaking for using paper_trail so I'm happy to see this work started. 🎉

Will this API allow inserting a versioned and non-versioned record in the same transaction? How would it look?

PaperTrail.Multi.new()
|> PaperTrail.Multi.insert(versioned_changeset, options)
|> Multi.insert(non_versioned_changeset_in_different_table) # would this work?
|> PaperTrail.Multi.commit()

@gshaw
Copy link

gshaw commented Nov 10, 2019

@DiscoStarslayer I tried your branch and the above sample code works exactly as I had hoped. Great work, I hope this PR will move forward and get released. I'll keep using this branch to help with the testing and will comment back if I find any problems.

Thanks for getting this going. Much appreciated.

@DiscoStarslayer
Copy link
Contributor Author

DiscoStarslayer commented Nov 11, 2019

@gshaw That's good to hear! I was going to comment saying theoretically there is no reason why that use case should not work as this is simply reusing the existing Ecto.Multi struct. Great to hear you tested it out and confirmed it though!

@izelnakri izelnakri marked this pull request as ready for review December 15, 2019 17:26
@izelnakri
Copy link
Owner

Hi @DiscoStarslayer, I just noticed your PR. Im reviewing it now, thank you for all the work and sorry for responding late!

@izelnakri
Copy link
Owner

This is awesome @DiscoStarslayer! Proposed changes enhance the usability of the library without adding much complexity. Frankly we should have done this earlier :) I'm merging this PR and cut a release!

@izelnakri izelnakri merged commit 86e4f4c into izelnakri:master Dec 16, 2019
@DiscoStarslayer DiscoStarslayer deleted the ecto-multi-support branch December 19, 2019 15:36
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

3 participants