SiteSetting file format update for locale override and hidden params based on Rails.env

Dev
1

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:

min_search_term_length:
  client: true
  default:
    development: 2
    default: 3
  hidden:
    production: false
    development: true

As an aside, the overrides setting format will be:

min_search_term_length:
  client: true
  default: 3
  locale_default:
    zh_CN: 1
  hidden: false

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 development category.

Rails.env params in the site setting

Remove any 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 config/site_settings_{dev,env}.yml

  • + 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 config/initializers/005-site_settings.rb.

Merge them with locale_default

Instead of sending them outside the site setting files, we can have a override params to put locale or env.

min_search_term_length:
  client: true
  default: 3
  default_overrides:
    development: 2
    zh_CN: 1

This style also looks good considering such overrides are not much.

3 Likes
2

@team any input here?

1 Like
4

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?

5 Likes
6

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:

  1. Always use override if present
  2. If not overridden, use locale specific
  3. If no locale specific, use default
5 Likes
7

OK,

@fantasticfears so we have a call here.

  • Remove all per-env settings from file, move the logic for per-env overrides into development.rb and test.rb , so you got to make sure that the API still supports the overriding.

  • Go with the locale_default syntax.

  • Make sure there are lots of tests for this stuff, it is super critical code.

3 Likes
10

@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.

12

Sure, give decomposing a shot I would like to see how it looks.

13

Hi Erick,

Any updates here? Are you blocked on anything?

14

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 :wink:

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

8 Likes