spree-contrib / spree_multi_vendor

Spree marketplace extension. Create your own marketplace on top of Spree Commerce
https://spreecommerce.org/marketplace-ecommerce/
BSD 3-Clause "New" or "Revised" License
142 stars 135 forks source link

Fixes #162 and #160 #172

Closed hoshinotsuyoshi closed 3 years ago

hoshinotsuyoshi commented 3 years ago

Make order-complete hook methods(generate_order_commissions and send_notification_mails_to_vendors) be called in Spree::Order#finalize! instead of using after_transition. fixes https://github.com/spree-contrib/spree_multi_vendor/issues/162 and https://github.com/spree-contrib/spree_multi_vendor/issues/160


So far, even if you don't have problems in production env, but in development env(usually config.cache_class = false), you will run into problems such as https://github.com/spree-contrib/spree_multi_vendor/issues/162 or https://github.com/spree-contrib/spree_multi_vendor/issues/160 .

It is hard (or difiicult) to avoid re-registering after_transition.

For example, some workaround such as https://github.com/spree-contrib/spree_multi_vendor/issues/162#issuecomment-774948105 will be needed.

This patch approach is focusing on that the finalize! method is already set with after_transition to: :complete by spree/spree's core.

Calling spree-multi-vendor's hook methods in finalize (a stable method, I think) will be a simple solution.


Fixes #160 and #162 .

PR description

1.

1st commit 47cedb5eeaf08addc17897f0952d7554dbae48e8 https://github.com/spree-contrib/spree_multi_vendor/pull/172/commits/e1879a22c74fed8d079eb08d156cb532b91f7206 simply adds a spec(regression test). This spec is related to https://github.com/spree-contrib/spree_multi_vendor/issues/162 . And this spec will be passed.

2.

In spec/dummy/config/environments/test.rb, if you set config.cache_classes = false, the spec will be failed.

Dummy::Application.configure do
  # Settings specified here will take precedence over those in config/application.rb

  # The test environment is used exclusively to run your application's
  # test suite. You never need to work with it otherwise. Remember that
  # your test database is "scratch space" for the test suite and is wiped
  # and recreated between test runs. Don't rely on the data there!
-  config.cache_classes = true
+  config.cache_classes = false

And a spec https://github.com/spree-contrib/spree_multi_vendor/blob/bf37f0624cc1a5f7a3b7431136cd8416a54fed60/spec/models/spree/order_spec.rb#L24-L36 will be failed, too. (This spec is related to https://github.com/spree-contrib/spree_multi_vendor/issues/160 .)

4.

the 2nd commit https://github.com/spree-contrib/spree_multi_vendor/pull/172/commits/ab0dc2503a9d90b38a03ad5e85a1fb594e5029cc https://github.com/spree-contrib/spree_multi_vendor/pull/172/commits/a5333cfa0ac21010f1693f121a009c10f5d360bd will solve both.

hoshinotsuyoshi commented 3 years ago

Build failing is due to other flaky test.

I made just another PR to fix that.

https://github.com/spree-contrib/spree_multi_vendor/pull/173

I hope you can see it. @damianlegawiec

Thank you!

damianlegawiec commented 3 years ago

Thank you @hoshinotsuyoshi 🙏