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

**URL:** https://meta.discourse.org/t/sitesetting-file-format-update-for-locale-override-and-hidden-params-based-on-rails-env/66254
**Category:** Development
**Created:** [7월 15, 2017, 11:48오전 UTC](https://meta.discourse.org/t/sitesetting-file-format-update-for-locale-override-and-hidden-params-based-on-rails-env/66254 "2017-07-15T11:48:24Z")
**Posts on this page:** 9
**Page:** 1

<div class="post-metadata">

### Author: ![fantasticfears](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/fantasticfears/32/119608_2.png) [@fantasticfears](https://meta.discourse.org/u/fantasticfears)
#### Post date: [7월 15, 2017, 11:48오전 UTC](https://meta.discourse.org/t/sitesetting-file-format-update-for-locale-override-and-hidden-params-based-on-rails-env/66254/1 "2017-07-15T11:48:24Z")

</div>

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:

```yml
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:

```yml
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.

```yml
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.

---

<div class="post-metadata">

### Author: ![erlend\_sh](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/erlend_sh/32/119475_2.png) [@erlend\_sh](https://meta.discourse.org/u/erlend_sh)
#### Post date: [7월 17, 2017, 7:33오후 UTC](https://meta.discourse.org/t/sitesetting-file-format-update-for-locale-override-and-hidden-params-based-on-rails-env/66254/2 "2017-07-17T19:33:02Z")

</div>

[@team](https://meta.discourse.org/groups/team) any input here?

---

<div class="post-metadata">

### Author: ![sam](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/sam/32/102149_2.png) [@sam](https://meta.discourse.org/u/sam)
#### Post date: [7월 17, 2017, 8:24오후 UTC](https://meta.discourse.org/t/sitesetting-file-format-update-for-locale-override-and-hidden-params-based-on-rails-env/66254/4 "2017-07-17T20:24:51Z")

</div>

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?

---

<div class="post-metadata">

### Author: ![eviltrout](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/eviltrout/32/5275_2.png) [@eviltrout](https://meta.discourse.org/u/eviltrout)
#### Post date: [7월 18, 2017, 2:48오후 UTC](https://meta.discourse.org/t/sitesetting-file-format-update-for-locale-override-and-hidden-params-based-on-rails-env/66254/6 "2017-07-18T14:48:28Z")

</div>

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

---

<div class="post-metadata">

### Author: ![sam](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/sam/32/102149_2.png) [@sam](https://meta.discourse.org/u/sam)
#### Post date: [7월 18, 2017, 2:52오후 UTC](https://meta.discourse.org/t/sitesetting-file-format-update-for-locale-override-and-hidden-params-based-on-rails-env/66254/7 "2017-07-18T14:52:36Z")

</div>

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.

---

<div class="post-metadata">

### Author: ![fantasticfears](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/fantasticfears/32/119608_2.png) [@fantasticfears](https://meta.discourse.org/u/fantasticfears)
#### Post date: [7월 20, 2017, 12:12오후 UTC](https://meta.discourse.org/t/sitesetting-file-format-update-for-locale-override-and-hidden-params-based-on-rails-env/66254/10 "2017-07-20T12:12:23Z")

</div>

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

---

<div class="post-metadata">

### Author: ![sam](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/sam/32/102149_2.png) [@sam](https://meta.discourse.org/u/sam)
#### Post date: [7월 20, 2017, 1:51오후 UTC](https://meta.discourse.org/t/sitesetting-file-format-update-for-locale-override-and-hidden-params-based-on-rails-env/66254/12 "2017-07-20T13:51:02Z")

</div>

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

---

<div class="post-metadata">

### Author: ![sam](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/sam/32/102149_2.png) [@sam](https://meta.discourse.org/u/sam)
#### Post date: [7월 25, 2017, 7:24오후 UTC](https://meta.discourse.org/t/sitesetting-file-format-update-for-locale-override-and-hidden-params-based-on-rails-env/66254/13 "2017-07-25T19:24:48Z")

</div>

Hi Erick,

Any updates here? Are you blocked on anything?

---

<div class="post-metadata">

### Author: ![fantasticfears](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/fantasticfears/32/119608_2.png) [@fantasticfears](https://meta.discourse.org/u/fantasticfears)
#### Post date: [7월 26, 2017, 6:11오전 UTC](https://meta.discourse.org/t/sitesetting-file-format-update-for-locale-override-and-hidden-params-based-on-rails-env/66254/14 "2017-07-26T06:11:40Z")

</div>

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 😉

- ✅ Refactor

- ✅ Rebased

- ✅ Spec passed

- 🏁 Peer review [https://github.com/discourse/discourse/pull/4967](https://github.com/discourse/discourse/pull/4967)

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
