update product scheme to set country autovalue #2840
Conversation
kieckhafer
requested review from
brent-hoover
and removed request for
aaronjudd
lib/collections/schemas/helpers.js
Outdated
|
|
||
| /** | ||
| * shopDefaulCountry | ||
| * @summary used for schema injection autoValue |
There was a problem hiding this comment.
I hadn't seen @summary or @example used before - would you mind adding those to the ongoing discussion in docs? reactioncommerce/reaction-docs#275
There was a problem hiding this comment.
shopDefaultCountry - minor typo!
lib/collections/schemas/helpers.js
Outdated
| * @return {String} returns country value from default shop | ||
| */ | ||
| export function shopDefaultCountry() { | ||
| if (this.isSet && Meteor.isServer) { |
There was a problem hiding this comment.
I can read through this, but I think it's probably worth adding a little more inline commentary as to what's going on for people who are less versed in how autoValue works.
lib/collections/schemas/helpers.js
Outdated
| const shop = Shops.findOne({ | ||
| _id: Reaction.getShopId() | ||
| }); | ||
| if (shop && shop.addressBook && shop.addressBook[0]) { |
There was a problem hiding this comment.
I don't love that we're assuming that the 0th address is the correct address here, but I know that's a pattern that's older than this PR.
Is there an indicator that a shop is using a specific address as their "shop address" similar to how customers can have a default address? Is it the default flag? Or any other way to identify the Shop's main address?
There was a problem hiding this comment.
I don't believe there is any way to determine the default address, and calling on the [0] is the way to do it.
There was a problem hiding this comment.
Actually, there's a field on the schema that determines this.
isBillingDefault: {
label: "Make this your default billing address?",
type: Boolean
},
isShippingDefault: {
label: "Make this your default shipping address?",
type: Boolean
},
There was a problem hiding this comment.
Do we not use that for Shops?
There was a problem hiding this comment.
I see that it's currently set to True on one created shop. Even if we don't set it for now this feels like a more "future-proof" way of doing it.
There was a problem hiding this comment.
^^ yeah... dunno why I didn't connect that, will update to use... one of the two. Which should be used for this? Shipping?
This is for the product origin.
There was a problem hiding this comment.
I think Shipping makes the most sense. That where you would have things shipped to is there you want to ship from. At least in most cases and as accurate as being the right country.
There was a problem hiding this comment.
So we do set it explicitly in the dashboard so I think that's the way to go. Maybe we could add a note there that says setting that will set the default origin for Products? It's probably an autoform so not sure how hard that would be.
lib/collections/schemas/helpers.js
Outdated
| }); | ||
|
|
||
| // Find the current shops primary address | ||
| if (shop && shop.addressBook && shop.addressBook[0]) { |
There was a problem hiding this comment.
Not sure how much work this would really be, but why we don't extend this a little more, with the goal of removing [0] dependencies, and remove the need refactor this again (so soon)? One approach that seems easy, could be to just set and check isShippingDefault to know the origination country?
|
@aaronjudd @zenweasel my latest update should take care of getting rid of using |
|
Tests are failing. |
|
@kieckhafer can you fix conflicts and tests here? |
|
fixed and fixed |
Update our product schema to set default origin country for Product and Variant.
Default country is country the shop address is based in.