bleroy / Nwazet.Commerce

Idiomatic commerce module for Orchard CMS.
BSD 3-Clause "New" or "Revised" License
26 stars 21 forks source link

Localization-related enhancements #99

Closed MatteoPiovanelli-Laser closed 7 years ago

MatteoPiovanelli-Laser commented 7 years ago

I have been going through the module with an eye out to possible issues related internaltionalization/localization, because my colleagues and I usually work on things that require at least 2/3 localizations.

I found quite a bit of work I am planning to do:

Currency used / displayed It looks like the store uses the server's culture for things it probably shouldn't. On great example of this is the currency the prices are displayed in. The machine I am doing test on is, of course, in it-IT culture, so the currency symbols and such all default to €. However, I can set up Stripe so that payments are in USD (or any other currency for that matter). I feel this could be a massive functional problem. I am plannig to fix this by introducing a site setting for the store to set the currency, and then use that everywhere: in writing out prices, in setting up Stripe, anywhere else the currency is involved/relevant.

Multi-language store We will need to have localized versions of products, bundles and attributes. This opens up the field to several considerations not unlike those we did in Orchard for Taxonomies and ContentPicker: synchronization, limiting selection to same-language things only... On top of that, there is the issue of SKUs: the way I see this, a SKU should be a unique product identifier (even though there is currently no check for that, but I'll come to that later). However, if a ContentItem of type Product has localized versions, they should in general have the same SKU, because they represent the same product.

Search I refer here to a search feature included in the product list. Something to filter products by Culture, SKU, price range, isdigital... or any combination of the properties of a ProductPart. This will come useful when there are a lot of products.

SKU As I mentioned above, to me a SKU should be a unique product identifier. However, this unicity is currently not enforced, and has some caveats (e.g. the case of multiple locales). I would create a feature to:

Feel free to pitch in with further comments or ideas on those themes. I'll be prototyping the things above in the next few days.

bleroy commented 7 years ago

I'm a little confused: the culture in Orchard is a setting. Are there places where this setting is not respected? The culture's currency should be the default, I think. If you want to introduce an override, I suppose that's fine.

Not sure about search: it seems like this should be orthogonal to commerce. I know @sebastienros did some work on faceted search, on top of the built-in search features. Maybe he would have some suggestions. I have been trying hard to keep the commerce module focused on commerce-specific features, leveraging Orchard for everything else.

SKU work looks fine.

MatteoPiovanelli-Laser commented 7 years ago

Culture is indeed a setting, but it can be changed, and I think that should not affect prices. Moreover, if I take for example the ShoppingCart view, price is formatted as @unitPrice.ToString("c"), and does not care about my site's culture. Even if that did, changing my culture would create issues where a product's price, initially displayed in €, would then be displayed in $ (or any other currency), but would keep the same numerical value. What I am proposing is to introduce a setting that would only control the currency prices are being expressed in.

I am thinking that faceted search would require something related to what is in place for projections/queries. I should give more thought to this, but my feeling is that where we list contents through a custom controller (as is the case for products), there would still be a bunch of work to do to replicate the behavior, once that is in place for the "normal" contents' list.

bleroy commented 7 years ago

@unitPrice.ToString("c") would be a bug that would need to be fixed.

I'm ok with an additional setting that would be an optional override of the culture's setting. You'll probably have to introduce helpers to replace current formatting. That will be an issue for existing themes out there. Not sure how you'd solve that issue, except by saying that setting that new setting to a non-default value would be breaking for those view overrides.

Another aspect of search that we might want to give guidance over is search hints. Those are a must in commerce apps.

Finally, you may want to break this issue down, to make it more manageable, and enable future PRs to address the issues individually.

MatteoPiovanelli-Laser commented 7 years ago

I will open the smaller issues later and get to them. I am currently doing that "culture override" because it really bothers me.

petedavis commented 7 years ago

I dont think this is a culture issue, but another setting entirely that is specific to the e-commerce domain? If someone is an Italian living in Australia, and their language settings is all in Italian, they would still be interested in AUD currency. So they would be completely settings, not an override?

Example from wiggle: image

And gearbest.com image

bleroy commented 7 years ago

It depends how the current culture is set on the site, which is not mandated by Orchard itself, but by implementations of the culture provider interface. In some cases, the culture will be set site-wide, in others, it can be set according to user preferences, or who knows what. The bottom line is, there should probably be a similar currency provider interface, an implementation of which gets the currency from the culture, and another from settings. Yeah, that sounds like a much more sane design. And it opens up the possibility of creating your own implementation that gets the currency from some user selection, or who knows what else.

MatteoPiovanelli-Laser commented 7 years ago

Hi,

I am working on that currency provider, which I like.

There are three base providers I can think of:

A question is in the choice of provider to be used at any given time, because it does not make sense, I think, to be using more than one. My tentative answer is that the provider interface will have an Enabled property, and only one implementation will have that set to true (I'm starting to work on this configuration). An alternative would be a way to sort provider implementations based on their relative importance (even though I cannot think of a reason why someone would have several active providers anyway), but then we should be providing a way to customize the importance. Honestly, I'd rather worry about that further down the line, because having the option to have several different currencies opens up a whole lot of additional issues to consider.

MatteoPiovanelli-Laser commented 7 years ago

In case you want to comment on the implementation of this currency thing, I am doing it on this branch (currently not ready for a PR): https://github.com/LaserSrl/Nwazet.Commerce/tree/Enhancement/Internationalization

bleroy commented 7 years ago

That's a relatively easy question to answer because there's a standard Orchard way of dealing with this. when multiple implementations of a service make sense (e.g. token providers, shape table providers, theme selector, etc.), you inject an IEnumerable<ITheServiceType> and handle prioritization in any custom way that makes sense. If only one makes sense (culture selector, etc.), you can either just inject ITheServiceType, and you'll get null or the first one provided by Orchard, which is determined by dependency order and registration order. In this case, it seems like a single one should be active at any time, and it should be the site admin's responsibility to enable the one they want. There's always the option, if you need something more subtle, to implement your own provider that does different prioritization. So in summary, I'd go with a simple injection of a single implementation of the service.

MatteoPiovanelli-Laser commented 7 years ago

When you say "site admin's responsibility to enable the one they want" do you mean that each implementation should be in its own feature, and only one should be enabled at one time?

The way it's currently done in that branch I linked is through a setting, so that multiple providers can be enabled, and whoever manages/configures the store can select the one they want.

bleroy commented 7 years ago

Yes. It's simpler, and that's the way countless similar services are implemented.

MatteoPiovanelli-Laser commented 7 years ago

I don't think using the standard Orchard way of selecting a provider (dependency/registration order) is necessarily the best here. Maybe it's just that I don't have the details of that behaviour clear, so I fear that could mess up a site's configuration.

Suppose I build a webstore using Provider1. The feature for that provider has other settings/things/features that are related to it or depend on it. Then I have to change the webstore so it uses Provider2. I enable the corresponding feature. Not Provider2 should be active. But if I still have other active features that depended on the former, what is the provider that is actually active?

To try and be clearer on my doubt:

MatteoPiovanelli-Laser commented 7 years ago

As an aside, Orders should store the currency they were made in, and use that rather than whatever currency provider is there.

MatteoPiovanelli-Laser commented 7 years ago

On the branch I linked earlier, I am doing this as per the normal Orchard way, instead of using an "Active" flag on the provider.

bleroy commented 7 years ago

The administrator who activates provider 2 should disable provider 1. FeatureA should only contain provider 1. FeatureB should not depend on featureA. FeatureC should not depend on provider2. This is way too vague. Do you have a specific scenario where this would make sense?

I agree that orders should store the currency. That should probably be a separate work item.

MatteoPiovanelli-Laser commented 7 years ago

The way I am setting it up is that a default provider is directly in Nwazet.Commerce, so there is no need to enable a separate feature to begin with. That would be FeatureA in my example. FeatureB is then any other feature in the module right now. FeatureC is a future feature where I would go and create a different provider.

Are you saying that you think the Nwazet.Commerce feature should have no default provider implementation?

MatteoPiovanelli-Laser commented 7 years ago

I tested the following:

bleroy commented 7 years ago

a default provider is directly in Nwazet.Commerce, so there is no need to enable a separate feature to begin with

I don't understand what you're saying here: the point of putting that in a separate feature is that it can be enabled and disabled in isolation. That has nothing to do with having a default provider: you can enable the default feature through a module recipe. If one doesn't exist, warn the user. It's a little like Lucene in the Search module.

MatteoPiovanelli-Laser commented 7 years ago

The thing is, the changes in the branch make it so a provider is always used when writing out a price. SO I implemented a provider that does the same thing that's being done right now in the master branch, and uses the currency for the site's culture. Basically, when it writes a price it does T("{0:c}") as it is done in the view in the master branch. The providers giving different options then go in their own features. I implemented the one requiring to set up a site setting for the currency to be used in the store. When this feature is enabled, because of the dependecy order, the default provider is "replaced" by the one from the feature.

MatteoPiovanelli-Laser commented 7 years ago

I say we close this issue here since: