-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[Marketplace] Set order(s) to a specific status #2613
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
Changes from 1 commit
ca8c102
c3467f5
7ac944f
48b0893
dd0a77b
b64ee84
72d1ad3
18778df
25e8942
cea9557
1d0fd64
1a6a8f3
d3e7da5
b59263b
2c58cba
c9a0b28
670ee6b
e87ea72
8166dc1
bcb1fa3
ee7d9a7
e083bb0
c737ac9
c27f70e
698ed21
0b97a00
3144dc3
b16eb03
9c07755
df80998
ab14f03
1c36848
8545717
3f252fe
6bbbef6
990574d
db4e8d2
543ddc0
6c60ea1
1e11e68
dc76234
f1016d9
b2460bd
e47dfd9
f04c46f
a55229a
075defe
5a91d9f
b089d03
430815a
f7e1c4c
34162e0
6e05c46
809bbb9
921e423
e0cf90b
751d7df
ea9a4f1
6d3dc05
0481dfc
dd98eee
9ba62c8
bef35d9
a6c941b
907f54f
7dc1840
50c75f1
588c794
af54492
e8da685
d0babc2
f34865b
89c24aa
c8a1eca
8722120
6f3f887
82c130b
ab9f6b2
c023440
ff54605
3137366
9fc7708
d1e700c
b58a840
6595954
cc482d8
ac491f8
27c5fef
ca2a285
c37eef0
6c83d1a
5d5569f
207d4ab
bbfcaa5
14b86fc
5e2c06b
ae5e284
69d9667
be43f08
8054401
f32e595
c8dc32f
32e8a06
0d56e30
4326811
1c06ab2
c430114
7e8d495
55fe0f9
72f708e
3792290
3dcfdac
dcc74f3
f4a9e25
86e18ad
25d07b5
be42bfa
d36580d
ec28de5
05cdec9
a306623
05916fc
62f9109
50c3637
356cb95
dbf2f7d
6ba33ae
f97db7e
9283c39
e18e14d
42107a3
e252336
0198d98
9accb83
425233f
5bbf553
5a5104f
e00a960
21b3cdb
672c007
f18df54
59066e8
43d5d74
01cdc15
84e3ab0
f303673
3b9fcf2
3c81615
6331480
3b18108
3db5597
ffebdd0
adefa91
b4af025
ab31de3
0edf42b
5c3deba
37d07b4
0c77e32
c1e0633
1e4a12f
0a5dcfc
182cdc5
b8a2e92
f8cbd95
09f04be
6777666
da73d48
b6c224f
1af4b6b
816bd3d
0600ce5
7432962
cfe7b00
999231a
4d4d336
98d5dc7
aacebe1
9a8c92f
8ce785e
a8b1aa3
24d4ae5
03e7317
6633579
ed15acd
df28375
711a526
3c05af4
6c8f08b
e15f625
cb9bfcb
d736644
a5a8901
fb566c4
5513bba
5561e57
b456aec
c743187
e270c06
1f1907e
8246459
6a148fa
6a1d4d9
b05fd6e
f2c4802
263eb22
97cc49a
2c80e65
333ed7a
426d7b3
2ce2523
cd81d1d
b649bce
e08f839
cf524d8
c1bba88
95d962c
e729461
29ca951
f7caaed
dbfd24b
444405c
f1f6260
d46245d
7e908b5
bf46c0c
a21497b
bc9dc81
da20f96
d981b9c
2cdc1ad
7239bde
a7fdb4e
39c9348
dff2ee5
dc73ba1
487b97f
79f9034
993ca17
e4ce99d
09917fe
ea2131b
ec80a2d
6dc056f
2e63b52
c76d9d3
63aa52a
ab35062
678321d
1748247
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,7 +77,7 @@ Template.coreOrderShippingTracking.events({ | |
| "click [data-event-action=shipmentPacked]": () => { | ||
| const template = Template.instance(); | ||
|
|
||
| Meteor.call("orders/shipmentPacked", template.order, template.order.shipping[0], true); | ||
| Meteor.call("orders/shipmentPacked", template.order, template.order.shipping[0]); | ||
| }, | ||
|
|
||
| "submit form[name=addTrackingForm]": (event, template) => { | ||
|
|
@@ -193,6 +193,7 @@ Template.coreOrderShippingTracking.helpers({ | |
| const order = Template.instance().order; | ||
| const shipment = order.shipping[0]; | ||
|
|
||
| return (shipment.packed && shipment.tracking) || shipment.packed; | ||
| return _.includes(shipment.workflow.workflow, "coreOrderWorkflow/packed") && shipment.tracking || | ||
| _.includes(shipment.workflow.workflow, "coreOrderWorkflow/packed"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this feels brittle as well. What if there is a custom workflow that doesn't include |
||
| } | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -671,6 +671,11 @@ Meteor.methods({ | |
| order.shipping[0].items.shipped = false; | ||
| order.shipping[0].items.delivered = false; | ||
|
|
||
| // begin a new shipping workflow for the order | ||
| order.shipping[0].workflow = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't assume that this shipment is the only shipment. |
||
| status: "new", | ||
| workflow: ["coreOrderWorkflow/notStarted"] | ||
| }; | ||
| order.billing[0].currency.exchangeRate = exchangeRate; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This issue seems like it is older than this particular PR because this code is older, but needs to get solved ASAP. Can't assume that this is the only payment in this order, or that the 0th payment corresponds with this shipment. I'll repeat myself a few more times: hardcoded 0th indexes are an indicator that code is not ready for marketplace. Hard coded indexes, 0th especially, in general are a code smell. It's a sign that that the data structure doesn't fit the application, or that you don't have a better way to identify elements in the array other than just arbitrarily taking the first one. |
||
| order.workflow.status = "new"; | ||
| order.workflow.workflow = ["coreOrderWorkflow/created"]; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,30 +67,16 @@ export const methods = { | |
| * @summary update picking status | ||
| * @param {Object} order - order object | ||
| * @param {Object} shipment - shipment object | ||
| * @param {Boolean} picked - picked status | ||
| * @return {Object} return workflow result | ||
| */ | ||
| "orders/shipmentPicked": function (order, shipment, picked) { | ||
| "orders/shipmentPicked": function (order, shipment) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels brittle to me as well. I'm always open to new ways to do things, but it seems like we should have "orders/progressStatus" and "orders/regressStatus" methods instead of one method per status. We could then have a set of workflows that identifies the next or previous status from the existing status and proceeds. There could be different states for items than for orders, and each should be overridable. This seems like it would make it easier to extend or replace functionality because if you replace the workflow set, then you replace the way that orders progress. |
||
| check(order, Object); | ||
| check(shipment, Object); | ||
| check(picked, Boolean); | ||
|
|
||
| if (!Reaction.hasPermission("orders")) { | ||
| throw new Meteor.Error(403, "Access Denied"); | ||
| } | ||
|
|
||
| Orders.update({ | ||
| "_id": order._id, | ||
| "shipping._id": shipment._id | ||
| }, { | ||
| $set: { | ||
| "shipping.$.picked": picked, | ||
| "shipping.$.workflow.status": "coreOrderWorkflow/picked" | ||
| }, $push: { | ||
| "shipping.$.workflow.workflow": "coreOrderWorkflow/picked" | ||
| } | ||
| }); | ||
|
|
||
| // Set the status of the items as picked | ||
| const itemIds = shipment.items.map((item) => { | ||
| return item._id; | ||
|
|
@@ -103,7 +89,9 @@ export const methods = { | |
| "shipping._id": shipment._id | ||
| }, { | ||
| $set: { | ||
| "shipping.$.picked": picked | ||
| "shipping.$.workflow.status": "coreOrderWorkflow/picked" | ||
| }, $push: { | ||
| "shipping.$.workflow.workflow": "coreOrderWorkflow/picked" | ||
| } | ||
| }); | ||
| } | ||
|
|
@@ -115,32 +103,18 @@ export const methods = { | |
| * @summary update packing status | ||
| * @param {Object} order - order object | ||
| * @param {Object} shipment - shipment object | ||
| * @param {Boolean} packed - packed status | ||
| * @return {Object} return workflow result | ||
| */ | ||
| "orders/shipmentPacked": function (order, shipment, packed) { | ||
| "orders/shipmentPacked": function (order, shipment) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See comments on unifying status progression methods in the shipmentPicked comment. |
||
| check(order, Object); | ||
| check(shipment, Object); | ||
| check(packed, Boolean); | ||
|
|
||
| // REVIEW: who should have permission to do this in a marketplace setting? | ||
| // Do we need to update the order schema to reflect multiple packers / shipments? | ||
| if (!Reaction.hasPermission("orders")) { | ||
| throw new Meteor.Error(403, "Access Denied"); | ||
| } | ||
|
|
||
| Orders.update({ | ||
| "_id": order._id, | ||
| "shipping._id": shipment._id | ||
| }, { | ||
| $set: { | ||
| "shipping.$.packed": packed, | ||
| "shipping.$.workflow.status": "coreOrderWorkflow/packed" | ||
| }, $push: { | ||
| "shipping.$.workflow.workflow": "coreOrderWorkflow/packed" | ||
| } | ||
| }); | ||
|
|
||
| // Set the status of the items as packed | ||
| const itemIds = shipment.items.map((item) => { | ||
| return item._id; | ||
|
|
@@ -153,7 +127,9 @@ export const methods = { | |
| "shipping._id": shipment._id | ||
| }, { | ||
| $set: { | ||
| "shipping.$.packed": packed | ||
| "shipping.$.workflow.status": "coreOrderWorkflow/packed" | ||
| }, $push: { | ||
| "shipping.$.workflow.workflow": "coreOrderWorkflow/packed" | ||
| } | ||
| }); | ||
| } | ||
|
|
@@ -166,30 +142,16 @@ export const methods = { | |
| * @summary update labeling status | ||
| * @param {Object} order - order object | ||
| * @param {Object} shipment - shipment object | ||
| * @param {Boolean} labeled - labeled status | ||
| * @return {Object} return workflow result | ||
| */ | ||
| "orders/shipmentLabeled": function (order, shipment, labeled) { | ||
| "orders/shipmentLabeled": function (order, shipment) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See comments on unifying status progression methods in the shipmentPicked comment. |
||
| check(order, Object); | ||
| check(shipment, Object); | ||
| check(labeled, Boolean); | ||
|
|
||
| if (!Reaction.hasPermission("orders")) { | ||
| throw new Meteor.Error(403, "Access Denied"); | ||
| } | ||
|
|
||
| Orders.update({ | ||
| "_id": order._id, | ||
| "shipping._id": shipment._id | ||
| }, { | ||
| $set: { | ||
| "shipping.$.labeled": labeled, | ||
| "shipping.$.workflow.status": "coreOrderWorkflow/labeled" | ||
| }, $push: { | ||
| "shipping.$.workflow.workflow": "coreOrderWorkflow/labeled" | ||
| } | ||
| }); | ||
|
|
||
| // Set the status of the items as labeled | ||
| const itemIds = shipment.items.map((item) => { | ||
| return item._id; | ||
|
|
@@ -202,7 +164,9 @@ export const methods = { | |
| "shipping._id": shipment._id | ||
| }, { | ||
| $set: { | ||
| "shipping.$.labeled": labeled | ||
| "shipping.$.workflow.status": "coreOrderWorkflow/labeled" | ||
| }, $push: { | ||
| "shipping.$.workflow.workflow": "coreOrderWorkflow/labeled" | ||
| } | ||
| }); | ||
| } | ||
|
|
@@ -378,13 +342,11 @@ export const methods = { | |
| * @summary trigger shipmentShipped status and workflow update | ||
| * @param {Object} order - order object | ||
| * @param {Object} shipment - shipment object | ||
| * @param {Boolean} shipped - shipped status | ||
| * @return {Object} return results of several operations | ||
| */ | ||
| "orders/shipmentShipped": function (order, shipment, shipped) { | ||
| "orders/shipmentShipped": function (order, shipment) { | ||
| check(order, Object); | ||
| check(shipment, Object); | ||
| check(shipped, Match.Maybe(Boolean)); | ||
|
|
||
| // TODO: Who should have access to ship shipments in a marketplace setting | ||
| // Should be anyone who has product in an order. | ||
|
|
@@ -429,7 +391,6 @@ export const methods = { | |
| "shipping._id": shipment._id | ||
| }, { | ||
| $set: { | ||
| "shipping.$.shipped": shipped || true, | ||
| "shipping.$.workflow.status": "coreOrderWorkflow/shipped" | ||
| }, $push: { | ||
| "shipping.$.workflow.workflow": "coreOrderWorkflow/shipped" | ||
|
|
@@ -488,10 +449,9 @@ export const methods = { | |
| "shipping._id": shipment._id | ||
| }, { | ||
| $set: { | ||
| "shipping.$.delivered": true, | ||
| "shipping.$.workflow.status": "delivered" | ||
| "shipping.$.workflow.status": "coreOrderWorkflow/delivered" | ||
| }, $push: { | ||
| "shipping.$.workflow.workflow": "delivered" | ||
| "shipping.$.workflow.workflow": "coreOrderWorkflow/delivered" | ||
| } | ||
| }); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't assume that there is only one shipment for any given order.
template.order.shipping[0]is too brittle for multiple shipments per order and in a marketplace setting we have to support multiple shipments per order.In general, hard-coding the 0th index for an array is a code smell, especially as we continue to build multi-shop support