Refactor line_item.rb #522
Conversation
|
|
||
| private | ||
| # Sets the quantity to zero if it is nil or less than zero. | ||
| def invalid_quantity_check |
There was a problem hiding this comment.
If we're already renaming and moving stuff (what I like), we should rename this method into normalize_quantity, because that's what it does.
PS: And please add a new line after the private. My dear eyes are bleeding.
|
The initial idea for this PR is not working out - there are many ways of setting a line item's price, and directly hooking into all of them them for deducing the merchant's home VAT (like I did it in Spree) is not feasible or beautiful. As a consequence, this is a pretty pure refactoring of the line item, adding mostly readability for the time being. Next up: Think deeply about a good way of modifying line item prices. This PR is now ready to merge. Btw: I did have to force push twice to account for random failures in the back end suite. |
|
I'm not sure how (or if?) you're going to utilize any of these changes in the VAT code but I'm still 👍 on the commit. |
This just moves lines up and down so it's easier to see where associations, AR callbacks, configuration and actual methods are. I've also moved to methods to private which are not called from outside the line item instance. I think they were mostly public because their tests required that, so I reorganized the tests for them a little. I've also de-indented the private methods. Hound will be happy! I'd like to have this in as preparation for the VAT work.
Before, there was two private methods for copying various attributes from the variant into the line item. This commit moves them into one.
|
Rebased. @cbrunsdon it helps, because having easier-to-read code when refactoring things helps. :) |
|
I love this change, but am a little concerned about changing Talked about this IRL with @cbrunsdon who suggested we could check this and warn users if the |
Many projects have overridden the #copy_price hook. In order for them to easily upgrade, re-extract setting the price-related attributes to a private method #set_pricing_attributes and notify about the name change if #copy_price is set.
|
OK, totally understand the concern about I don't think tackling #390 in this PR makes much sense, as this PR doesn't address changes in functionality. The idea is really just to fix naming and housekeeping. |
|
👍 (and clarke is here and says 👍) |
| it 'should run the user-defined copy_price method' do | ||
| expect_any_instance_of(Spree::LineItem).to receive(:copy_price).and_call_original | ||
| ActiveSupport::Deprecation.silence do | ||
| Spree::LineItem.new(variant: variant, order: order) |
There was a problem hiding this comment.
Spree::LineItem.new(variant: variant, order: order) it self doesn't call the #copy_price method, but build the order object does.
Instead, one should do:
let(:order) { create :order }
# ...
Spree::LineItem.new(variant: variant, order: order).save(with a clean order created, without calling copy_price)
See #1591
This PR was supposed to be preparation for the VAT stuff, but turned out to be more of a housekeeping exercise: Some naming improvements and a little bit of code style, plus different tests for creating line items.
In order to improve readability of the file, I sorted the code by what it does (associations, callbacks, methods, private methods).
Nevertheless, I do think this is an improvement, so I'm keeping the PR open.