Remove hardcoded billing and shipping objects #2813

kieha · 2017-09-08T20:52:57Z

Resolves #2770

This PR aims to remove hard coded first billing and shipping objects from most code around orders.

How to test

Open browser console.
Go through order creating/completing/processing-as-admin process.
Make sure everything works as normal, and there are no errors in the browser console and/or server.

kieha · 2017-09-21T18:38:52Z

@spencern @aaronjudd this is ready for another look through.

aaronjudd · 2017-09-23T15:08:26Z

@spencern this is ready for you to review again

spencern · 2017-09-24T18:01:27Z

imports/plugins/core/orders/client/components/orderSummary.js

+          }
+          <br/>
+          <span>
+            {profileShippingAddress && profileShippingAddress.city},&nbsp;


This works. 👍

You could have setup profileShippingAddress to pass an empty object if undefined and then just used profileShippingAddress.property everywhere, but that's not necessarily better, just different. Not blocking.

Yeah, this makes sense. Avoids having to repeat profileShippingAddress && in a lot of places.

spencern · 2017-09-24T18:02:13Z

imports/plugins/core/orders/client/components/orderSummary.js

+          <br/>
+          <span>
+            {profileShippingAddress && profileShippingAddress.city},&nbsp;
+            {profileShippingAddress && profileShippingAddress.region},&nbsp;


I'm actually more curious about the need for   here. Does React not respect spaces here?

@spencern I added the   because the addition of profileShippingAddress &&s made the line too long, and I had to break it into different lines, therefore line 172 and 173 end up being two strings without space in between them.

But if I did this:

You could have set up profileShippingAddress to pass an empty object if undefined and then just used profileShippingAddress.property everywhere.

then I wouldn't have to change the line/add  s.

spencern

Some functions that should use prototype functions instead of lodash, otherwise looks pretty good. A few other "optional" style thoughts that don't block merging.

spencern · 2017-09-24T20:43:33Z

imports/plugins/core/orders/client/templates/workflow/shippingTracking.js

@@ -115,7 +123,7 @@ Template.coreOrderShippingTracking.helpers({
    const currentData = Template.currentData();
    const order = Template.instance().order;

-    const shippedItems = _.every(currentData.fulfillment.items, (shipmentItem) => {
+    const shippedItems = _.every(currentData.fulfillment && currentData.fulfillment.items, (shipmentItem) => {


This should use prototype every instead of lodash https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/every

spencern · 2017-09-24T20:44:40Z

imports/plugins/core/orders/client/templates/workflow/shippingTracking.js

@@ -132,7 +140,7 @@ Template.coreOrderShippingTracking.helpers({
    const currentData = Template.currentData();
    const order = Template.instance().order;

-    const canceledItems = _.every(currentData.fulfillment.items, (shipmentItem) => {
+    const canceledItems = _.every(currentData.fulfillment && currentData.fulfillment.items, (shipmentItem) => {


Use prototype every https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/every

spencern · 2017-09-24T20:45:11Z

imports/plugins/core/orders/client/templates/workflow/shippingTracking.js

@@ -149,7 +157,7 @@ Template.coreOrderShippingTracking.helpers({
    const currentData = Template.currentData();
    const order = Template.instance().order;

-    const completedItems = _.every(currentData.fulfillment.items, (shipmentItem) => {
+    const completedItems = _.every(currentData.fulfillment && currentData.fulfillment.items, (shipmentItem) => {


prototype instead of lodash

spencern · 2017-09-24T20:46:21Z

imports/plugins/core/orders/client/templates/workflow/shippingTracking.js


-    return _.includes(shipment.workflow.workflow, "coreOrderWorkflow/packed") && shipment.tracking ||
-    _.includes(shipment.workflow.workflow, "coreOrderWorkflow/packed");
+    return _.includes(shipmentWorkflow && shipmentWorkflow.workflow, "coreOrderWorkflow/packed") && shipment.tracking


use prototype includes https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes

spencern · 2017-09-24T20:47:08Z

imports/plugins/included/search-mongo/server/methods/searchcollections.js

-  orderSearch.shippingName = shopShipping.address.fullName;
-  orderSearch.shippingPhone = _.replace(shopShipping.address.phone, /\D/g, "");
+  orderSearch.shippingName = shopShipping.address && shopShipping.address.fullName;
+  orderSearch.shippingPhone = _.replace(shopShipping.address && shopShipping.address.phone, /\D/g, "");


Any reason we can't use prototype replace here? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace

spencern · 2017-09-24T20:48:47Z

server/methods/core/orders.js

@@ -290,10 +300,13 @@ export const methods = {
      ordersInventoryAdjust(order._id);
    }

+    const billingRecord = order.billing.find((billing) => { return billing.shopId === Reaction.getShopId(); });
+    const shippingRecord = order.shipping.find((shipping) => { return shipping.shopId === Reaction.getShopId(); });


on a single line, you can use the shorthand find if you prefer.

order.shipping.find((shipping) => shipping.shopId === Reaction.getShopId());

Just trying to avoid using the shorthand for multi-line functions.

…undefined

…/reactioncommerce/reaction into kieha-remove-zeroth-indexes-2770

…-liners

kieha · 2017-09-26T18:31:38Z

@spencern I've addressed the most recent changes that you had requested. I believe this is good to go now.

spencern · 2017-09-26T21:18:07Z

Only issue was when clicking

Client Error:

Error invoking Method 'notification/send': Match failed [400]

Server Error:

Exception while invoking method 'notification/send' Error: Match error: Expected string, got null
    at exports.check (packages/check.js:55:15)
    at [object Object].notificationSend (imports/plugins/included/notifications/server/methods/notifications.js:21:5)
    at packages/check.js:128:16
    at [object Object].EVp.withValue (packages/meteor.js:1134:15)
    at Object.exports.Match._failIfArgumentsAreNotAllChecked (packages/check.js:127:41)
    at maybeAuditArgumentChecks (packages/ddp-server/livedata_server.js:1765:18)
    at packages/ddp-server/livedata_server.js:719:19
    at [object Object].EVp.withValue (packages/meteor.js:1134:15)
    at packages/ddp-server/livedata_server.js:717:46
    at [object Object].EVp.withValue (packages/meteor.js:1134:15)
    at packages/ddp-server/livedata_server.js:715:46
    at [object Object]._.extend.protocol_handlers.method (packages/ddp-server/livedata_server.js:689:23)
    at packages/ddp-server/livedata_server.js:559:43
Sanitized and reported to the client as: Match failed [400]

spencern · 2017-09-26T22:15:14Z

Turns out that issue is present in the marketplace branch.
Approved 👍

brent-hoover and others added 30 commits September 1, 2017 07:51

When creating shipping records, create one for each shop

d7b40d0

Return an empty object if cart has no items

864e52d

Fix grammatical errors in test description

dc3f5ee

Update copyToCart to work with by shop shipping records

e0dc83e

Add billing/shipping shopIds to fix stripe test

e23ed7c

Break copyCartToOrder to it's own file

aa772ee

Add shopId to schema

fa45c4f

Set selected method on all shipping records

bf9e50d

Only show shipping methods associate with the primary shop

42d6341

Check rates against schema

a9433c4

Remove blackbox from Cart.shipping so it will actually validate

cd701b1

Validate shipping methods

5ba6521

Put blackbox back

37f6a7c

simple-schema not working, remove

baab68c

only show shipping methods from primaryShopId

8916529

Merge branch 'marketplace' into brent-fix-shipping-record-noshopid

9d0010a

Lint fixes

d3d1da9

Merge branch 'marketplace' into brent-fix-shipping-record-noshopid

ded21a6

Assign items to shipping records based on shopId

b24c32a

Assign shopIds when adding shipping address

278ee70

Only show the address when it has data

b245a4d

Lint fix

48b81fe

FIx tests

553d893

Merge branch 'marketplace' into brent-fix-shipping-record-noshopid

78fd30e

Remove hardcoded shipping object

f33da7d

Remove more instances of shipping[0] and billing[0]

4fd58e1

Merge branch 'marketplace' into kieha-remove-zeroth-indexes-2770

5fd8f8c

Fix react proptype error

a3c4c4c

Fix billingRecord error

4933b60

export getShipppingObject function outside of component

7e624ac

Njeri Kieha added 4 commits September 21, 2017 18:01

Merge branch 'marketplace' into kieha-remove-zeroth-indexes-2770

806e1e8

Merge branch 'marketplace' into kieha-remove-zeroth-indexes-2770

da8f1b3

Handle errors if getBillingInfo method returns empty object

6e59986

Resolve merge conflicts

fe06cf1

spencern and others added 2 commits September 22, 2017 12:58

Merge branch 'marketplace' into kieha-remove-zeroth-indexes-2770

dd1c62f

Merge branch 'marketplace' into kieha-remove-zeroth-indexes-2770

a4fcf23

aaronjudd approved these changes Sep 23, 2017

View reviewed changes

Merge branch 'marketplace' into kieha-remove-zeroth-indexes-2770

c8209df

spencern reviewed Sep 24, 2017

View reviewed changes

spencern suggested changes Sep 24, 2017

View reviewed changes

Merge branch 'marketplace' into kieha-remove-zeroth-indexes-2770

8b0b539

spencern mentioned this pull request Sep 25, 2017

Remove hardcoded zeroth indexes of the billing and shipping arrays in the order object #2770

Closed

Njeri Kieha added 5 commits September 26, 2017 20:39

Pass empty object to 'getBillingInfo' and 'getShippingInfo' calls if …

529a531

…undefined

Merge branch 'marketplace' into kieha-remove-zeroth-indexes-2770

d5590a6

Merge branch 'kieha-remove-zeroth-indexes-2770' of https://github.com…

6b7cf0a

…/reactioncommerce/reaction into kieha-remove-zeroth-indexes-2770

Remove lodash references

16a7be4

Remove more lodash references; use shorthand function patters for one…

608b086

…-liners

Njeri Kieha and others added 2 commits September 26, 2017 22:29

Fix failing tests

e6368d7

Fix case issue in orderTable

0fe9e6a

Merge branch 'marketplace' into kieha-remove-zeroth-indexes-2770

e645ede

spencern approved these changes Sep 26, 2017

View reviewed changes

spencern merged commit a6fc9f9 into marketplace Sep 26, 2017

spencern deleted the kieha-remove-zeroth-indexes-2770 branch September 26, 2017 22:24

spencern mentioned this pull request Oct 11, 2017

Release 1.5.0 #3089

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove hardcoded billing and shipping objects #2813

Remove hardcoded billing and shipping objects #2813

kieha commented Sep 8, 2017 •

edited

Loading

kieha commented Sep 21, 2017

aaronjudd commented Sep 23, 2017

spencern Sep 24, 2017

kieha Sep 24, 2017

spencern Sep 24, 2017

kieha Sep 24, 2017 •

edited

Loading

spencern left a comment

spencern Sep 24, 2017

spencern Sep 24, 2017

spencern Sep 24, 2017

spencern Sep 24, 2017

spencern Sep 24, 2017

spencern Sep 24, 2017

kieha commented Sep 26, 2017

spencern commented Sep 26, 2017

spencern commented Sep 26, 2017

Remove hardcoded billing and shipping objects #2813

Remove hardcoded billing and shipping objects #2813

Conversation

kieha commented Sep 8, 2017 • edited Loading

How to test

kieha commented Sep 21, 2017

aaronjudd commented Sep 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kieha Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

spencern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kieha commented Sep 26, 2017

spencern commented Sep 26, 2017

spencern commented Sep 26, 2017

kieha commented Sep 8, 2017 •

edited

Loading

kieha Sep 24, 2017 •

edited

Loading