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 bug with user address forms #2766

Merged
merged 4 commits into from Jun 14, 2018

Conversation

jacobeubanks
Copy link

refs #2519

Bug: states would not reload with change in country on user address forms

Stock Location JS solved this for stock location forms so it was renamed and used in user address forms.

Notes: Considering this change a refactor so no new tests were added. Let me know if tests should be written.

Jacob Eubanks and others added 2 commits June 5, 2018 17:02
Add Stock Location Address functionality to User Address
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.

I don’t understand why renaming a file and a css class fixes the issue you are refering to.

Could you please provide some more background on how to reproduce the issue in the sandbox and why your fix works? The commit message is unfortunately not very helpful either.

Thanks

@jacobherrington
Copy link
Contributor

Hey @tvdeyen, thanks for the quick review.

The issue occurs in the backend when updating a user's address information. When the country is changed, the states collection does not update. See #2519 for a screenshot of the issue.

You can see the behavior in the sandbox app or any vanilla solidus app at http://localhost:3000/admin/users/1/addresses.

Existing code in stock_locations.js (now locations.js) handled the problem by targeting a CSS class. We added that class to the user's address form, as opposed to rewriting the same code.

We changed the name of the JS file and the CSS class name to reflect an increase in usage. It is now being used in admin/users/_addresses_form.html.erb as well as admin/stock_locations/_form.html.erb.

Let me know if there are any other changes needed.

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.

I see. Thanks for the clarification. This makes sense. I think, though, the name of the file should be addresses instead of location

@jacobeubanks
Copy link
Author

@tvdeyen Agreed! Change made.

Thanks for the speedy responses!

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.

Sorry, still one little nit

@@ -3,7 +3,7 @@
Spree.ready(function() {
"use strict";

_.each(document.querySelectorAll('.js-stock-locations-form'), function(el) {
_.each(document.querySelectorAll('.js-locations-form'), function(el) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we also rename the class to js-addresses-form?

Copy link
Author

Choose a reason for hiding this comment

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

@tvdeyen Good idea! Change made. Thanks!

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

@jacobherrington
Copy link
Contributor

@tvdeyen @kennyadsl just curious, is there anything else we need to do on this PR?

@kennyadsl
Copy link
Member

No I think we just need to merge, doing it now!

@kennyadsl kennyadsl merged commit 89f7dae into solidusio:master Jun 14, 2018
@jacobeubanks
Copy link
Author

@kennyadsl Thank you!

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.

None yet

4 participants