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

[API] Add customer show api path #3734

Merged
merged 1 commit into from
Jan 4, 2016
Merged

[API] Add customer show api path #3734

merged 1 commit into from
Jan 4, 2016

Conversation

gonzalovilaseca
Copy link
Contributor

Q A
Bug fix? [yes]
New feature? [no]
BC breaks? [no]
Deprecations? [no]
Fixed tickets
License MIT
Doc PR

sylius_api_customer_show route doesn't exist, should we create it or just use sylius_api_user_show?

@@ -20,7 +20,7 @@ Sylius\Component\User\Model\User:
relations:
- rel: user
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that proper fix will be to change this rel to customer, since we still do id: expr(object.getCustomer().getId()).

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Or if you want the User link, change to:

- rel: self
          href:
              route: sylius_api_user_show
              parameters:
                  id: expr(object.getId())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so should I leave the route as it was before? but then it has to be created in some routing.yml file as it doesn't exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so we have a bug 😄

Proper fix - add route for api customer show
Quick fix - remove this rel

Are you willing to fix this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I found it bc it was breaking my app! I'll fix it during the day.

@gonzalovilaseca
Copy link
Contributor Author

I've added the route in the user.yml file, unless someone thinks it's better to create a customer.yml file

@michalmarcinkowski
Copy link
Contributor

@gonzalovilaseca it's better to create a customer.yml file since now the routing looks like /users/customer/{id}. The proper routing will be /customers/{id}.

@gonzalovilaseca
Copy link
Contributor Author

@michalmarcinkowski updated!

@michalmarcinkowski michalmarcinkowski changed the title Update Model.User.yml [API] Add customer show api path Jan 4, 2016
michalmarcinkowski added a commit that referenced this pull request Jan 4, 2016
[API] Add customer show api path
@michalmarcinkowski michalmarcinkowski merged commit 539eac7 into Sylius:master Jan 4, 2016
@michalmarcinkowski
Copy link
Contributor

Thank you Gonzalo! 👍

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