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

Improve the extensibility of Rules::ItemTotal #3431

Merged
merged 3 commits into from
May 5, 2021

Conversation

elia
Copy link
Member

@elia elia commented Nov 19, 2019

Description

  • Make operators extendable without reopening the method
  • Allow to overwrite/extend the total
  • Allow to overwrite/extend the threshold
  • Simplify #eligible? making it more readable

This change stems from the need to add the :eq/=== operator and to modify the total, beyond that need it should make the code more readable and linear.

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)

@elia elia force-pushed the elia/item-total-extendability branch from da25ca6 to 7fa99c7 Compare November 19, 2019 09:56
@elia elia marked this pull request as ready for review November 19, 2019 15:17
@elia elia requested a review from kennyadsl December 27, 2019 18:00
@elia elia force-pushed the elia/item-total-extendability branch from 7fa99c7 to 616e1a7 Compare January 15, 2020 13:29
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.

@elia, thanks for the PR, I left a comment and have some concerns about the extendability approach: we are basically changing the class to let some of its logic easily extendable, suggesting users to override private methods in order to configure it. I think this is not a practice that we should promote since I'd prefer to be free to change/remove private methods in core classes without any concern. The basic solution could be moving those methods in the public scope, but I'm wondering if there's a better design for this.

@elia
Copy link
Member Author

elia commented Jan 20, 2020

I'd prefer to be free to change/remove private methods in core classes without any concern.

The original drive for this PR was to enable creating rules such as the following:

      class ItemTotalBeforeTax < ItemTotal
        # Rely on the item total with the adjustments already applied
        def total_for(order)
          order.item_total_before_tax
        end
      end

and

      module ItemTotalDecorator
        def operators_map
          super.merge(eq: :==)
        end

        Spree::Promotion::Rules::ItemTotal.prepend self
      end

where it's really easy to cherry-pick just the method you want to target.
Would it be better to have completely independent classes and let them have their own life instead of subclassing or decorating ItemTotal?

@elia elia force-pushed the elia/item-total-extendability branch from 616e1a7 to 4968e0a Compare February 4, 2020 13:58
@elia elia requested a review from kennyadsl February 4, 2020 13:58
@aldesantis aldesantis added this to the 2.11 milestone May 22, 2020
Copy link
Member

@aldesantis aldesantis left a comment

Choose a reason for hiding this comment

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

Thanks @elia!

@aldesantis
Copy link
Member

On the matter of private methods, I think we may want to make them public, but in this case having private methods may also be an acceptable exception. The API seems to make 100% sense to me.

@kennyadsl kennyadsl removed this from the 2.11 milestone Aug 28, 2020
@elia elia force-pushed the elia/item-total-extendability branch from 4968e0a to 01b237f Compare September 21, 2020 09:11
@elia elia force-pushed the elia/item-total-extendability branch from 01b237f to 5305751 Compare February 26, 2021 11:41
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.

@elia thank you! Looks good, I left a comment on a small improvement, let me know your opinion!

@spaghetticode
Copy link
Member

I discussed with Elia via chat about the improvement (replace the method ItemTotal::operators_map with a class attribute)
and we agreed the downside (unexpected behavior when changing in subclasses) is more relevant than the advantages.

@kennyadsl
Copy link
Member

Works for me, can we just add some documentation/example on how people should use this?

The accepted values are "gt" or "gte" but the default value was set to
the symbol for greater-than. It was never a problem because the HTML
form will use the correct values for its dropdown select and the first
value happens to be the intended default value.
@elia elia force-pushed the elia/item-total-extendability branch from 5305751 to 069f333 Compare April 30, 2021 10:21
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.

Looks good!


For what it's worth, the second commit contains the word "extendability", which should probably be "extensibility". The meaning is clear, so I don't care at all if you change it.

elia added 2 commits May 4, 2021 13:21
- Make operators extendable without reopening the method
- Allow to overwrite/extend the total
- Allow to overwrite/extend the threshold
- Simplify #eligible? making it more readable
- Migrate the OPERATORS constant to an extendible method
- Fix the default preferred operator
Also support a generic message that can work with any operator.
@elia elia force-pushed the elia/item-total-extendability branch from 069f333 to 3f1be03 Compare May 4, 2021 11:22
@elia
Copy link
Member Author

elia commented May 4, 2021

For what it's worth, the second commit contains the word "extendability", which should probably be "extensibility". The meaning is clear, so I don't care at all if you change it.

Fixed, thanks!

@kennyadsl kennyadsl changed the title Improve the extendability of Rules::ItemTotal Improve the extensibility of Rules::ItemTotal May 5, 2021
@kennyadsl kennyadsl added changelog:solidus_core Changes to the solidus_core gem and removed Needs Core Team Review labels May 5, 2021
@kennyadsl
Copy link
Member

Thanks @elia !

@kennyadsl kennyadsl merged commit a1bac19 into solidusio:master May 5, 2021
@kennyadsl kennyadsl deleted the elia/item-total-extendability branch May 5, 2021 09:20
kennyadsl added a commit to nebulab/solidus that referenced this pull request Mar 28, 2023
kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 12, 2023
kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 14, 2023
kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 18, 2023
kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 18, 2023
kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 19, 2023
kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 24, 2023
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.

6 participants