-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Deprecate public visibility of order#finalize! #4260
Deprecate public visibility of order#finalize! #4260
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Marc, I especially ❤️ the removal of some expect to receive
on the object under testing in favor of testing the real behavior 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider this a breaking change, mark it for the next major release?
7fe17cd
to
5f22628
Compare
Good point. I've updated the code with the following:
I think this way we can go ahead and merge it, leaving for the future the actual deprecation. |
order#finalize!
private
After a meeting with the Core Team, we have decided to do a smarter deprecation by renaming the current |
Cool, that makes sense to me. |
The only invocation of `order#finalize!` comes from the state machine transaction to `complete`: https://github.com/solidusio/solidus/blob/e18f34541bcb9b396cc026f9579d01bfae12182e/core/lib/spree/core/state_machines/order.rb#L122 Calling it standalone should not be encouraged, as all the safety checks implemented as before hooks would be skipped. As a deprecation strategy, we're renaming the old `#finalize!` method to `#finalize` and updating the state machine to call the latter. In turn, `#finalize!` deprecates but still delegates to the new method. Consequently, we're updating the test suite only to reference the public API. I.e, `Spree::Order#complete!`. That has forced us to refactor some tests. From `core/spec/models/spree/order/finalizing_spec.rb`: - `should set completed_at`: Before being called with `:completed_at` as an argument, `touch` is called by the state machine transition. It's easier if we just assert on the field value. - `should sell inventory units`: During the transaction, the shipment's `#object_id` gets changed, so we can't longer assert on those instances. Instead, we test the actual behavior. The assertion about `#update_state` is already covered in the following test. - `should change the shipment state to ready if order is paid`: We take the ocassion to assert on the actual behavior. - `should freeze all adjustments`: There're other methods called on the collection of adjustments that are not covered by the double. It's easier to test the actual behavior. Besides, we're moving some tests about the order completion from `order_spec.rb` to `finalizing_spec.rb`, also adapting them to test `#complete!` instead of `finalize!`. These added tests explain the removal of others which were simple duplication.
5f22628
to
a249b34
Compare
Done! cc @spaghetticode @kennyadsl |
Please, use Spree::Order#complete! now. Ref solidusio#4260
Please, use Spree::Order#complete! now. Ref solidusio#4260
Please, use Spree::Order#complete! now. Ref solidusio#4260
Please, use Spree::Order#complete! now. Ref solidusio#4260
Please, use Spree::Order#complete! now. Ref solidusio#4260
Please, use Spree::Order#complete! now. Ref solidusio#4260
Please, use Spree::Order#complete! now. Ref solidusio#4260
Description
The only invocation of
order#finalize!
comes from the state machinetransaction to
complete
:solidus/core/lib/spree/core/state_machines/order.rb
Line 122 in e18f345
Calling it standalone should not be encouraged, as all the safety checks
implemented as before hooks would be skipped.
As a deprecation strategy, we're renaming the old
#finalize!
method to#finalize
and updating the state machine to call the latter. In turn,#finalize!
deprecates but still delegates to the new method.Consequently, we're updating the test suite only to reference the public
API. I.e,
Spree::Order#complete!
. That has forced us to refactor sometests. From
core/spec/models/spree/order/finalizing_spec.rb
:should set completed_at
: Before being called with:completed_at
as an argument,
touch
is called by the state machine transition. It'seasier if we just assert on the field value.
should sell inventory units
: During the transaction, the shipment's#object_id
gets changed, so we can't longer assert on those instances.Instead, we test the actual behavior. The assertion about
#update_state
is already covered in the following test.should change the shipment state to ready if order is paid
: We takethe ocassion to assert on the actual behavior.
should freeze all adjustments
: There're other methods called on thecollection of adjustments that are not covered by the double. It's
easier to test the actual behavior.
Besides, we're moving some tests about the order completion from
order_spec.rb
tofinalizing_spec.rb
, also adapting them to test#complete!
instead offinalize!
. These added tests explain theremoval of others which were simple duplication.
Checklist: