Skip to content
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

Accept source as permitted attribute importing orders #3066

Merged
merged 1 commit into from
Apr 8, 2019
Merged

Accept source as permitted attribute importing orders #3066

merged 1 commit into from
Apr 8, 2019

Conversation

jtapia
Copy link
Contributor

@jtapia jtapia commented Jan 30, 2019

Description

Sending the source data inside payments payload to spree/api/orders#create is returning an error:
ActiveRecord::RecordInvalid: Validation failed: Source can't be blank

Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

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

@jtapia the changes in file core/spec/lib/spree/core/importer/order_spec.rb are only formatting, I think this creates confusion in PRs that include code changes. I think it would be better to remove these changes from this PR.

It seems to me that the code you added in orders_controller.rb is not covered by new specs, can you please add some? As usual, thank you for your work!

@jtapia
Copy link
Contributor Author

jtapia commented Jan 31, 2019

@spaghetticode I just added a test, please let me know what do you think

@spaghetticode
Copy link
Member

@jtapia I think the tests don't verify that the payment source is correctly saved. They only test that the controller action returns a 201 or a 404.

Also, I ran the tests commeting the code you added in Spree::Api::OrdersController and they still pass, so I'm a bit puzzled 🤔... did not investigate more though.

@jtapia
Copy link
Contributor Author

jtapia commented Feb 4, 2019

@spaghetticode please check again the test case, it's returning:

1) Spree::Api::OrdersController as an admin creation with payment with source creates a payment
     Failure/Error: @order = Spree::Core::Importer::Order.import(determine_order_user, order_params)

     ActiveRecord::RecordInvalid:
       Validation failed: Source can't be blank

Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

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

@jtapia it looks good I left a few comments, it's only minor improvements 👍

api/spec/requests/spree/api/orders_controller_spec.rb Outdated Show resolved Hide resolved
api/spec/requests/spree/api/orders_controller_spec.rb Outdated Show resolved Hide resolved
api/spec/requests/spree/api/orders_controller_spec.rb Outdated Show resolved Hide resolved
@jtapia
Copy link
Contributor Author

jtapia commented Feb 6, 2019

@spaghetticode please let me know what do you think, thanks

Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

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

@jtapia looks good to me , thank you 👍

@jtapia
Copy link
Contributor Author

jtapia commented Feb 6, 2019

@spaghetticode should we remove the Needs Tests tag?

@spaghetticode
Copy link
Member

@jtapia let's wait for somebody from the core team to chime in for that.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Thanks. This is a great addition. But:

We need a way to allow other payment sources than credit cards as well.

api/app/controllers/spree/api/orders_controller.rb Outdated Show resolved Hide resolved
@kennyadsl kennyadsl requested a review from tvdeyen April 3, 2019 14:25
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Great thanks 👏

@kennyadsl kennyadsl merged commit 4d53bdc into solidusio:master Apr 8, 2019
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.

5 participants