Skip to content

Use OrderContents#update_cart in Api::PaymentsController#create#299

Closed
jordan-brough wants to merge 1 commit into
solidusio:masterfrom
jordan-brough:use-update-cart-in-api-payments-create
Closed

Use OrderContents#update_cart in Api::PaymentsController#create#299
jordan-brough wants to merge 1 commit into
solidusio:masterfrom
jordan-brough:use-update-cart-in-api-payments-create

Conversation

@jordan-brough

Copy link
Copy Markdown
Contributor

First step in getting all payment creation code going through a common
path.

@jhawthorn @cbrunsdon @gmacdougall @athal7 @magnusvk here's a first step in the direction we talked about this afternoon. I haven't started making a CartUpdate class or anything yet but I'm trying to tackle this incrementally and I think this change can stand on its own, but definitely open to any thoughts.

First step in getting all payment creation code going through a common
path.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this respond with the correct payment in a failed scenario? it wouldn't be the order's last payment would it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was worried about that also but it's cached inside the association since we just tried to add it through the association. There's a spec that verifies this:

expect(json_response['errors']['source']).to eq(["can't be blank"])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah cool. if it were me i'd probably want a separate add_payment method that update_cart would use if it detects nested params as opposed to the other way around. but i think this is fine as well. 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@athal7 cool, yeah, just trying to stay real minimal for the moment before moving on to better stuff like that.

@magnusvk

Copy link
Copy Markdown
Contributor

👍 @jhawthorn does this look good to you?

@jordan-brough

Copy link
Copy Markdown
Contributor Author

ping @jhawthorn

@jordan-brough

Copy link
Copy Markdown
Contributor Author

@jhawthorn if you're going to start working on the "CartUpdate" idea perhaps we should have this use CartUpdate instead of update_cart?

@jordan-brough

Copy link
Copy Markdown
Contributor Author

This is taken care of in #357

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants