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

Close #769 discount maximum cap #802

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Conversation

kimsrung
Copy link
Collaborator

@kimsrung kimsrung commented Nov 13, 2023

Form
image
image

Order detail
image

image

@kimsrung kimsrung requested a review from channainfo November 13, 2023 09:12
@kimsrung kimsrung self-assigned this Nov 13, 2023
@kimsrung kimsrung linked an issue Nov 13, 2023 that may be closed by this pull request
@kimsrung kimsrung force-pushed the 769-discount-maximum-cap branch 2 times, most recently from 0477616 to a69a413 Compare November 16, 2023 09:03
@theachoem
Copy link
Collaborator

theachoem commented Nov 16, 2023

It is great for whole order adjustment, but It will not work with item adjustment Or date-specific item adjustment.

Another approach we can do is putting CAP inside calculable of Spree::Calculator instead.

calculator is belongs_to :calculable. Caculatable here can be CreateAdjustment, CreateItemAdjustments, or CreateDateSpecificItemAdjustments.

Then, we can override compute method or override it logic based on a specific calculator:

def compute
   return super unless calculable.defined?(:cap)

   [super, calculable.cap].min
end

This will be a lot of change but please take it into consideration.

@kimsrung
Copy link
Collaborator Author

It is great for whole order adjustment, but It will not work with item adjustment Or date-specific item adjustment.

Another approach we can do is putting CAP inside calculable of Spree::Calculator instead.

calculator is belongs_to :calculable. Caculatable here can be CreateAdjustment, CreateItemAdjustments, or CreateDateSpecificItemAdjustments.

Then, we can override compute method or override it logic based on a specific calculator:

def compute
   return super unless calculable.defined?(:cap)

   [super, calculable.cap].min
end

This will be a lot of change but please take it into consideration.

From what I know, we apply cap to whole order only. We would get final price and calculate. And this cap would be on promotion only. In case you apply the promotion without cap, everything is still the same. We can write spec to verify this.

@kimsrung kimsrung force-pushed the 769-discount-maximum-cap branch 2 times, most recently from 99776a3 to 5eda9d5 Compare November 21, 2023 01:02
@kimsrung
Copy link
Collaborator Author

kimsrung commented Dec 7, 2023

bong @channainfo , after refactoring code I think we are no longer need cap on promotion. As of now, I still keep my old code but I would like to remove that after discussion.

Also these classes:

  • Promotion::Actions::CreateItemAdjustments
  • Promotion::Actions::CreateDateSpecificItemAdjustments
    are using the same calculator Spree::Calculator::PercentOnLineItem, so override compute function here would affect all.

as of ItemTotal adjustment, we shall modify in this Spree::Calculator::FlatPercentItemTotal

@theachoem, @channainfo we can try as Thea mentioned to modify in these 3 classes:

  • Promotion::Actions::CreateAdjustment
  • Promotion::Actions::CreateItemAdjustments
  • Promotion::Actions::CreateDateSpecificItemAdjustments
    but this shall affected in other calculators.
    For example, Promotion::Actions::CreateAdjustment could have many calculators such as flat_rate, flexi_rate, shipping, tiered_percent.... If we override method calculate in this class, I'm not sure if it would be the right place.

@kimsrung kimsrung force-pushed the 769-discount-maximum-cap branch 2 times, most recently from 7ee08ed to e9930b7 Compare December 11, 2023 05:44
@kimsrung kimsrung force-pushed the 769-discount-maximum-cap branch from e9930b7 to d9970e1 Compare December 11, 2023 05:45
Copy link
Collaborator

@theachoem theachoem left a comment

Choose a reason for hiding this comment

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

Look good to me bong, adding in calculator make more sense than my method if we want to cap each day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Discount maximum (CAP)
3 participants