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

Make order-related service objects configurable #4138

Merged
merged 3 commits into from
Aug 9, 2021

Conversation

aldesantis
Copy link
Member

@aldesantis aldesantis commented Jul 30, 2021

Description

Solidus has a few "service objects" that are used to manage orders:

  • Spree::OrderContents, for managing the contents and the state of an order,
  • Spree::OrderShipping, for shipping a completed order, and
  • Spree::OrderCancellations, for managing a completed order's inventory units.

In custom stores, it's common to customize these classes to inject your own logic/checks in different flows.

This PR makes it possible to replace these classes with a custom implementation without monkey-patching the default implementations or the corresponding builders on Spree::Order.

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)

@aldesantis aldesantis self-assigned this Jul 30, 2021
@aldesantis aldesantis marked this pull request as ready for review July 30, 2021 16:20
@aldesantis aldesantis requested a review from a team July 30, 2021 16:21
@waiting-for-dev
Copy link
Contributor

Allowing a neat way to replace services is definitely a must!! Related to that, I'd like to point out a POC we have on the Nebulab's fork: nebulab#115

Even if merging this PR will definitely be an improvement, maybe we'd like to consider if it's worth it to wait until we have a definitive way to inject services to avoid requiring users to change their code again. If we add more service objects, having an auto-registration mechanism would help to have our codebase cleaner. On top of that, I think there's a benefit to having the injection of objects separated from the more generic configuration.

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that we are referencing these classes only in this file! Good stuff, thanks Ale.

@kennyadsl
Copy link
Member

Even if merging this PR will definitely be an improvement, maybe we'd like to consider if it's worth it to wait until we have a definitive way to inject services to avoid requiring users to change their code again. If we add more service objects, having an auto-registration mechanism would help to have our codebase cleaner. On top of that, I think there's a benefit to having the injection of objects separated from the more generic configuration.

@waiting-for-dev this is definitely interesting but I'd wait to have something in place for injection and switch all the existing preference classes over there in one shot when it's time.

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.

@aldesantis thanks 👍

@kennyadsl kennyadsl merged commit 64ec398 into solidusio:master Aug 9, 2021
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.

4 participants