Add status field to Orders #1391
Conversation
| @@ -0,0 +1,125 @@ | |||
| 'use strict'; | |||
| import status from '../server/constants/order_status'; | |||
There was a problem hiding this comment.
Just a note, we ave several style in this code base for naming files, would be great to pick one (order_status vs order-status vs orderStatus).
There was a problem hiding this comment.
I agree. I used this convention to match the expense_status.js file. I'll create an issue to standardize this.
There was a problem hiding this comment.
After having looked at several repositories, looks like the current common practice is ... hyphen case! So that would be order-status. Anyway, let's take care of that later.
server/graphql/mutations/orders.js
Outdated
| if (!orderCreated.processedAt) { | ||
| // TODO: Order should be updated with data JSON field to store the error to review later | ||
| orderCreated.update({ status: status.ERROR }); | ||
| } | ||
| throw e; |
There was a problem hiding this comment.
Do you need to throw the error as you're already inside the catch?
There was a problem hiding this comment.
I believe it was thrown again to bubble it back up to the graphql response, otherwise the frontend never sees the error.
There was a problem hiding this comment.
got it! LGTM then..
There was a problem hiding this comment.
Rock solid!
I agree with @znarf about having a standard to name files but it doesn't mean order_status.js may go with the standard, so it's all good here!
HipsterBrown
force-pushed
the
order-status
branch
from
e037a78
to
44078b7
Compare
HipsterBrown
force-pushed
the
order-status
branch
from
80d15c1
to
7096fb4
Compare
HipsterBrown
force-pushed
the
order-status
branch
from
7096fb4
to
b2a9f0c
Compare
To facilitate creating pledges for collectives (see opencollective/opencollective#1168), this PR adds a
statusfield to Orders:The migration also includes code to update existing Orders.