-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
In-memory order updater #5872
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
base: main
Are you sure you want to change the base?
In-memory order updater #5872
Conversation
27f19a8 to
340032c
Compare
d22210b to
d244646
Compare
|
Just making a note that we are waiting on Alistair to rebase #6026 against this. @AlistairNorman could you just make sure that gets done at some point before the 20th? That way we can have it for our next session. |
a3bb1fc to
06e3a2a
Compare
9fd5c8d to
27e4988
Compare
4a0f623 to
90f5420
Compare
While working on the in-memory updater in solidusio#5872, we found the need to change how item totals were being calculated, so that we could mark adjustments for destruction without actually destroying them, while still keeping tax adjustments intact. This change is completely backwards-compatible with the current OrderUpdater, so to reduce the scope of our PR, we wanted to make this change separately. Since the OrderUpdater is already very large, this helps reduce its responsibilities and makes it easier to test this behaviour. We don't see it as necessary to make this a configurable class, but this change leaves that option open in the future. Co-authored-by: Adam Mueller <[email protected]> Co-authored-by: Alistair Norman <[email protected]> Co-authored-by: Chris Todorov <[email protected]> Co-authored-by: Harmony Bouvier <[email protected]> Co-authored-by: Sofia Besenski <[email protected]> Co-authored-by: benjamin wil <[email protected]>
While working on the in-memory updater in solidusio#5872, we found the need to change how item totals were being calculated, so that we could mark adjustments for destruction without actually destroying them, while still keeping tax adjustments intact. This change is completely backwards-compatible with the current OrderUpdater, so to reduce the scope of our PR, we wanted to make this change separately. Since the OrderUpdater is already very large, this helps reduce its responsibilities and makes it easier to test this behaviour. We don't see it as necessary to make this a configurable class, but this change leaves that option open in the future. Co-authored-by: Adam Mueller <[email protected]> Co-authored-by: Alistair Norman <[email protected]> Co-authored-by: Chris Todorov <[email protected]> Co-authored-by: Harmony Bouvier <[email protected]> Co-authored-by: Sofia Besenski <[email protected]> Co-authored-by: benjamin wil <[email protected]>
While working on the in-memory updater in solidusio#5872, we found the need to change how item totals were being calculated, so that we could mark adjustments for destruction without actually destroying them, while still keeping tax adjustments intact. This change is completely backwards-compatible with the current OrderUpdater, so to reduce the scope of our PR, we wanted to make this change separately. Since the OrderUpdater is already very large, this helps reduce its responsibilities and makes it easier to test this behaviour. We don't see it as necessary to make this a configurable class, but this change leaves that option open in the future. Co-authored-by: Adam Mueller <[email protected]> Co-authored-by: Alistair Norman <[email protected]> Co-authored-by: Chris Todorov <[email protected]> Co-authored-by: Harmony Bouvier <[email protected]> Co-authored-by: Sofia Besenski <[email protected]> Co-authored-by: benjamin wil <[email protected]>
While working on the in-memory updater in solidusio#5872, we found the need to change how item totals were being calculated, so that we could mark adjustments for destruction without actually destroying them, while still keeping tax adjustments intact. This change is completely backwards-compatible with the current OrderUpdater, so to reduce the scope of our PR, we wanted to make this change separately. Since the OrderUpdater is already very large, this helps reduce its responsibilities and makes it easier to test this behaviour. We don't see it as necessary to make this a configurable class, but this change leaves that option open in the future. Co-authored-by: Adam Mueller <[email protected]> Co-authored-by: Alistair Norman <[email protected]> Co-authored-by: Chris Todorov <[email protected]> Co-authored-by: Harmony Bouvier <[email protected]> Co-authored-by: Sofia Besenski <[email protected]> Co-authored-by: benjamin wil <[email protected]>
9ecbd13 to
92fc679
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5872 +/- ##
==========================================
+ Coverage 89.46% 89.53% +0.07%
==========================================
Files 977 980 +3
Lines 20397 20543 +146
==========================================
+ Hits 18248 18394 +146
Misses 2149 2149 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
92fc679 to
e7faef3
Compare
f3d92a4 to
a5ed6c6
Compare
d0322b2 to
07add2a
Compare
d22dbf5 to
770e527
Compare
770e527 to
c8e434e
Compare
7bf4adb to
2d497bd
Compare
67186f3 to
defde7e
Compare
5423a0e to
62209cf
Compare
2b60e3b to
083fafe
Compare
6db349f to
0e6c01d
Compare
9f6ddee to
bb4fc40
Compare
bb4fc40 to
ea5be0c
Compare
ea5be0c to
d15a265
Compare
d15a265 to
0ffb769
Compare
Now that we are going to be using parts of this gem in our monitoring class, we need it to be required everywhere. This change also ensures the configuration is shared across the Solidus gems, which previously was not the case. Co-authored-by: benjamin wil <[email protected]> Co-authored-by: Sofia Besenski <[email protected]>
In subsequent commits we'll ensure that this can update orders in memory, without persisting changes using manipulative DB queries. Co-authored-by: Adam Mueller <[email protected]> Co-authored-by: Alistair Norman <[email protected]> Co-authored-by: Andrew Stewart <[email protected]> Co-authored-by: benjamin wil <[email protected]> Co-authored-by: Chris Todorov <[email protected]> Co-authored-by: Harmony Evangelina <[email protected]> Co-authored-by: Jared Norman <[email protected]> Co-authored-by: Kendra Riga <[email protected]> Co-authored-by: Nick Van Doorn <[email protected]> Co-authored-by: Noah Silvera <[email protected]> Co-authored-by: Senem Soy <[email protected]> Co-authored-by: Sofia Besenski <[email protected]> Co-authored-by: Tom Van Manen <[email protected]>
We don't want our in-memory order updater to have to persist shipment state. Because we no longer rely on `Shipment#update_state` which uses `update_column`, we are now logging any additional `Shipment#state` changes when the order is saved. The changes to the specs reflect this change in behaviour. Co-authored-by: Jared Norman <[email protected]> Co-authored-by: Noah Silvera <[email protected]> Co-authored-by: Alistair Norman <[email protected]>
We want our new in-memory order updater to be able to persist or not persist changes to the order record. This is a first step in ensuring we don't need to write to the database using the order updater. Clearly we have more work to do to ensure this functions like the existing updater. Co-authored-by: Adam Mueller <[email protected]> Co-authored-by: Andrew Stewart <[email protected]> Co-authored-by: Harmony Evangelina <[email protected]> Co-authored-by: Jared Norman <[email protected]> Co-authored-by: Nick Van Doorn <[email protected]> Co-authored-by: Noah Silvera <[email protected]> Co-authored-by: Senem Soy <[email protected]> Co-authored-by: Sofia Besenski <[email protected]> Co-authored-by: Tom Van Manen <[email protected]>
We have prevented write calls to update the cost and `updated_at` of a shipment, as well as allowed us to conditionally persist item totals, by passing down the `persist` argument to that method. Co-authored-by: Adam Mueller <[email protected]> Co-authored-by: Alistair Norman <[email protected]> Co-authored-by: Andrew Stewart <[email protected]> Co-authored-by: Harmony Evangelina <[email protected]> Co-authored-by: Kendra Riga <[email protected]> Co-authored-by: Jared Norman <[email protected]> Co-authored-by: Tom Van Manen <[email protected]> Co-authored-by: Chris Todorov <[email protected]> Co-authored-by: Sofia Besenski <[email protected]> Co-authored-by: Senem Soy <[email protected]> Co-authored-by: benjamin wil <[email protected]>
This puts all the update and recalculate methods together. Co-Authored-By: Adam Mueller <[email protected]> Co-Authored-By: benjamin wil <[email protected]> Co-Authored-By: Andrew Stewart <[email protected]> Co-Authored-By: Harmony Bouvier <[email protected]> Co-Authored-By: Jared Norman <[email protected]> Co-Authored-By: Kendra Riga <[email protected]> Co-Authored-By: Sofia Besenski <[email protected]> Co-Authored-By: Chris Todorov <[email protected]> Co-Authored-By: Tom Van Manen <[email protected]> Co-Authored-By: Noah Silvera <[email protected]>
Includes a small refactor to the internal recalculate method to simplify the code while maintaining the existing logic around only persisting when the values have changed. We'll use this persist flag to eventually only save changes to the DB when requested. Allowing us to use this adjuster to update the order in-memory. Co-authored-by: An Stewart <[email protected]> Co-authored-by: Harmony Evangelina <[email protected]> Co-authored-by: Kendra Riga <[email protected]> Co-authored-by: Nick Van Doorn <[email protected]> Co-authored-by: Sofia Besenski <[email protected]> Co-authored-by: benjamin wil <[email protected]>
Similar to the previous change. We're now passing the persist flag down to all promotion order adjusters. This does not implement the logic within the individual adjuster classes to skip persistance when required, only ensures the flag is pass down from our in-memory order updater. Co-Authored-By: Adam Mueller <[email protected]> Co-Authored-By: Andrew Stewart <[email protected]> Co-Authored-By: Harmony Evangelina <[email protected]> Co-Authored-By: Jared Norman <[email protected]> Co-Authored-By: Kendra Riga <[email protected]> Co-Authored-By: Senem Soy <[email protected]> Co-Authored-By: benjamin wil <[email protected]>
Previously this would update the eligible column. We now only assign the value and then save if persist is true. Co-Authored-By: Adam Mueller <[email protected]> Co-Authored-By: Andrew Stewart <[email protected]> Co-Authored-By: Harmony Evangelina <[email protected]> Co-Authored-By: Jared Norman <[email protected]> Co-Authored-By: Kendra Riga <[email protected]> Co-Authored-By: benjamin wil <[email protected]> Co-Authored-By: Chris Todorov <[email protected]> Co-Authored-By: Nick Van Doorn <[email protected]> Co-Authored-By: Sofia Besenski <[email protected]>
Integration level test using the InMemoryOrderUpdater to ensure we are not persisting changes during the promotion recalculations. Co-authored-by: Chris Todorov <[email protected]> Co-authored-by: benjamin wil <[email protected]> Co-authored-by: An Stewart <[email protected]> Co-authored-by: Jared Norman <[email protected]>
This change introduces some high-level integration tests and a patch similar to the one for the regular order updater, which provides a compatibility layer for the legacy promotion system, specifically around the eligible flag on adjustments. We want to protect against manipulative database queries in the legacy promotion system. We also want to ensure that objects in memory have their attributes changed correctly but not persisted. Co-authored-by: Jared Norman <[email protected]> Co-authored-by: Kendra Riga <[email protected]> Co-authored-by: Adam Mueller <[email protected]> Co-authored-by: Alistair Norman <[email protected]> Co-authored-by: Senem Soy <[email protected]> Co-authored-by: Sofia Besenski <[email protected]> Co-authored-by: Noah Silvera <[email protected]> Co-authored-by: Chris Todorov <[email protected]>
This will be used by the in-memory order updater to log manipulative queries when the persist flag is set to false. This will inform application developers whether their application is complying with the in-memory order updater's "no writes" philosophy. Previously the `db-query-matchers` gem we use to check for manipulative queries was only used in tests. However, we now use it in application code so the dependency must be moved to the gemspec. Co-authored-by: Alistair Norman <[email protected]> Co-authored-by: Sofia Besenski <[email protected]> Co-authored-by: Adam Mueller <[email protected]> Co-authored-by: Chris Todorov <[email protected]> Co-authored-by: Jared Norman <[email protected]> Co-authored-by: benjamin wil <[email protected]> Co-authored-by: Noah Silvera <[email protected]>
Enabling manipulative query logging will show the number of manipulative queries on each call to Spree::InMemoryOrderUpdater#recalculate. Co-authored-by: Chris Todorov <[email protected]> Co-authored-by: Adam Mueller <[email protected]> Co-authored-by: Alistair Norman <[email protected]> Co-authored-by: Senem Soy <[email protected]> Co-authored-by: benjamin wil <[email protected]>
Sets up some shared examples so that we can run the full promotion integration suite against both the existing order updater and the in-memory order updater. Co-authored-by: Jared Norman <[email protected]> Co-authored-by: Noah Silvera <[email protected]>
We no longer want to have individual methods use a persist: flag, instead we can save at the end if needed. Co-authored-by: Chris Todorov <[email protected]> Co-authored-by: Senem Soy <[email protected]> Co-authored-by: benjamin wil <[email protected]>
We no longer want to have individual methods use a persist: flag, instead we can save at the end if needed.
On the classic order updater, we were persisting updates to shipment and payment states in the respective recalculate methods. We are no longer persisting changes in those methods; now we only save changes in `#persist_totals`, so this is where we should be logging. Co-authored-by: Senem Soy <[email protected]> Co-authored-by: Adam Mueller <[email protected]> Co-authored-by: Kendra Riga <[email protected]>
0ffb769 to
2a95c04
Compare
Summary
This pull request proposes a replacement to the default
Spree::OrderUpdaterthat has new and improved functionality.We don't expect this to be the default order updater implementation in the next minor version of Solidus, but we would like to propose it as the default for the next major version of Solidus.
Note: The commits on this pull request have a long list of co-authors, as the Super Good team is approaching this as a collaborative mob programming exercise.
Below we'll add some more context about exactly what benefits the in-memory order updater brings to Solidus.
Increased performance for order updates
By default, the new order updater make fewer write calls to the database. This means that order- and checkout-related controller actions can resolve faster, with fewer round trips to the database server whenever an order is updated (which is a very often).
Order update simulation
By passing
persist: false, to the in-memory order updater, an order can be recalculated and rolled back to its last state without any database writes. This enables1 stores to let administrators and customers preview changes to orders before accepting those changes.We know that many stores add custom features and enhancements to the order model and the order updater, so we have also added a database write query monitor that warns developers if their application code is making unintended writes when
persist: falsehas been set.Test performance, deeper test coverage
Because order updates require fewer round trips, this also benefits the Solidus test suite: the main side effect is that any order scaffolding and factories should just be faster.
This pull request includes additional test coverage against the current order updater as well as test coverage for the new in-memory order updater. This includes integration test coverage against both
solidus_promotionsandsolidus_legacy_promotions.Anatomy of this pull request
Obviously, this is a massive change. We appreciate your careful review, and if you're confused about any of these changes please leave your comments and questions. We touch a lot code across the Solidus meta gem, but all of our changes are in service of:
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
Footnotes
By enables we mean that application developers can build features that preview order changes. But as of this implementation, no part of
solidus_admin,solidus_backend, or the Solidus starter frontend make use of this functionality. ↩