Properly create/remove/prune shipping records everywhere #2847
Conversation
|
@kieckhafer Not sure that Stripe translations are in the scope of this PR. Maybe we can note on #3009? |
| // Because we are duplicating shipment quotes across shipping records | ||
| // we will get duplicate shipping quotes but we only want to diplay one | ||
| // So this function eliminates duplicates | ||
| function uniqObjects(methods) { |
There was a problem hiding this comment.
Could methods.reduce on line 15 instead of map, and check for uniqueness before pushing which would avoid the cost of looping through two extra times.
Not sure it's worth optimizing.
There was a problem hiding this comment.
I don't see how that would work. You can't compare them until you have stringified them.
| }); | ||
| } | ||
|
|
||
| // cartShippingQuotes |
There was a problem hiding this comment.
Should this be proper jsdoc using @private?
|
|
||
| // Verify that we have a valid address to work with | ||
| let shippingErrorDetails; | ||
| cart.shipping.map((shippingRecord) => { |
There was a problem hiding this comment.
why is this a map and not a forEach (which doesn't return an array) or a find (which stops after the first match? In this case find seems like it would perform the correct function.
| }); | ||
|
|
||
| if (shippingErrorDetails) { | ||
| return [[shippingErrorDetails], []]; |
There was a problem hiding this comment.
Comments would be helpful.
| }; | ||
| rates.push(errorDetails); | ||
| return [rates, retrialTargets]; | ||
| return [[errorDetails], []]; |
There was a problem hiding this comment.
My guess is we're returning an array of arrays here because we want to proceed to the next hook? Where does the error get logged or thrown? What does the empty array represent? I think some comments here would be good
There was a problem hiding this comment.
I feel like these comments are about the error handling techniques of the shipping method hooks which I am not creating or changing, just correcting the fact that it calls out the wrong shipment method. These comments are valid but probably better for a separate issue.
There was a problem hiding this comment.
You were the reviewer on the PR (#2738) that introduced this pattern a few weeks ago and this is the first I've seen of this pattern.
We can punt to a separate issue to fix this, but I'd like to understand what is happening here if you know.
There was a problem hiding this comment.
I will go through the code and try to add comments to try and make this more clear. I do feel like the general pattern here makes sense and works well. It's a little challenging trying to deal with a series of Hooks that can be extended by any plugin and deal with feedback from each hook on how to proceed (do I retry?, etc.) especially since how they need to communicate is with a record on the cart.
The timing of putting in this change while also dealing with multi-shop was extremely unfortunate however.
server/methods/core/shipping.js
Outdated
| }); | ||
| }); | ||
| const shopIds = Object.keys(cart.getItemsByShop()); | ||
| shopIds.map((shopId) => { |
| "shipping.$.address": address | ||
| } | ||
| }; | ||
| Cart.update(selector, update); |
There was a problem hiding this comment.
Any particular reason you're performing a Cart.update for each shop rather than updating the entire array of shipping records and then doing a single Cart.update?
server/methods/core/shipping.js
Outdated
| }); | ||
| }); | ||
| const shopIds = Object.keys(cart.getItemsByShop()); | ||
| shopIds.map((shopId) => { |
| const userId = cart.userId; | ||
| const account = Accounts.findOne(userId); | ||
| if (account && account.profile && account.profile.addressBook) { | ||
| const address = account.profile.addressBook.find((addressEntry) => addressEntry.isShippingDefault === true); |
There was a problem hiding this comment.
Are we guaranteed to have an address with isShippingDefault, that's true from my recollection in which case 👍
There was a problem hiding this comment.
AFAIK if we have an addressBook we always have a default shipping address
server/methods/core/shipping.js
Outdated
| const address = getDefaultAddress(cart); | ||
| if (address) { | ||
| const shopIds = Object.keys(cart.getItemsByShop()); | ||
| shopIds.map((shopId) => { |
|
Looks like this is fixed in #3035 but for visibility just putting it here as well... I cannot check out as a guest user, as I don't have an email address registered yet.
|
|
When I have two shops, and I log out and use the site as an anonymous user, I seem to be stuck on the newest created shop (Reaction 1 in this case), even though I can see products from both shops. I also cannot view any PDP's of either shop, I get an authorization error / sign in required.
Don't know if this is an issue for this PR, just wanted to document as I couldn't get to the PDP -> Checkout to test this PR because of that error. |
|
@kieckhafer I think that's something we should definitely open an issue for (the not getting to the PDP error) |
|
@spencern Ready for re-review |
|
@kieckhafer That may be an issue with us not changing back to the primary shop after a logout, I don't believe that this PR affects that. |
|
@spencern That issue is fixed (again) |
Closes #2836 and #2870 #2838 (and probably a bunch of other bugs)
To Test
That's just the basic test case for the bugs, but really this PR became "make shipping work in marketplace" so it needs to get tested with all the various scenarios e.g.: