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

Deprecate public visibility of order#finalize! #4260

Conversation

waiting-for-dev
Copy link
Contributor

@waiting-for-dev waiting-for-dev commented Feb 8, 2022

Description

The only invocation of order#finalize! comes from the state machine
transaction to complete:

after_transition to: :complete, do: :finalize!

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.

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)
  • I have attached screenshots to this PR for visual changes (if needed)

Copy link
Member

@spaghetticode spaghetticode left a 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 🎉

Copy link
Member

@jarednorman jarednorman left a 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?

@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev/make_sense_of_finalizing_tests branch from 7fe17cd to 5f22628 Compare February 9, 2022 04:31
@waiting-for-dev
Copy link
Contributor Author

Should we consider this a breaking change, mark it for the next major release?

Good point. I've updated the code with the following:

  • Restored public visibility of order#finalized!.
  • Added # @api private yard tag.
  • Added a TODO: note for 4.0.

I think this way we can go ahead and merge it, leaving for the future the actual deprecation.

@waiting-for-dev waiting-for-dev changed the title Make order#finalize! private Deprecate public visibility of order#finalize! Feb 9, 2022
@waiting-for-dev
Copy link
Contributor Author

waiting-for-dev commented Feb 9, 2022

After a meeting with the Core Team, we have decided to do a smarter deprecation by renaming the current #finalize! to #finalize, calling the latter from the state machine, and rendering a deprecation message, while still forwarding to the new method, in the former. I'll work on it shortly.

@jarednorman
Copy link
Member

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.
@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev/make_sense_of_finalizing_tests branch from 5f22628 to a249b34 Compare February 10, 2022 04:05
@waiting-for-dev
Copy link
Contributor Author

waiting-for-dev commented Feb 10, 2022

Done! cc @spaghetticode @kennyadsl

@waiting-for-dev waiting-for-dev merged commit 68e97f3 into solidusio:master Feb 11, 2022
@waiting-for-dev waiting-for-dev deleted the waiting-for-dev/make_sense_of_finalizing_tests branch February 11, 2022 04:14
waiting-for-dev added a commit to solidusio/solidus_subscriptions that referenced this pull request May 24, 2022
waiting-for-dev added a commit to solidusio/solidus_subscriptions that referenced this pull request May 24, 2022
waiting-for-dev added a commit to solidusio/solidus_subscriptions that referenced this pull request May 24, 2022
kennyadsl added a commit to nebulab/solidus that referenced this pull request Mar 21, 2023
Please, use Spree::Order#complete! now.

Ref solidusio#4260
kennyadsl added a commit to nebulab/solidus that referenced this pull request Mar 27, 2023
Please, use Spree::Order#complete! now.

Ref solidusio#4260
kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 12, 2023
Please, use Spree::Order#complete! now.

Ref solidusio#4260
kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 14, 2023
Please, use Spree::Order#complete! now.

Ref solidusio#4260
kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 18, 2023
Please, use Spree::Order#complete! now.

Ref solidusio#4260
kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 19, 2023
Please, use Spree::Order#complete! now.

Ref solidusio#4260
kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 24, 2023
Please, use Spree::Order#complete! now.

Ref solidusio#4260
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