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

Introduce a null promotion configuration #5667

Merged
merged 4 commits into from
Feb 27, 2024

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Feb 18, 2024

Summary

This adds a "null" promotion configuration, consisting of only those extension points for the promotion system that are used from within core (rather than from within the promotion system).

This is a nice exercise as it also tells us what extension points we need for any promo system: A class to help recalculate orders, and one to add coupon codes to orders.

This includes the commits from #5635 . I'll rebase once that is merged.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

@mamhoff mamhoff requested a review from a team as a code owner February 18, 2024 09:56
@github-actions github-actions bot added changelog:solidus_api Changes to the solidus_api gem changelog:solidus_backend Changes to the solidus_backend gem changelog:solidus_core Changes to the solidus_core gem labels Feb 18, 2024
@mamhoff mamhoff force-pushed the null-promo-config branch 2 times, most recently from a7f58db to dcc8f86 Compare February 18, 2024 22:31
Copy link

codecov bot commented Feb 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.64%. Comparing base (637dbad) to head (e29983f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5667      +/-   ##
==========================================
+ Coverage   88.62%   88.64%   +0.02%     
==========================================
  Files         688      691       +3     
  Lines       16493    16523      +30     
==========================================
+ Hits        14617    14647      +30     
  Misses       1876     1876              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Very nice work!

# frozen_string_literal: true

module Spree
class NullAdjuster
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a more specific name would help to understand the purpose of the object.

What about?

Spree::NullPromotionAdjuster

Copy link
Member

Choose a reason for hiding this comment

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

I second this question, unless you see a reason to keep it more generic. In that case, can you please help us understand where it is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My observation was that the class doesn't actually do anything promotion-specific, and the interface (#call) also doesn't necessarily do anything. I was suspecting (but wrongly) that also tax adjuster might share that interface.

Whatever: It's completely fine to call it NullPromotionAdjuster, and I've amended the PR to reflect that.

@tvdeyen
Copy link
Member

tvdeyen commented Feb 19, 2024

@mamhoff the PR title is a bit confusing, though. Mind to adjust it?

@mamhoff mamhoff changed the title Null promo config Introduce a null promotion configuration Feb 19, 2024
@mamhoff
Copy link
Contributor Author

mamhoff commented Feb 19, 2024

@mamhoff the PR title is a bit confusing, though. Mind to adjust it?

Done, thank you!

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.

Well done, thanks Martin, left a comment.

# frozen_string_literal: true

module Spree
class NullAdjuster
Copy link
Member

Choose a reason for hiding this comment

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

I second this question, unless you see a reason to keep it more generic. In that case, can you please help us understand where it is?

Spree::Core::PromotionConfiguration will have to live in the
solidus_legacy_promotions gem. We will need a stand-in! But for that
stand-in, we also need an extension point.
This allows us to have a stand-in for a promotion system when, in fact,
we don't have one.
This is a stand-in for all the promotion handlers we have. It implements
all the methods from the promotion handler "coupon".
We need a promotion configuration when solidus_legacy_promotions is out
of core. This provides it.
@github-actions github-actions bot removed changelog:solidus_api Changes to the solidus_api gem changelog:solidus_backend Changes to the solidus_backend gem labels Feb 24, 2024
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.

Great work. 👏

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.

👍 Thanks Martin, you rock!

@kennyadsl kennyadsl merged commit 2f2bd69 into solidusio:main Feb 27, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_core Changes to the solidus_core gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants