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

Rename method_type into partial_name #1978

Merged
merged 1 commit into from
Jul 2, 2017

Conversation

tvdeyen
Copy link
Member

@tvdeyen tvdeyen commented May 29, 2017

PaymentMethod#method_type is a confusing name for a method returning the name of the partial to use.

As we cannot use to_partial_path (we use it in four different places), we use partial_name as more descriptive name.

This removes the key from the /api/orders/:id API (How ever this landed up there). As I can't imagine a good approach for deprecating API keys renaming is the only option I have.

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.

I approve, this makes the purpose of this method much more clear. Needs a rebase for the Changelog though. :)

@tvdeyen tvdeyen force-pushed the rename-method-type branch 2 times, most recently from a3e8b31 to 5675a46 Compare June 8, 2017 12:31
@@ -2,7 +2,7 @@ object @order
extends "spree/api/orders/order"

child :available_payment_methods => :payment_methods do
attributes :id, :name, :method_type
attributes :id, :name, :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.

We probably want to keep the old field around (until the next major version)

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean additionally? Yes, this is a good idea

@tvdeyen
Copy link
Member Author

tvdeyen commented Jun 21, 2017

@jhawthorn rebased on master and re-added the method_type key to the api response.

@gmacdougall
Copy link
Member

@tvdeyen can we get a rebase on this?

PaymentMethod#method_type is a confusing name for a method returning the name of the partial to use.

As we cannot use `to_partial_path` (we use it in four different places), we use `partial_name` as more descriptive name.
@tvdeyen
Copy link
Member Author

tvdeyen commented Jul 2, 2017

@gmacdougall done

@mamhoff mamhoff merged commit 94827fd into solidusio:master Jul 2, 2017
@jhawthorn
Copy link
Contributor

@tvdeyen Sorry, this was reverted in #2057 as the deprecation warnings were causing CI failures

@tvdeyen tvdeyen deleted the rename-method-type branch July 4, 2017 20:24
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.

4 participants