-
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
[WIP] Stripe Connect Capture charge #2824
Conversation
Probably not the place for this, but I'm still pretty sure we decided we were going to remove the |
createdAt: 1 | ||
} | ||
} | ||
], selector); |
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.
should this selector
be { observeSelector: selector }
or similar?
if (Roles.userIsInRole(this.userId, ["admin", "owner", "orders"], shopId)) { | ||
Counts.publish(this, "order-count", Orders.find({ shopId: shopId }), { noReady: true }); | ||
return Orders.find({ shopId: shopId }); | ||
ReactiveAggregate(this, 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.
How is this filtered initially? - does it use the selector
which turns into observeSelector
?
cartId: 1, | ||
sessionId: 1, | ||
shopId: 1, | ||
workflow: 1, |
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 probably should make sure that any give shop can't move the overall order workflow forward. The overall workflow should only be progressed/regressed by some type of system process or hook that knows when all shipments/payments/items have been progressed.
Also discussion topic, maybe we don't use this field at all? Maybe we keep it for marketplace?
shopId: 1, | ||
workflow: 1, | ||
discount: 1, | ||
tax: 1, |
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.
Let's look into this tax and make sure it's marketplace compatible.
return Orders.find({ shopId: shopId }); | ||
ReactiveAggregate(this, Orders, [ | ||
{ | ||
$project: { |
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.
Overall, I think using a projection like this is definitely the right way to filter orders for marketplaces.
$filter: { | ||
input: "$items", | ||
as: "item", | ||
cond: { $eq: ["$$item.shopId", shopId] } |
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.
Let's consider using $in
and passing in an array of shopIds instead of checking against one shopId. That would permit marketplace owners or multi-shop admins to see several shops rather than just one.
That being said, it might make the UI far more complicated, so it's possible that it's better to permit those types of users to switch between the shops they are viewing the order as.
@rymorgan any thoughts on this?
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.
In Mongo3.2, $in
is not supported in projections. It is in 3.4
// send raw error to server log, but sanitized version to client | ||
const sanitisedError = { | ||
details: "An unexpected error has occurred" | ||
StripeApi.methods.createCharge = function ({ chargeObj, apiKey }) { |
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.
I don't think this method is used anymore, replaced by createCharges
.
} else { | ||
stripe = require("stripe")(apiKey); | ||
} | ||
StripeApi.methods.captureCharge = function ({ transactionId, captureDetails, apiKey }) { |
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.
Once all of this "works" for both direct and regular charges, I'd like to see us use a consistent style from method to method. The stripe/payment/createCharges
method uses the export const methods
style vs these using the StripeApi.methods
let's figure out which direction we want to go here and update the other ones to match.
It seems like the rest of the app uses the export const methods
style.
@aaronjudd or @machikoyasuda any preference on method naming conventions?
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.
Yeah, the StripeApi
was a convention created before we had import/exports. I think we can eliminate it and combine stripeapi.js
and stripe.js
into one file. Most of this was to allow for testing but I question the value of doing it that way now.
* @param {Array} pipeline - The mongo aggregation pipeline to run | ||
* @param {Object} options - an object of options | ||
* @returns {Cursor} A mongo cursor for subscription | ||
* @constructor |
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.
Should this be using @constructor
? My understanding of that tag would be that it's used for classes.
@machikoyasuda ?
* @returns {Cursor} A mongo cursor for subscription | ||
* @constructor | ||
*/ | ||
export function ReactiveAggregate(sub, collection, pipeline, options) { |
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.
Should this be on the Reaction namespace? and where should it live?
Closing this until I have finished all stages of Stripe Connect. |
Resolves #2765
Also makes Approvals happen and be by shop for each order.
Note This adds filtering by shop for an order. This feels a little bit not great but seems to work. Take a close look at the
reactiveAggregate.js
file that allows us to treat an aggregated result like a collection. If it does work then we can use it everywhere we need to filter collections by shop.To Test