I am working on the feature to allow overriding site setting based on the
default_locale. @sam have talked with me that site setting based on environment is misleading. So I’d like to draw some proposals here to see which one is the best to go.
For the record, taking an example of the possible format:
As an aside, the overrides setting format will be:
hidden property proposal
hidden only accepts a
true value instead of dependending on env. I don’t think a good use to hide anything in the dev and test environment. There are only 4 cases under the
Rails.env params in the site setting
Rails.env params in the site setting and put them in another yaml file.
Related site settings should be put on a file and loaded with site setting. Can be
+ The site setting file is clean.
+ The site settings are centralized in a place so that it’s easy to follow.
- Another file
I’d prefer this for clarity.
Put them into an initializer
Same pros as above. It’s not that obvious if they are put in
Merge them with
Instead of sending them outside the site setting files, we can have a
override params to put locale or env.
This style also looks good considering such overrides are not much.
My gut feel here is to clean up site setting yml file so it only includes per locale settings and does not have any of the per-env settings.
Otherwise what happens is that devs need to start reasoning about precedence.
- Do env default overrides happen prior to locale overrides?
- Do we need per env per locale overrides?
It gets super complicated fast, so my call here is just to add an initializer that does all the per-env stuff and keep the yml file so it only contains per-locale default overrides.
@eviltrout what do you think?
I definitely think it’s simpler not to include per-env settings in the file.
I think it’s reasonable to add per-locale to the file then follow this order:
- Always use override if present
- If not overridden, use locale specific
- If no locale specific, use default
@fantasticfears so we have a call here.
Remove all per-env settings from file, move the logic for per-env overrides into
test.rb , so you got to make sure that the API still supports the overriding.
Go with the
Make sure there are lots of tests for this stuff, it is super critical code.
@sam, I have to ask for a confirmation.
For the per-env settings, they can be loaded inside the defaults directly. But as of the
default_locale, if it’s being treated as other settings, I’ll have to control all various parts in
SiteSettingExtension. Do you think if it’s better to go with a new module
SiteSettingDefaultLocaleProvider that handles everything about default_locale and notifies the extension module to refresh? In this way, the coupling would be less than hard coded
:default_locale in a lot of places.
Sure, give decomposing a shot I would like to see how it looks.
Any updates here? Are you blocked on anything?
Days spent for two refactors. Many new lines of spec to ensure
locale_default is running and type is enforced for site settings. And all other specs are fixed to this point. The PR is finally ready
As an aside, I put
default_locale in the
required category now. And the client side takes the property in object is ordered implicitly that is not guaranteed by the ECMAScript. So the order of site setting panels may result in position change if such thing happened. Anyway, site setting in the client side needs a migration to the new
RestModel in the future