Enterprise settings

[dpsutton]

Fixes #23414

To correctly implement a setting into OSS code that allows for enhancements with enterprise, there is a lot of ceremony involved. We put the setting in enterprise code, we then try to require it, and put it behind a second var that can delegate either to the enterprise setting if it exists otherwise some default value or nil.

;; custom namespace metabase-enterprise.embedding.utils that only contains a single setting

(defsetting notification-link-base-url
  (deferred-tru "By default \"Site Url\" is used in notification links, but can be overridden.")
  :visibility :internal
  :getter (fn []
            (when (premium-features/hide-embed-branding?)
              (or (setting/get-value-of-type :string :notification-link-base-url)
                  (public-settings/site-url)))))

;; extra var for indirection
(def ^:private site-url*
  (or (u/ignore-exceptions
        (classloader/require 'metabase-enterprise.embedding.utils)
        (resolve 'metabase-enterprise.embedding.utils/notification-link-base-url))
      (constantly nil)))

And it was important to remember to put the (when (premium-features/hide-embed-branding?) ...) check in there.

Notably many settings related to appearance did not remember to do this check. (application-font, application-colors). These are enterprise changes that affect regular OSS code. But to do it right means the regular OSS code has to jump through hoops conditionally requiring setting namespaces, checking if the feature is enabled, etc.

I originally wanted this to have use a more enterprise related key:

(defsetting application-name
  (deferred-tru "This will replace the word \"Metabase\" wherever it appears.")
  :visibility :public
  :type       :string
  :enterprise :whitelabel
  :default    "Metabase")

But this ran into circularity issues. The enterprise token is a setting. So checking this token in the defsetting macro was circular. Could have hacked around defsetting depending on (defsetting premium-embedding-token ...) , done a delay or something but that's rather gross. So instead it just takes a predicate function:

(defsetting application-name
  (deferred-tru "This will replace the word \"Metabase\" wherever it appears.")
  :visibility :public
  :type       :string
  :enabled?   premium-features/enable-whitelabeling?
  :default    "Metabase")

and update the get function in models.setting to check for that:

(defn get
  "Fetch the value of `setting-definition-or-name`. What this means depends on the Setting's `:getter`; by default, this
  looks for first for a corresponding env var, then checks the cache, then returns the default value of the Setting,
  if any."
  [setting-definition-or-name]
  (let [{:keys [cache? getter enabled? default]} (resolve-setting setting-definition-or-name)
        disable-cache?                           (not cache?)]
    (if (or (nil? enabled?) (enabled?))
      (if (= *disable-cache* disable-cache?)
        (getter)
        (binding [*disable-cache* disable-cache?]
          (getter)))
      default)))

This does mean that all settings value checks need to go through metabase.models.setting/get rather than going to lower-level apis like get-value-of-type. If this becomes an issue we'll have to fixup the :getter to include this information. Not as fun because its inside the macro instead of in a nice helper. But shouldn't be too hard to write some fn that wraps the getter.

[codecov]

Codecov Report

Merging #23441 (ebadb6e) into master (70ba4d5) will decrease coverage by 0.00%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##           master   #23441      +/-   ##
==========================================
- Coverage   64.29%   64.29%   -0.01%     
==========================================
  Files        2635     2634       -1     
  Lines       82720    82722       +2     
  Branches    10269    10268       -1     
==========================================
+ Hits        53183    53184       +1     
- Misses      25326    25329       +3     
+ Partials     4211     4209       -2     

Flag	Coverage Δ
back-end	85.48% <76.92%> (+<0.01%)	⬆️
front-end	44.94% <ø> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/metabase/util/urls.clj	82.35% <0.00%> (+0.53%)	⬆️
src/metabase/public_settings.clj	94.11% <83.33%> (+0.24%)	⬆️
...ase/parameters/components/ParameterValueWidget.jsx	4.76% <0.00%> (-0.33%)	⬇️
frontend/src/metabase/parameters/utils/fields.js	100.00% <0.00%> (ø)
...ontend/src/metabase/parameters/utils/dashboards.ts	92.40% <0.00%> (+0.09%)	⬆️
src/metabase/driver/impl.clj	70.21% <0.00%> (+1.06%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70ba4d5...ebadb6e. Read the comment docs.

[noahmoss]

Current code looks good to me, so stamping to unblock. But should we prevent writing a disabled setting as well?

I'm not sure about the answer since there are some cases where we need to write enterprise feature data in OSS, so that things make sense when an instance upgrades to enterprise (this happens with download permissions, for example). But I'm not sure if that would ever apply to simple settings.

[noahmoss]

One option could be to make the default value for enabled? (constantly true) so that you don't need to worry about the (nil? enabled) check in the getter

[dpsutton]

I think I'm going to leave it as is. My thinking is that enabled? is an optional property and the default is of course settings just have values. I like that its an optional policy and we aren't adding lots of functions to most of them. If we live to regreat this we can easily add the (constantly true) and drop the nil checks.