-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[Marketplace] Set order(s) to a specific status #2613
Conversation
* switch to official Loggly lib for Node and write new Bunyan connector for it * update bunyan to 2.0.0 * add REACTION_LOG_FORMAT config option * add extra comments about log formatting options
2452 fix on UI icon toolbar
Fix on tag menu-sidebar glitch
* add new status for mediaGalleryIndicator * i18n translations for tooltips * status-badge styles for media gallery * update media gallery image new / deleted indicator * style updates * lint fix
# Conflicts: # package.json
* simple fix * fix typo & linting * fix linting issues
* imported LoginFormValidation to forgotContainer * imported LoginFormValidation to passwordOverlayContainer * added setTimeout function to delay rendering * wrap component in translations (#2434) * removed setTimeouts from updatePassword.js * added spinner before mounting the component * replaced setTimeout with life-cycle hook(componentWillRecieveProps) #2422 * imported LoginFormValidation to prevent console error
… added. (#2455) * Update current user's name and profile when address is added * Update user's name when address is updated * Get correct admin user's name from env variables
* Move babelrc into package.json * Updated dependencies * Define missing options variable
On a UI/UX note, I think the way that inbox handles the visibility of the checkbox makes a lot of sense. Before anything has been clicked: After at least one is selected, we're aware of the users intent (to select at least one order), and in inbox they toggle the images to checkboxes without enforcing the hover to make it easier to select multiple. @rymorgan curious what you think about this. I think it would make selecting multiple easier and wouldn't be a drag on the UI when not selecting orders. |
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.
It works as is for single shop orders.
There are a number of places where the 0th index is hard-coded. This will not work for marketplace orders.
I'm also concerned that the implementation of the different states of the order fulfillment process are not extensible and will be difficult to swap out or override.
I think we could consider merging and fixing the marketplace issues in another PR, but I think we need to figure out the flexibility and extensibility of the order fulfillment workflow now.
picked: false, | ||
packed: false, | ||
labeled: false, | ||
shipped: false |
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.
How does this work if there is a shipping workflow that has additional or fewer steps?
@@ -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]); |
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
@@ -193,6 +193,7 @@ Template.coreOrderShippingTracking.helpers({ | |||
const order = Template.instance().order; | |||
const shipment = order.shipping[0]; | |||
|
|||
return shipment.packed && shipment.tracking; | |||
return _.includes(shipment.workflow.workflow, "coreOrderWorkflow/packed") && shipment.tracking || | |||
_.includes(shipment.workflow.workflow, "coreOrderWorkflow/packed"); |
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.
this feels brittle as well. What if there is a custom workflow that doesn't include coreOrderWorkflow/packed
(e.g. uses a different workflow, or pre-packed items, etc.)
packed: "Packed", | ||
labeled: "Labeled", | ||
shipped: "Shipped", | ||
delivered: "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.
How would this be overridden or extended?
@@ -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 = { |
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 this shipment is the only shipment.
order.shipping[0].workflow = { | ||
status: "new", | ||
workflow: ["coreOrderWorkflow/notStarted"] | ||
}; | ||
order.billing[0].currency.exchangeRate = exchangeRate; |
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.
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.
* @param {Object} shipment - shipment object | ||
* @return {Object} return workflow result | ||
*/ | ||
"orders/shipmentPicked": function (order, shipment) { |
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.
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.
* @return {Object} return workflow result | ||
*/ | ||
"orders/shipmentPacked": function (order, shipment, packed) { | ||
"orders/shipmentPacked": function (order, shipment) { |
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.
See comments on unifying status progression methods in the shipmentPicked comment.
* @param {Object} shipment - shipment object | ||
* @return {Object} return workflow result | ||
*/ | ||
"orders/shipmentLabeled": function (order, shipment) { |
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.
See comments on unifying status progression methods in the shipmentPicked comment.
@spencern Yes, totally agree, once you check one box, all the boxes should appear. That makes sense. Will update the design specs. |
@spencern I agree that the implementation of most of this (the 0th index hard coding) isn't marketplace ideal, and needs to be refined. I also agree with rethinking the order fulfillment workflow to be more flexible and extensible, and I'll look into it, with a bit of direction. |
@kieha I don't mind guiding or helping with the direction for any of this, let me know if you would like to pair on it sometime. |
…ctioncommerce/reaction into kieha-bulk-order-status-2341
Merging, additional work will be performed in #2724 |
Resolves #2341
This allows users with permission to view and process orders to be able to set/update the status of multiple orders in bulk.
How to test
Testing
Bulk Actions
Bulk Actions
buttonShipping
column change according to what you selectLabeled
option on a new order). You should see a modal that asks if you're sure you want to skip the shipping flowTesting
Capture
Capture
buttonInvoice
section and observe that the payment has been capturedP.S. The
Print Invoice and Pick Sheet
step that appears on the list underBulk Actions
is not functional yet in this PR.