-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5872 +/- ##
==========================================
- Coverage 92.42% 88.76% -3.67%
==========================================
Files 389 831 +442
Lines 8005 18102 +10097
==========================================
+ Hits 7399 16069 +8670
- Misses 606 2033 +1427 ☔ View full report in Codecov by Sentry. |
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
Update implies that we are persisting the change in Rails, which this method does not do. Co-authored-by: Adam Mueller <[email protected]> Co-authored-by: Andrew Stewart <[email protected]> Co-authored-by: Benjamin Willems <[email protected]> Co-authored-by: Senem Soy <[email protected]> Co-authored-by: Sofia Besenski <[email protected]> Co-authored-by: Kendra Riga <[email protected]>
These methods don't persist so it's more accurate to say that they recalculate the total instead of saying that they update it. Co-Authored-By: Adam Mueller <[email protected]> Co-Authored-By: Benjamin Willems <[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]>
We want all the methods that might persist data to be called update_ instead of recalculate to be clear that they hit the database. Co-Authored-By: Adam Mueller <[email protected]> Co-Authored-By: Benjamin Willems <[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]>
This is calling the recalculate method not update_adjustments. Co-Authored-By: Adam Mueller <[email protected]> Co-Authored-By: Benjamin Willems <[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]>
This puts all the update and recalculate methods together. Co-Authored-By: Adam Mueller <[email protected]> Co-Authored-By: Benjamin Willems <[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]>
Co-authored-by: Adam Mueller <[email protected]> Co-authored-by: Andrew Stewart <[email protected]> Co-authored-by: Jared Norman <[email protected]> Co-authored-by: Noah Silvera <[email protected]> Co-authored-by: Benjamin Willems <[email protected]> Co-authored-by: Alistair Norman <[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]>
210eb32
to
89ea48b
Compare
We were missing the whole path of doing order level adjustments, e.g. The CreateDiscountedItem benefit.
89ea48b
to
ef7fe07
Compare
This makes working with unpersisted line items easier because required attributes are set immediately. This change is in service of creating an in memory order updater.
This is in service of the in-memory order updater. Co-authored-by: Adam <[email protected]> Co-authored-by: Alistair <[email protected]> Co-authored-by: An <[email protected]> Co-authored-by: Benjamin <[email protected]> Co-authored-by: Chris <[email protected]> Co-authored-by: Harmony <[email protected]> Co-authored-by: Jared <[email protected]> Co-authored-by: Nick <[email protected]> Co-authored-by: Noah <[email protected]> Co-authored-by: Senem <[email protected]> Co-authored-by: Sofia <[email protected]>
See code comments and spec changes. This is similar to how we handled this in the legacy promotion system. Co-authored-by: Adam <[email protected]> Co-authored-by: Alistair <[email protected]> Co-authored-by: An <[email protected]> Co-authored-by: Benjamin <[email protected]> Co-authored-by: Harmony <[email protected]> Co-authored-by: Jared <[email protected]> Co-authored-by: Nick <[email protected]> Co-authored-by: Senem <[email protected]> Co-authored-by: Sofia <[email protected]
We're seeing a spec failure related to changing setting required line item attributes from `before_validation` to `after_initialize`. We can "fix" the spec by setting a nil tax category, but this may not be appropriate. We've also added a TODO item regarding this change, as we think it may be a breaking change requiring further investigation. Co-authored-by: Noah Silvera <[email protected]> Co-authored-by: Kendra Chateau <[email protected]>
This is a similar change to the last commit. We still need to evaluate if this is appropriate spec setup. Co-authored-by: Noah Silvera <[email protected]> Co-authored-by: Kendra Riga <[email protected]>
Integration level test using the InMemoryOrderUpdater to ensure we are not persisting changes during the promotion recalculations. Co-authored-by: Chris <[email protected]> Co-authored-by: Benjamin <[email protected]> Co-authored-by: An <[email protected]> Co-authored-by: Jared <[email protected]>
We can just use the FactoryBot factories instead. Co-Authored-By: Benjamin <[email protected]> Co-Authored-By: Alistair <[email protected]>
Integration level test using the InMemoryOrderUpdater to ensure we are not persisting changes during the promotion recalculations. Co-authored-by: Chris <[email protected]> Co-authored-by: Benjamin <[email protected]> Co-authored-by: An <[email protected]> Co-authored-by: Jared <[email protected]>
ef7fe07
to
7cb94b6
Compare
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]>
Ran out of time so added this to fix next time.
7cb94b6
to
306eb45
Compare
Next steps:
|
This is now merged in main and we should be using it from the patch. Co-authored-by: Benjamin Willems <[email protected]> Co-authored-by: Noah Silvera <[email protected]> Co-authored-by: Kendra Riga <[email protected]>
In an associated change we made the `CreateDiscoutedItem` benefit create in-memory line-items. That causes an issue during the recalculation of the item totals. Co-authored-by: Benjamin Willems <[email protected]> Co-authored-by: Kendra Riga <[email protected]> Co-authored-by: Noah Silvera <[email protected]> Co-authored-by: Jared Norman <[email protected]>
This will be used by the in-memory order updater to log manipulative queries when the persist flag is set to false. Co-authored-by: Alistair <[email protected]> Co-authored-by: Sofia <[email protected]> Co-authored-by: Adam <[email protected]>
To-do: - Wrap recalculate in query monitor - Add tests Co-authored-by: Adam <[email protected]> Co-authored-by: Sofia <[email protected]> Co-authored-by: Alistair <[email protected]>
Summary
This pull request proposes a replacement to the default
Spree::OrderUpdater
that has new and improved functionality:There may be some beneficial side-effects that come out of this new order updater implementation:
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.
Milestones
For this order updater, we intend to achieve the following during updates:
We appreciate that there is a lot of complexity to achieving these goals (dealing with active promotions, for example).
Notes
Spree::OrderUpdater
makes on a typical recalculate.Todo
recalculate_item_total
when line item totals changeInMemoryOrderUpdater
to not marked for destructionOrderUpdater
to not marked for destructionupdate_taxes
Write theInMemoryOrderAdjuster
(also, should we rename this toInMemoryOrderPromotionAdjuster
)LineItem#set_required_attributes
inafter_initialize
instead ofbefore_validation
. We think this may be a breaking change that requiresfurther investigation. (We investigated and don't believe it is a breaking change.)
ensure the above
promotions.order_adjuster_class
compute_amount
. We know the create quantity adjustments does. This action persists when compute_amount is called.persist: false
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: