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

Wrap paddle actions with DB transactions #2558

Merged
merged 4 commits into from
Jan 3, 2023

Conversation

aerosol
Copy link
Member

@aerosol aerosol commented Jan 3, 2023

Changes

This change makes all the paddle webhook updates transactional.
I considered rewriting it to Ecto.Multi-based services but that can be made on top of this in another iteration, since shipping the change itself feels quite critical.

Tests

  • Automated tests have been added
  • This PR does not require tests

Changelog

  • Entry has been added to changelog
  • This PR does not make a user-facing change

Documentation

  • Docs have been updated
  • This change does not need a documentation update

Dark mode

  • The UI has been tested both in dark and light mode
  • This PR does not change the UI

@aerosol aerosol marked this pull request as ready for review January 3, 2023 13:35
@aerosol aerosol requested review from ukutaht and vinibrsl and removed request for ukutaht January 3, 2023 13:35
Copy link
Contributor

@vinibrsl vinibrsl left a comment

Choose a reason for hiding this comment

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

Good one! And I agree that moving to Ecto.Multi is a good idea and we should do it in a separate release.

@bundlemon
Copy link

bundlemon bot commented Jan 3, 2023

BundleMon

Unchanged files (7)
Status Path Size Limits
static/css/app.css
515.19KB -
static/js/dashboard.js
297.45KB -
static/js/app.js
12.13KB -
static/js/embed.host.js
5.58KB -
static/js/embed.content.js
5.06KB -
tracker/js/plausible.js
748B -
static/js/applyTheme.js
314B -

No change in files bundle size

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

@aerosol aerosol marked this pull request as draft January 3, 2023 13:46
@aerosol aerosol marked this pull request as ready for review January 3, 2023 14:02
@aerosol aerosol requested a review from vinibrsl January 3, 2023 14:02
@aerosol
Copy link
Member Author

aerosol commented Jan 3, 2023

@vinibrsl made the errors bubble up so it rolls back this time

@aerosol aerosol requested a review from ukutaht January 3, 2023 14:03
Copy link
Contributor

@ukutaht ukutaht left a comment

Choose a reason for hiding this comment

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

Looks good!

Is there any benefit between Ecto.Multi vs Repo.transaction? If I understand right it's just cosmetic / style difference or is there more to it?

@aerosol
Copy link
Member Author

aerosol commented Jan 3, 2023

Is there any benefit between Ecto.Multi vs Repo.transaction? If I understand right it's just cosmetic / style difference or is there more to it?

Repo.transaction accepts Ecto.Multi structs. The way I see it there's testability benefit - you can test some aspects without having to provision the DB; sometimes all you care about is whether the transaction structure/context has been built valid, not whether the database works. You also get better insight into failures, the failing step is explicitly named & returned. It makes it simpler to mock (or skip) certain transaction steps if need be. You can compose multiple Multis 😈 together with little to no effort - this supports building "service oriented" code bases.

@aerosol aerosol merged commit 44afbaa into master Jan 3, 2023
@aerosol aerosol deleted the make-paddle-webhook-updates-transactional branch January 3, 2023 15:14
aerosol added a commit that referenced this pull request Jan 4, 2023
* Wrap paddle actions with DB transactions

* Bow to credo

* Make the paddle transactions crash when applicable

* s/changeset/details
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.

3 participants