Skip to content
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

Properly create/remove/prune shipping records everywhere #2847

Merged
merged 67 commits into from
Oct 6, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
67 commits
Select commit Hold shift + click to select a range
4aa4a5e
Don't filter for primary shop methods on the client
brent-hoover Sep 13, 2017
de5933d
call updateShipping *after* cart iitem has been removed
brent-hoover Sep 13, 2017
b9e619e
Prune shipping records when we update the cart
brent-hoover Sep 13, 2017
da648a4
Remove trailing spaces for ESLint
brent-hoover Sep 13, 2017
b94937d
Keep shipping objects consistent with items
brent-hoover Sep 13, 2017
5305b9d
Only show unique shipping methods
brent-hoover Sep 13, 2017
22b5713
Temporarily work around Stripe refund list so it doesn't block the cart
brent-hoover Sep 13, 2017
6f6d9ae
Create a shipping record with default address when we create cart
brent-hoover Sep 13, 2017
543b6f7
Remove commented code, add JSDoc
brent-hoover Sep 13, 2017
349a17f
Merge branch 'marketplace' into brent-fix-issue-2836
brent-hoover Sep 13, 2017
3a7212b
Merge branch 'marketplace' into brent-fix-issue-2836
brent-hoover Sep 18, 2017
a357d9e
Temporarily skip Stripe test
brent-hoover Sep 18, 2017
569c069
Only add shipping records per shop when we have items in the cart
brent-hoover Sep 18, 2017
2161e59
Handle changing addresses when cart is empty
brent-hoover Sep 18, 2017
dc8ecae
Don't update shipping records twice
brent-hoover Sep 18, 2017
2e647da
fixed the duplication of some data in the db
foladipo Sep 18, 2017
72c6d56
Merge branch 'marketplace' into brent-fix-issue-2836
brent-hoover Sep 19, 2017
67befc5
Merge branch 'marketplace' into brent-fix-issue-2836
brent-hoover Sep 20, 2017
35892a0
Don't test for parcel for flat-rates. Correct package-name
brent-hoover Sep 20, 2017
1f0c6c3
Add default address when it's been removed
brent-hoover Sep 20, 2017
6096784
Add address before calling getRates
brent-hoover Sep 20, 2017
3e3eb0e
regrab cart before passing to getRates
brent-hoover Sep 20, 2017
266b1d5
Remove references to shipping[0]
brent-hoover Sep 20, 2017
61efb98
Lint fixes and cleanup
brent-hoover Sep 20, 2017
3f9b058
remove check for flat rate carriers
foladipo Sep 20, 2017
d3418df
Merge branch 'marketplace' into brent-fix-issue-2836
brent-hoover Sep 21, 2017
8afd110
in custom shops, rely on the primary shop's shippo setup
foladipo Sep 24, 2017
ca58e66
Merge branch 'marketplace' into brent-fix-issue-2836
Sep 25, 2017
1b4941a
Merge branch 'marketplace' into brent-fix-issue-2836
brent-hoover Sep 29, 2017
0427418
Back out changes to shippo
brent-hoover Sep 29, 2017
3367dad
Revert "remove check for flat rate carriers"
brent-hoover Sep 29, 2017
95a6d0b
Don't do a special check for flat_rates
brent-hoover Sep 29, 2017
5b0ec19
Revert "in custom shops, rely on the primary shop's shippo setup"
brent-hoover Oct 2, 2017
03ab67a
Remove unused file
brent-hoover Oct 2, 2017
3923b8e
Don't add an error just because the package is not enabled
brent-hoover Oct 2, 2017
a5615e8
Always make updates to all shipping records
brent-hoover Oct 2, 2017
c507b71
Always select with shopId
brent-hoover Oct 2, 2017
5ce25b8
Merge branch 'marketplace' into brent-fix-issue-2836
brent-hoover Oct 2, 2017
eb148d2
Eliminate unnecessary branch
brent-hoover Oct 2, 2017
bd75724
Merge branch 'marketplace' into brent-fix-issue-2836
brent-hoover Oct 4, 2017
3d60e1d
Fix broken createShipmentQuotes that wasn't creating shipping records
brent-hoover Oct 4, 2017
a904b79
Don't return an error because the package is disabled
brent-hoover Oct 4, 2017
81fec90
Grab config from primaryShop if merchantShippingRates for set to true
brent-hoover Oct 4, 2017
fa5d5c3
Remove spurious parcel object and put weight/dimensions on options
brent-hoover Oct 4, 2017
2994411
Grab settings based on marketplace settings
brent-hoover Oct 4, 2017
c659aeb
Use total cart weight for "parcel"
brent-hoover Oct 4, 2017
3f8edbf
destructuring is magic
brent-hoover Oct 4, 2017
feb9603
Reenable test
brent-hoover Oct 4, 2017
1643664
Rename plugin to be consistent with pattern
brent-hoover Oct 4, 2017
4314a50
Correct typo
brent-hoover Oct 4, 2017
c798c33
Use error rather than e
brent-hoover Oct 4, 2017
4341665
JSdoc for uniqObjects
brent-hoover Oct 5, 2017
c9d9756
Change map to find. Add better comments
brent-hoover Oct 5, 2017
1c46531
Add better TODOs
brent-hoover Oct 5, 2017
3ac502d
Throw not-implemented error if multi-providers enabled
brent-hoover Oct 5, 2017
db1307b
Throw not-implemented error if multi-providers enabled
brent-hoover Oct 5, 2017
75c6bb3
Add comment
brent-hoover Oct 5, 2017
6bd6562
Add comment about flag
brent-hoover Oct 5, 2017
55330ba
Update JSDoc
brent-hoover Oct 5, 2017
e6fdd9c
Add comments and use forEach instead of map
brent-hoover Oct 5, 2017
6d36a5e
lots of forEach instead of map
brent-hoover Oct 5, 2017
b4e694b
Change to forEach
brent-hoover Oct 5, 2017
8b5bd21
Don't change billing code to avoid confusion
brent-hoover Oct 5, 2017
bac0a5e
Update shipping records all at once
brent-hoover Oct 5, 2017
304f605
Correctly update shipping record
brent-hoover Oct 5, 2017
a71fa92
Back out change to normalizeAddress to fix
brent-hoover Oct 6, 2017
94cd6a6
Merge branch 'marketplace' into brent-fix-issue-2836
brent-hoover Oct 6, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 26 additions & 12 deletions imports/plugins/core/shipping/client/templates/checkout/shipping.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,33 @@
import _ from "lodash";
import { Meteor } from "meteor/meteor";
import { EJSON } from "meteor/ejson";
import { Template } from "meteor/templating";
import { ReactiveDict } from "meteor/reactive-dict";
import { Reaction } from "/client/api";
import { Cart } from "/lib/collections";


// 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
/**
* Return a unique list of objects
* @param {Array} objs - An array of objects
* @returns {Array} An array of object only containing unique members
* @private
*/
function uniqObjects(objs) {
const jsonBlobs = objs.map((obj) => {
return JSON.stringify(obj);
});
const uniqueBlobs = _.uniq(jsonBlobs);
return uniqueBlobs.map((blob) => {
return EJSON.parse(blob);
});
}

// cartShippingQuotes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be proper jsdoc using @private?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

// returns multiple methods
/**
* cartShippingQuotes - returns a list of all the shipping costs/quotations
* of each available shipping carrier like UPS, Fedex etc.
Expand All @@ -16,24 +39,18 @@ import { Cart } from "/lib/collections";
function cartShippingQuotes(currentCart) {
const cart = currentCart || Cart.findOne();
const shipmentQuotes = [];
const primaryShopId = Reaction.getPrimaryShopId();
if (cart) {
if (cart.shipping) {
for (const shipping of cart.shipping) {
if (shipping.shipmentQuotes) {
for (const quote of shipping.shipmentQuotes) {
if (shipping.shopId === primaryShopId) {
if (quote.carrier === "Flat Rate" || quote.requestStatus !== "error") {
shipmentQuotes.push(quote);
}
}
shipmentQuotes.push(quote);
}
}
}
}
}

return shipmentQuotes;
return uniqObjects(shipmentQuotes);
}

function shippingMethodsQueryStatus(currentCart) {
Expand Down Expand Up @@ -66,13 +83,10 @@ function shippingMethodsQueryStatus(currentCart) {
function cartShipmentMethods() {
const cart = Cart.findOne();
const shipmentMethods = [];
const primaryShopId = Reaction.getPrimaryShopId();
if (cart) {
if (cart.shipping) {
for (const shipping of cart.shipping) {
if (shipping.shipmentMethod && shipping.shopId === primaryShopId) {
shipmentMethods.push(shipping.shipmentMethod);
}
shipmentMethods.push(shipping.shipmentMethod);
}
}
}
Expand Down
60 changes: 38 additions & 22 deletions imports/plugins/included/shipping-rates/server/hooks/hooks.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { check } from "meteor/check";
import { Meteor } from "meteor/meteor";
import { Shipping, Packages } from "/lib/collections";
import { Logger, Reaction, Hooks } from "/server/api";
import { Cart as CartSchema } from "/lib/collections/schemas";
Expand Down Expand Up @@ -34,20 +35,35 @@ function getShippingRates(previousQueryResults, cart) {
return previousQueryResults;
}
}
if (!(cart.shipping && cart.shipping[0] && cart.shipping[0].address)) {

// Verify that we have shipping records
if (!cart.shipping || !cart.shipping.length) {
const errorDetails = {
requestStatus: "error",
shippingProvider: "shippo",
message: "The 'shipping' property of this cart is either missing or incomplete."
shippingProvider: "flat-rate-shipping",
message: "this cart is missing shipping records"
};
rates.push(errorDetails);
return [rates, retrialTargets];
return [[errorDetails], []];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@brent-hoover brent-hoover Oct 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
if (!(cart.items && cart.items[0] && cart.items[0].parcel)) {

// Verify that we have a valid address to work with
let shippingErrorDetails;
if (cart.shipping.find((shippingRecord) => !shippingRecord.address)) {
shippingErrorDetails = {
requestStatus: "error",
shippingProvider: "flat-rate-shipping",
message: "The address property on one or more shipping records are incomplete"
};
return [[shippingErrorDetails], []];
}

// Validate that we have valid items to work with. We should never get here since we filter for this
// at the cart level
if (!cart.items || !cart.items.length) {
const errorDetails = {
requestStatus: "error",
shippingProvider: "shippo",
message: "This cart has no items, or the first item has no 'parcel' property."
shippingProvider: "flat-rate-shipping",
message: "this cart has no items"
};
return [[errorDetails], []];
}
Expand All @@ -58,21 +74,20 @@ function getShippingRates(previousQueryResults, cart) {
merchantShippingRates = marketplaceSettings.public.merchantShippingRates;
}

// TODO: Check to see if the merchantShippingRates flag is set in
// marketplace and get rates from the correct shop.
const pkgData = Packages.findOne({
name: "reaction-shipping-rates",
shopId: Reaction.getPrimaryShopId()
});
let pkgData;
if (merchantShippingRates) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can tell what's going on here, but might be nice to spell it out in comments
If merchantShippingRates is enabled, then get shipping rates from the appropriate shop, otherwise get rates from the primary shop.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments

// TODO this needs to be rewritten to handle getting rates from each shops that's represented on the order
Logger.fatal("Multiple shipping providers is currently not supported");
throw new Meteor.Error("not-implemented", "Multiple shipping providers is currently not supported");
} else {
pkgData = Packages.findOne({
name: "reaction-shipping-rates",
shopId: Reaction.getPrimaryShopId()
});
}


if (!pkgData || !cart.items || pkgData.settings.flatRates.enabled !== true) {
const errorDetails = {
requestStatus: "error",
shippingProvider: "flat-rate-shipping",
message: "Error. Flat rate shipping might be uninstalled or disabled, or your cart is empty."
};
// There's no need for a retry in this case.
rates.push(errorDetails);
return [rates, retrialTargets];
}

Expand All @@ -82,6 +97,8 @@ function getShippingRates(previousQueryResults, cart) {
"provider.enabled": true
};

// Get rates from shops if merchantShippingRates is enabled
// Otherwise just get them from the primaryShop
if (merchantShippingRates) {
// create an array of shops, allowing
// the cart to have products from multiple shops
Expand All @@ -90,7 +107,6 @@ function getShippingRates(previousQueryResults, cart) {
shops.push(product.shopId);
}
}

// if we have multiple shops in cart
if ((shops !== null ? shops.length : void 0) > 0) {
selector = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,30 @@ import { Logger, Reaction, Hooks } from "/server/api";

// callback ran on getShippingRates hook
function getShippingRates(previousQueryResults, cart) {
const marketplaceSettings = Reaction.getMarketplaceSettings();
let merchantShippingRates = false;
if (marketplaceSettings && marketplaceSettings.public && marketplaceSettings.public.merchantShippingRates) {
merchantShippingRates = marketplaceSettings.public.merchantShippingRates;
}

const [rates, retrialTargets] = previousQueryResults;
const shops = [];
const products = cart.items;

const pkgData = Packages.findOne({
name: "reaction-shippo",
shopId: Reaction.getShopId()
});
let pkgData;
if (merchantShippingRates) {
Logger.fatal("Multiple shipping providers is currently not implemented");
throw new Meteor.Error("not-implemented", "Multiple shipping providers is currently not implemented");
} else {
pkgData = Packages.findOne({
name: "reaction-shippo",
shopId: Reaction.getPrimaryShopId()
});
}


// must have cart items and package enabled to calculate shipping
if (!pkgData || !cart.items || pkgData.settings.shippo.enabled !== true) {
const errorDetails = {
requestStatus: "error",
shippingProvider: "shippo",
message: "Error. The Shippo package may be uninstalled or disabled, or your cart is empty."
};
// There's no need for a retry in this case.
rates.push(errorDetails);
return [rates, retrialTargets];
}

Expand All @@ -31,22 +37,24 @@ function getShippingRates(previousQueryResults, cart) {
"provider.enabled": true
};

// create an array of shops, allowing
// the cart to have products from multiple shops
for (const product of products) {
if (product.shopId) {
shops.push(product.shopId);
// if we don't have merchant shipping rates enabled, only grab rates from primary shop
if (!merchantShippingRates) {
shops.push(Reaction.getPrimaryShopId());
} else {
// create an array of shops, allowing
// the cart to have products from multiple shops
for (const product of products) {
if (product.shopId) {
shops.push(product.shopId);
}
}
}
// if we have multiple shops in cart
if ((shops !== null ? shops.length : void 0) > 0) {
selector = {
"shopId": {
$in: shops
},
"provider.enabled": true
};
}
selector = {
"shopId": {
$in: shops
},
"provider.enabled": true
};

const shippingCollection = Shipping.find(selector);
const shippoDocs = {};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,27 @@ function createShippoAddress(reactionAddress, email, purpose) {

// Creates a parcel object suitable for Shippo Api Calls given
// a reaction product's parcel and units of measure for mass and distance
function createShippoParcel(reactionParcel, reactionMassUnit, reactionDistanceUnit) {
function createShippoParcel(reactionParcel, cartWeight, reactionMassUnit, reactionDistanceUnit) {
const shippoParcel = {
width: reactionParcel.width || "",
length: reactionParcel.length || "",
height: reactionParcel.height || "",
weight: reactionParcel.weight || "",
weight: cartWeight,
distance_unit: reactionDistanceUnit,
mass_unit: reactionMassUnit
};

return shippoParcel;
}

function getTotalCartweight(cart) {
const totalWeight = cart.items.reduce((sum, cartItem) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be getting total weight by shop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Right now we only do one call to Shippo. There's a lot more to be done to make Shippo actually work for multi-shop. (as in different quotes for different shops)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Didn't realize this was within the Shippo plugin here.

const itemWeight = cartItem.quantity * cartItem.parcel.weight;
return sum + itemWeight;
}, 0);
return totalWeight;
}

// converts the Rates List fetched from the Shippo Api to Reaction Shipping Rates form
function ratesParser(shippoRates, shippoDocs) {
return shippoRates.map(rate => {
Expand Down Expand Up @@ -411,7 +419,8 @@ export const methods = {
if (cart.items && cart.items[0] && cart.items[0].parcel) {
const unitOfMeasure = shop && shop.baseUOM || "kg";
const unitOfLength = shop && shop.baseUOL || "cm";
shippoParcel = createShippoParcel(cart.items[0].parcel, unitOfMeasure, unitOfLength);
const cartWeight = getTotalCartweight(cart);
shippoParcel = createShippoParcel(cart.items[0].parcel, cartWeight, unitOfMeasure, unitOfLength);
} else {
errorDetails.message = "This cart has no items, or the first item has no 'parcel' property.";
return [[errorDetails], []];
Expand Down
77 changes: 0 additions & 77 deletions imports/plugins/included/shippo/server/hooks/hooks.js

This file was deleted.

Loading