-
Notifications
You must be signed in to change notification settings - Fork 796
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
Fix/UI settings overrides #6730
base: main
Are you sure you want to change the base?
Fix/UI settings overrides #6730
Conversation
Signed-off-by: Josh Romero <rmerqg@amazon.com>
Signed-off-by: Josh Romero <rmerqg@amazon.com>
Signed-off-by: Josh Romero <rmerqg@amazon.com>
Signed-off-by: Josh Romero <rmerqg@amazon.com>
Signed-off-by: Josh Romero <rmerqg@amazon.com>
Signed-off-by: Josh Romero <rmerqg@amazon.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6730 +/- ##
==========================================
- Coverage 67.55% 67.51% -0.05%
==========================================
Files 3428 3428
Lines 67341 67341
Branches 10995 10995
==========================================
- Hits 45494 45463 -31
- Misses 19177 19204 +27
- Partials 2670 2674 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these changes Josh!
@joshuarrrr Dont we want a changelog entry for this change? |
Well... originally I was thinking no, in terms of fixing the new theme controls, which haven't yet been released. But now I'm remembering that this was actually a latent bug in the UI settings client, so I'll add a changelog entry. |
Description
There are a couple different fixes here, which can be viewed separately by looking at the separate commits. But the root cause of #6704 was primarily the fact that the implementation of
getOverridesOrDefault
assumed the wrong object shape for theoverrides
property, when it tried to access avalue
property instead of just the value itself. Note that the existing unit test also had this same error.Because the
ui_render_mixin
is an old legacy.js
file, there's not much help from typescript, so I also added some enforcement to make sure we're always sending boolean values to the template.Additionally, I realized that the validation for the
theme:version
setting was not correct, and needed updating to match our current themes.Finally, as requested, I updated the new user theme controls to be deactivated by default.
No changelog necessary, as the previous PR this fixes has not yet been released.
Future work
@AMoo-Miki @manasvinibs - The past discussion and this TODO comment seem to indicate that we don't actually want to maintain support for these overrides long term. But even if we do, I don't think the schema definition for overrides makes much sense:
We probably want users to be aware of if they specify invalid overrides and not silently ignore (which is what was happening with an incorrectly set value in @BionIT 's configuration.
Issues Resolved
Fixes #6704
Screenshot
Testing the changes
The linked issue only surfaced when deploying https://future.playground.opensearch.org/ , and only when not yet authenticated, because the configuration file specified overrides such as:
To validate these fixes, test with authentication enabled, a non-authenticated user, and theme UI Settings overrides.
Changelog
Check List
yarn test:jest
yarn test:jest_integration