-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Rewrite split shipments documentation and update overview of shipments #2345
Conversation
I intend to split the shipments guide into multiple articles. This would be the first of many infrastructural changes to the Solidus guides.
This commit is an edit of the original Solidus shipping guide. It reformats line lengths to 80 characters, hides to-do notes from being processed by using HTML-style comments, and organizes the article using additional subheadings.
The way split shipments works has changed significantly in Solidus v2.4.0. See solidusio#2199 for more information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is great!
I'm not the right person for the review of a long text in English :) but I left some comments mainly related to formatting and content.
I'm really loving to see first guides content landing on the Solidus repo! 📖 🎉
|
||
The customer must choose a shipping method for each shipment before proceeding to the next stage. At the confirmation step, the shipping cost will be shown and included in the order's total. | ||
|
||
**Note:*- *You can enable collection of extra _shipping instructions_ by setting the option `Spree::Config.shipping_instructions` to `true`. This is set to `false` by default. See [Shipping Instructions](#shipping-instructions) below.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From here on, we have some issues with markdown (I suspect it is the initial double *
which is not closed).
|
||
## Shipment setup examples | ||
|
||
### Simple setup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid exceeding the 80-char limit. Of course we can do that when we are sure the content is ready to be merged, just wanted to point that out so we don't forget. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely. Thanks for noting this.
Right now, all of the legacy content uses >80 character lines. I will be pruning these sections in further pull requests. (I'm just superficially breaking up these PRs since there is a lot of content.)
|
||
Your calculator should accept an array of `LineItem` objects and return a cost. It can look at any reachable data, but typically uses the address, the order and the information from variants which are contained in the line_items. | ||
|
||
### Product & Variant Configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this section is a bit out of context here? What about moving this text into What the Order's Administrator Sees
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad: I didn't make any changes here but this (and some other lines) appear in the diff. This is from the legacy shipments article, and it's being cleared out.
|
||
The option `Spree::Config[:shipping_instructions]` controls collection of additional shipping instructions. This is turned off (set to `false`) by default. If an order has any shipping instructions attached, they will be shown in an order's shipment admin page and can also be edited at that stage. Observe that instructions are currently attached to the _order_ and not to actual _shipments_. | ||
|
||
## The Active Shipping Extension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This explanation is very good, but in my opinion it's too much accurate for something which is not included in core. Also, it's very easy that it goes out of date here. I think this has to be moved in the Active Shipping Extension. The link provided in the first section of this guide could be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm creating a separate article for the solidus_active_shipping
gem, but I can see it belonging either as part of that repository or in an "Supported extensions" guide in the future.
#if weight in ounces > 13, then First Class Mail is not available for the order | ||
weight > 13 ? false : true | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use a code example with something that is not included into an extension (Usps stuff), so that it needs less overhead and we can keep it simpler.
- Shipping methods | ||
- Zones | ||
- Shipping categories | ||
- Shipping calculators (through shipping rates) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should also mention Inventory Units
(shipment content) here?
[solidus-active-shipping]: solidus-active-shipping-extension.html.markdown | ||
|
||
[custom-shipping-calculators]: custom-shipping-calculators.html.markdown | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure where but I'd also think about mentioning the concept of Carton
here. It is basically what the shipment becomes after it is shipped: a shipped shipment is a Carton
. Maybe it's just a technical detail but it is a question that pops up often and a section that explains it could be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I also have written another article that outline what a carton is, but I think it should be mentioned in this overview as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a whole bunch of comments for the adapted legacy docs. The new stuff is great!
guides/shipments/shipping.md
Outdated
|
||
An explanation of the different shipment states: | ||
### Additional attributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this distinction makes sense. cost
and order_id
are just as important as stock_location_id
.
guides/shipments/shipping.md
Outdated
- `additional_tax_total`: The sum of U.S.-style sales taxes on the shipment. | ||
- `promo_total`: The sum of the promotions on the shipment. | ||
- `included_tax_total`: The sum of the VAT-style taxes on the shipment. | ||
- `cost`: The order cost for the select shipping method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the estimated shipping costs, not the order cost.
guides/shipments/shipping.md
Outdated
- `promo_total`: The sum of the promotions on the shipment. | ||
- `included_tax_total`: The sum of the VAT-style taxes on the shipment. | ||
- `cost`: The order cost for the select shipping method. | ||
- `order_id`: The ID for the order that a shipment is being created for. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ID of the order the shipment belongs to.
guides/shipments/shipping.md
Outdated
not paid for. | ||
- `ready`: The shipment has no backordered inventory units and the order is paid | ||
for. | ||
- `shipped`: The shipment is on its way to the buyer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shipment has left the warehouse.
If it has arrived at the buyer, it will still be shipped, but not "on its way" anymore.
guides/shipments/shipping.md
Outdated
- Shipping methods | ||
- Zones | ||
- Shipping categories | ||
- Shipping calculators (through shipping rates) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah. Shipping calculators are more closely associated with shipping methods. The shipping rates are what the calculators compute.
guides/shipments/shipping.md
Outdated
@@ -284,7 +348,7 @@ class Calculator::Usps::FirstClassMailInternationalParcels < Calculator::Usps::B | |||
end | |||
``` | |||
|
|||
**Note:** *unlike calculators that you write yourself, these calculators do not have to implement a `compute` instance method that returns a shipping amount. The superclasses take care of that requirement.* | |||
**Note:*- *unlike calculators that you write yourself, these calculators do not have to implement a `compute` instance method that returns a shipping amount. The superclasses take care of that requirement.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Star has to be added back in.
guides/shipments/shipping.md
Outdated
@@ -380,7 +444,7 @@ class Calculator::Usps::FirstClassMailParcels < Calculator::Usps::Base | |||
def available?(order) | |||
multiplier = 1.3 | |||
weight = order.line_items.inject(0) do |weight, line_item| | |||
weight + (line_item.variant.weight ? (line_item.quantity * line_item.variant.weight * multiplier) : 0) | |||
weight + (line_item.variant.weight ? (line_item.quantity - line_item.variant.weight - multiplier) : 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is most certainly a wrong change.
guides/shipments/shipping.md
Outdated
@@ -479,8 +543,8 @@ The prioritizer is also a customization point. If you want to customize which pa | |||
|
|||
The `Adjuster` visits each package in an order and ensures the correct number of items are in each package. To customize this functionality, you need to do two things: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The adjuster does not exist anymore.
|
||
After the proposed shipments have been determined, the customer can continue the | ||
checkout process and take the order from the `delivery` state to the `payment` | ||
state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a sentence here about how Solidus < 2.4 have a different legacy stock coordinator and associated classes that are more complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added a note about this in 753c5ce. Thanks.
|
||
```ruby | ||
Rails.application.config.spree.stock_splitters = [] | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this is cool and very useful.
Some of my comments appear as outdated, but they are not. |
My apologies. In my attempt to format the original document consistently, I made this PR much longer than it actually should be - and I introduced some formatting errors as a result, too. |
753c5ce
to
d96e2c4
Compare
Because my other PRs depend on the changes here in My apologies for botching this round. I'm sure there's a better way to do/organize this. |
Okay, I'm going to close this and re-open a new pull request. That one will be a comprehensive everything-about-shipments PR that is well organized—an article for each commit. Thanks for all the feedback so far. It will not be lost. 👌 |
I am taking the shipping guide documentation merged in #1966 and I am breaking it into several articles about shipments. My goals are to:
Note that I'm splitting this into several smaller pull requests since there are a lot of changes. There is still legacy content in this PR that I will be pruning in further PRs. This PR begins the rewrite of the main overview of shipments and rewrites the content about split shipments (due to #2199).
This is part a larger project to improve Solidus's documentation. See this gist with the high-level table of contents. Where and how this documentation will exist is still up for discussion.