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

[ENHANCEMENT] Added province code to AddressView #600

Merged
merged 10 commits into from
Dec 19, 2019
Merged

[ENHANCEMENT] Added province code to AddressView #600

merged 10 commits into from
Dec 19, 2019

Conversation

AndreasA
Copy link

Releates: #506 506

@AndreasA AndreasA requested a review from a team as a code owner November 11, 2019 14:40
@mamazu
Copy link
Member

mamazu commented Nov 11, 2019

First of all, thanks for your pull request. This is definitely an improvement. Currently we have two ways of handleing the address, one part is the view factory that you changed which affects how the addresses are rendered in the cart. The other part is the CRUD bundle, that renders the addresses with it's own logic (controlled by the serializer).

It would be nice if you could fix the tests and maybe also add one that checks that the cart now shows the correct address fields.

@mamazu mamazu self-assigned this Nov 11, 2019
@AndreasA
Copy link
Author

AndreasA commented Nov 11, 2019

Sorry for not fixing the tests first but I had to exit my train. I will probably fix them tomorrow morning and add another test.

In regards to CRUD etc. from what I can tell everywhere else the provinceCode seems to work fine (when using my JS code with it). The only issue was the AddressView.

One thing though that is a bit annoying when saving an address (address book) you can only send either provinceCode (if the country has provinces assigned to it) or provinceName if the country does not have provinces assigned to it. Means unnecessary data changes when creating generalized code but that isn't that big an issue.

@AndreasA
Copy link
Author

@mamazu Fixed the tests and added a test for address with province codes instead of province names.

@mamazu
Copy link
Member

mamazu commented Nov 12, 2019

Thanks, this pull request looks great. 👍

Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

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

Hello @AndreasA

Welcome to our community ;) Nice PR 👍

@mamazu mamazu merged commit c6587a5 into Sylius:master Dec 19, 2019
@mamazu
Copy link
Member

mamazu commented Dec 19, 2019

Thank you, Andreas! 🥇

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