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

Fix view for orders api (Fixes Issue #2512) #2513

Merged
merged 2 commits into from
Apr 4, 2018

Conversation

skukx
Copy link
Contributor

@skukx skukx commented Jan 18, 2018

Refactor _big.json.jbuilder so that it dynamically loads the payment
source view based on that source's payment method.

Add payment views for each payment method which will be used to render a
payment source as json.

Fixes #2512.

@skukx skukx force-pushed the BUG/orders-api-payment-sources branch 4 times, most recently from 832bce7 to 78396ab Compare January 18, 2018 21:35
@skukx skukx changed the title Fix view for orders api Fix view for orders api (Fixes Issue #2512) Jan 18, 2018
Copy link
Contributor

@ericsaupe ericsaupe left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

Would you mind adding a test for this? Also, the partial name for credit cards should be _credit_card.json.jbuilder at this point if I'm not mistaken.

@skukx
Copy link
Contributor Author

skukx commented Jan 19, 2018

Yeah I can add a test for this. As far as the naming convention, I followed convention on how Spree admin chooses to display payment sources.

See: https://github.com/solidusio/solidus/tree/master/backend/app/views/spree/admin/payments/source_views

If we want to move away from that convention I will change the views mimic that payment source rather than the method

@mamhoff
Copy link
Contributor

mamhoff commented Jan 19, 2018

Sorry, I thought we changed the partial name with deprecating the Spree::Gateway class, but we didn't:
https://github.com/solidusio/solidus/blob/master/core/app/models/spree/payment_method/credit_card.rb#L15

So, keep the partial name, but do add a spec. ❤️

@skukx skukx force-pushed the BUG/orders-api-payment-sources branch from 78396ab to f941499 Compare January 19, 2018 17:41
@skukx
Copy link
Contributor Author

skukx commented Jan 19, 2018

Let me know if the specs added are sufficient


it 'renders the payment source view for gateway' do
subject
expect(response).to render_template partial: "spree/api/payments/source_views/_#{payment.payment_method.partial_name}"
Copy link
Contributor

Choose a reason for hiding this comment

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

These are sufficient, but should be more explicit. Either render_template('spree/api/payments/source_views/_gateway.html.erb), or test directly for the right thing being rendered as JSON.

@skukx skukx force-pushed the BUG/orders-api-payment-sources branch from f941499 to ce50561 Compare January 19, 2018 19:56
@skukx
Copy link
Contributor Author

skukx commented Jan 19, 2018

Made changes as requested

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -25,11 +25,10 @@ json.payments(order.payments) do |payment|
json.payment_method { json.(payment.payment_method, :id, :name) }
json.source do
if payment.source
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible that payment.source is a Spree::Payment.
See: https://github.com/solidusio/solidus/blob/v2.4/backend/app/views/spree/admin/payments/show.html.erb#L16

We will need to account for that situation here as well:

json.source do
    ##
    # payment.source could be a Spree::Payment. If it is then we need to call
    # source twice.
    # @see https://github.com/solidusio/solidus/blob/v2.4/backend/app/views/spree/admin/payments/show.html.erb#L16
    #
    payment_source = payment.source.is_a?(Spree::Payment) ? payment.source.source : payment.source
    
    if payment_source
      json.partial!(
        "spree/api/payments/source_views/#{payment.payment_method.partial_name}",
        payment_source: payment_source
      )
    else
      json.nil!
    end
  end

When I get a chance I'll add this to the PR

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)

@skukx
Copy link
Contributor Author

skukx commented Apr 2, 2018

What is the status of this?

@gmacdougall
Copy link
Member

There's a red CircleCI build on this right now. Do you know the source of the errors?

@skukx
Copy link
Contributor Author

skukx commented Apr 2, 2018

I just pushed something new so it may have to do with that. I'll take a look

Taylor Scott added 2 commits April 2, 2018 15:54
Refactor _big.json.jbuilder so that it dynamically loads the payment
source view based on that source's payment method.

Add payment views for each payment method which will be used to render a
payment source as json.
@skukx skukx force-pushed the BUG/orders-api-payment-sources branch from 34c41db to 89cb4d9 Compare April 2, 2018 22:54
@skukx
Copy link
Contributor Author

skukx commented Apr 2, 2018

Look like I just needed to rebase my branch on master

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.

6 participants