Open bleroy opened 9 years ago
Original comment by Bertrand Le Roy (Bitbucket: bleroy, GitHub: bleroy):
You might have to implement some kind of null shipping provider, but I'm more than a little surprised by this: I know there are several users of the module who are selling digital products. If you did mark all your products as digital, this should not be happening.
The way it looks to me is that while it is true that if all products in the cart are digital the customer does not have to pick a shipping option, the view requires both Country and ZipCode to be input to allow checkout.
In the ShoppinCartController
, the USA are treated as a special case:
in the BuildCartShape
the following code is used to compute the shippingOptions:
if ((country == Country.UnitedStates && !string.IsNullOrWhiteSpace(zipCode)) ||
(!String.IsNullOrWhiteSpace(country) && country != Country.UnitedStates)) {
shape.Country = country;
shape.ZipCode = zipCode;
if (!shopItemsAllDigital) {
if (!isSummary && shippingOption == null) {
var shippingMethods = _shippingMethodProviders
.SelectMany(p => p.GetShippingMethods())
.ToList();
shape.ShippingOptions =
ShippingService
.GetShippingOptions(
shippingMethods, productQuantities, country, zipCode, _wca).ToList();
}
}
}
For the USA, we also need a zip code, while for any other country we don't.
This distinction is not in place in ShoppingCart.cshtml, where we always need both Country and ZipCode for any country: they need to either both be null (or empty strings) or both have values.
This doesn't seem to be causing issues if any product is not digital, as long as there are shipping options to chose from. If no shipping options are there, the customer gets the following:
This means that ShippingService.GetShippingOptions does not return null. Note that in case both
Model.Country
and Model.ZipCode
have values, the view is like so:
Because it first shows
ShippingOptions.cshtml
, and then if goes through the Model.ShippingOption == null
branch.
I can see that for digital goods we may still need to chose a "destination" country, possibly for fiscal reasons.
To try and summarize, I see the following issues:
Model.Country
and Model.ZipCode
stays there, so that if a customer removes physical products form the cart after having started the checkout process he may get stuck, no matter if there are shipping options available, because the controller and the view don't treat country and zipcode strings the same way.Should zip codes be mandatory for every country? I don't know, but I feel that handling the USA as a special case everytime is kinda ugly. I'd rather have an abstraction describing how to behave in each country, and apply that, or a default behaviour. Nevertheless the same behaviour should be in place in both controller and view, to avoid breaking things as it happens right now. On top of that, I think there should be a different handling of the case where no shipping options are available, or even make sense.
I agree. FWIW, the zip code is required for the US because of the way USPS works, iirc. It needs a zipcode to even give shipping options.
The easiest thing to begin with, then, would be to make a ZIP code required for every country, even though some countries don't have them. That would at least avoid some breaking behaviours when adding/removing things form the cart in some cases.
Then I'll think about the default/null shipping option: ideally, the customer wouldn't even have to chose it.
Sure, that may be fine, if we don't make it mandatory for all countries. Ideally, we'd have a way of knowing if any of the shipping methods will require a zip code for the chosen country, but I'm not sure that's even possible.
A few thoughts:
If the shipping options "told" us whether they require a zip code or a country to do their estimates, we would know whether we should ask for them. Something like adding
public bool CountryRequired;
public bool ZipCodeRequired;
To their interface (probably IShippingMethod). Then we only ask for those if any interface needs them.
To expand on my earlier comment:
in the BuildCartShape
method I would replace the following code:
if ((country == Country.UnitedStates && !string.IsNullOrWhiteSpace(zipCode)) ||
(!String.IsNullOrWhiteSpace(country) && country != Country.UnitedStates)) {
shape.Country = country;
shape.ZipCode = zipCode;
if (!shopItemsAllDigital) {
if (!isSummary && shippingOption == null) {
var shippingMethods = _shippingMethodProviders
.SelectMany(p => p.GetShippingMethods())
.ToList();
shape.ShippingOptions =
ShippingService
.GetShippingOptions(
shippingMethods, productQuantities, country, zipCode, _wca).ToList();
}
}
}
With something using those flags from the IShippingMethod interface.
//First get all shipping methods
var shippingMethods = _shippingMethodProviders
.SelectMany(p => p.GetShippingMethods())
.ToList();
shape.ShippingOptions = ShippingService.GetShippingOptions(
shippingMethods.Where(sm =>
sm.CountryRequired == !string.IsNullOrWhiteSpace(country) &&
sm.ZipCodeRequired == !string.IsNullOrWhiteSpace(zipCode)
), productQuantities, country, zipCode, _wca
).ToList();
Add to that checks regarding digital products and so on.
I will give something like that a try after lunch.
While we are in the topic of shipping, I would personally remove
// Shipping areas are stored as comma-delimited lists
string IncludedShippingAreas { get; set; }
string ExcludedShippingAreas { get; set; }
from the IShippingMethod interface
The way I would add country-specific requirements is by writing them in each shippping method's ComputePrice
method. This is kind-of what is done in the UspsService
:
When computing the prices, the service does not use the Zip code for international orders.
I would "translate" that in UspsShippingMethodPart.ZipCodeRequired
defaulting at false. I would also add a string Message
and a bool Valid
to ShippingOption
objects, so, in the USPS case, the options returned from ComputePrice for domestic deliveries would "write" that the zip is required, and not be selectable for the cart.
How does that sound?
The changes I made to the view in #95 seem to fix the issue I saw (my first comment in this thread), by letting all-digital carts skip the shipping selection. However I feel that we should keep the discussion open to see how to evolve the shipping.
Originally reported by: Jon Veder (Bitbucket: jonvee, GitHub: jonvee)
Is it possible/what's the best approach to remove shipping entirely? (from checkout and order confirmation etc) I don't want customers to have to enter anything other than their billing address which is already in their user profiles (custom user item). All the products are digital or services so nothing to ship.
Thanks!