Make code work with one shipment per order (even if the DB still supports Spree's multi shipments per order model) #6433
Conversation
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
| # If the selection is successful, it persists it in the database by saving the shipment. | ||
| # If it fails, it does not clear the current shipping_method selection. | ||
| # | ||
| # @return [ShippingMethod] the selected shipping method, or nil if the given shipping_method_id is |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [102/100]
| shipment.shipping_method | ||
| end | ||
|
|
||
| # Finds the shipment's shipping_rate for the given shipping_method_id and selects that shipping_rate. |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [105/100]
| end | ||
|
|
||
| private | ||
|
|
||
| def prioritize_packages(packages) | ||
| def prioritize_package(package) |
There was a problem hiding this comment.
Lint/UnusedMethodArgument: Unused method argument - package. If it's necessary, use _ or _package as an argument name to indicate that it won't be used. You can also write as prioritize_package(*) if you want the method to accept any arguments but don't care about them.
|
ping @Matt-Yorkley I wonder if you are looking at similar things as you look at adjustments. I am not sure if I have pinged you already, you will like this draft. |
|
Nice 👍 I think I found an upstream commit from 2-2-stable which also would also improve loading the correct selected shipping rate. |
…DB still supports multiple shipments per order
…upports multiple shipments per order in OFN, one order only has one shipment
luisramos0
force-pushed
the
one_shipment
branch
from
3191700 to
15a2409
Compare
|
I think we should have a go at this. I need to start touching shipments elsewhere, and it'll be simpler if we do this first. I'll take a look at it, there's still a few build errors. |
#empty? is used on enumerators, it's not plural now.
These methods need to explicitly return false if there's no shipment (as they did before).
|
It feels like some of these obscure classes like |
|
This stock code is actually the best code in spree imo. Yes, it is mostly
useless for ofn. It would require a bit more work to remove.
…On Sun, 7 Feb 2021, 12:23 Matt-Yorkley, ***@***.***> wrote:
It feels like some of these obscure classes like Stock::Prioritizer are
designed solely to handle things like packages being split across multiple
shipments...?
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#6433 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMQPOTMRTFTK37JQKRYWGDS52A3PANCNFSM4UICFVMA>
.
|
|
I'm going to move this back to the Tech Debt column unless we're still actively working on it, feel free to move it back if so. |
|
ok, I am not planning to pick this up now but it's still a valid PR that makes the core order code easier to work with. |
kirstenalarsen
added
the
bug-s3
sigmundpetersen
removed
the
bug-s3
What? Why?
This is a draft attempt at #6428
I thought this would not be obvious for other people, this is what I think we can do.
Even better would be to change has_many :shipments in order.rb to has_one :shipment but I dont know how to do that without changing the DB? (the spree_shipments table with an order_id and no shipment_id in the spree_orders table)
Even better would be to change the DB... :-)
What should we test?
Release notes
Changelog Category: Technical changes
Making OFN code simpler in the shipments domain.