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

Remove admin's ability to edit countries and states #2118

Merged
merged 1 commit into from
Aug 17, 2017

Conversation

graygilmore
Copy link
Contributor

@graygilmore graygilmore commented Jul 25, 2017

From the commit:

Allow admins to edit this part of the system could have dramatic
consequences on a store. Developers will still be able to edit these
resources through a migration or directly in the console for extreme
edge cases.

This may also be a step in the direction of using an externally sourced
service such as Carmen.

If anyone can think of any other parts of the system that can also be removed with this change I will happily add to this PR. I think I've found them all but there could be some sneaky ones (like the JS . observe_field that @jhawthorn pointed out).

Closes #2114

TODO

  • Add changelog
  • Fix tests

@cbrunsdon
Copy link
Contributor

Spec failure is unrelated, should be fixed by #2117

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.

A good change. I don’t see a reason to keep - and even add code for- the possibility to list countries and states. What is the benefit of this?

@kennyadsl
Copy link
Member

I don't know, I'm not against this but I have some concerns about how much difficult/long it would be to extend solidus in case a store needs to add some editable field into countries/states. For example options specific for country that would need a custom field editable by admin into the country edit form. I know we have other resources that do not have an admin UI but I have the feeling that countries/states are more used.

I understand the concern about making some of those fields not editable though. What about just disable critical fields in the form?

Of course if our plan is to migrate to Carmen in the future this makes perfect sense.

@graygilmore
Copy link
Contributor Author

A good change. I don’t see a reason to keep - and even add code for- the possibility to list countries and states. What is the benefit of this?

You're right – if we're removing the editing of this we might as well nuke the whole thing. I'd be interested in adding that change if that's something we can agree on.

For example options specific for country that would need a custom field editable by admin into the country edit form.

I think I would rather see these kinds of modifications in a separate model: YourApp::CountryModification – what do you think?

@kennyadsl
Copy link
Member

@graygilmore we discussed about this one in the core team meeting. We'd go with removing this, including listing, from core for several reasons:

  • Solidus will slowly move towards Carmen. It also allow better countries/states I18n;
  • The only country's attribute that could be safely changed is states_required. It is usually a "dev thing" and it can easily be done via console;

We plan to move this backend functionalities to a separate extension so it can be added back to stores that need those easily.

Thanks!

kennyadsl added a commit to solidusio-contrib/solidus_countries_backend that referenced this pull request Jul 26, 2017
This way we can test this extension on a Solidus version that already
removes this code:

solidusio/solidus#2118
kennyadsl added a commit to solidusio-contrib/solidus_countries_backend that referenced this pull request Jul 26, 2017
- add a WIP section, it will be ready when solidusio/solidus#2118 will
  be merged
- add a note to discourage the usage of this extension
- update contributors section
@kennyadsl
Copy link
Member

@graygilmore I've created an extension (solidusio-contrib/solidus_countries_backend) that should have all the countries/states UI code. It currently points to this PR repo/branch to run specs against Solidus without this code.

@graygilmore
Copy link
Contributor Author

@kennyadsl excellent. I will make the changes to this PR to remove these entirely from the backend.

@graygilmore graygilmore force-pushed the remove-country-editing branch from cced5d3 to 5de73d9 Compare July 27, 2017 18:31
@graygilmore
Copy link
Contributor Author

This has been updated with a full removal.

@kennyadsl
Copy link
Member

@graygilmore there's a couple of feature specs failures, due to Locations -> Zones I18n key rename.

@graygilmore graygilmore force-pushed the remove-country-editing branch 2 times, most recently from ad40457 to d9f17e8 Compare July 31, 2017 16:45
@graygilmore
Copy link
Contributor Author

Updated the zones_spec.rb and added a changelog entry.

@graygilmore graygilmore force-pushed the remove-country-editing branch from d9f17e8 to 3bdc688 Compare July 31, 2017 18:33
@kennyadsl
Copy link
Member

The failure is not related, rebasing against master should fix it, Thanks!

@kennyadsl
Copy link
Member

Also, I'd mention the replacement extension (solidusio-contrib/solidus_countries_backend) in both commit message and PR description so that it's super easy to find it if needed, wdyt?

Allow admins to edit this part of the system could have dramatic
consequences on a store. Developers will still be able to edit these
resources through a migration or directly in the console for extreme
edge cases.

This may also be a step in the direction of using an externally sourced
service such as Carmen.
@graygilmore graygilmore force-pushed the remove-country-editing branch from 3bdc688 to 75e5ae4 Compare August 1, 2017 15:42
@graygilmore
Copy link
Contributor Author

@kennyadsl added a note about the extension to the changelog entry. Tests now passing after rebasing with master 🎉

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!

@tvdeyen tvdeyen merged commit 9f0a68e into solidusio:master Aug 17, 2017
@graygilmore graygilmore deleted the remove-country-editing branch August 17, 2017 19:42
@jhawthorn jhawthorn mentioned this pull request Nov 22, 2017
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