Skip to content
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

[Rebase #6730 and fix tests] Fix/UI settings overrides #7776

Merged
merged 2 commits into from
Aug 22, 2024

Conversation

ananzh
Copy link
Member

@ananzh ananzh commented Aug 20, 2024

Description

This PR is originally from #6730. We simply update the tests.

From #6730 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 the overrides property, when it tried to access a value 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:

overrides: schema.object({}, { unknowns: 'allow' }),

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

Changelog

  • skip

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

This PR is originally from opensearch-project#6730.
We simply update the tests.

Signed-off-by: Josh Romero <[email protected]>
Copy link
Contributor

❌ Invalid Prefix

Invalid description prefix. Found "bug". Expected "breaking", "deprecate", "feat", "fix", "infra", "doc", "chore", "refactor", "security", "skip", or "test".

@github-actions github-actions bot added failed changeset Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry labels Aug 20, 2024
Copy link

codecov bot commented Aug 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.80%. Comparing base (9d3ca79) to head (498376f).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7776      +/-   ##
==========================================
- Coverage   63.83%   63.80%   -0.04%     
==========================================
  Files        3655     3656       +1     
  Lines       81126    81205      +79     
  Branches    12923    12949      +26     
==========================================
+ Hits        51783    51809      +26     
- Misses      26165    26215      +50     
- Partials     3178     3181       +3     
Flag Coverage Δ
Linux_1 30.08% <0.00%> (-0.01%) ⬇️
Linux_2 55.87% <100.00%> (ø)
Linux_3 40.42% <0.00%> (+<0.01%) ⬆️
Linux_4 31.28% <0.00%> (-0.01%) ⬇️
Windows_1 30.09% <0.00%> (-0.01%) ⬇️
Windows_2 55.82% <100.00%> (ø)
Windows_3 ?
Windows_4 31.28% <0.00%> (-0.01%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ananzh ananzh force-pushed the fix/ui_settings_overrides-1 branch from c9a89a3 to 43e8f59 Compare August 21, 2024 01:43
ruanyl
ruanyl previously approved these changes Aug 21, 2024
Copy link
Member

@ruanyl ruanyl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look good to me, I'm just curious where to find theme:enableUserControl on the UI? Somehow I couldn't find it in advance settings page.

SuZhou-Joe
SuZhou-Joe previously approved these changes Aug 21, 2024
Signed-off-by: Anan Zhuang <[email protected]>
@ananzh ananzh dismissed stale reviews from SuZhou-Joe and ruanyl via 498376f August 22, 2024 00:02
@ananzh ananzh force-pushed the fix/ui_settings_overrides-1 branch from 43e8f59 to 498376f Compare August 22, 2024 00:02
@ananzh ananzh changed the title Fix/UI settings overrides [Rebase #6730 and fix tests] Fix/UI settings overrides Aug 22, 2024
@ananzh
Copy link
Member Author

ananzh commented Aug 22, 2024

Code changes look good to me, I'm just curious where to find theme:enableUserControl on the UI? Somehow I couldn't find it in advance settings page.

Not sure. If you search theme, will you see it?
Screenshot 2024-08-21 at 9 08 23 AM

@ananzh
Copy link
Member Author

ananzh commented Aug 22, 2024

@ruanyl @SuZhou-Joe I fixed the last test to make it more robust. Can you guys help me to re-approve it?

@ananzh ananzh mentioned this pull request Aug 22, 2024
7 tasks
@ananzh ananzh merged commit d7d0777 into opensearch-project:main Aug 22, 2024
70 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-7776-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 d7d07774add5fcf950244a43630c9ffad68f9678
# Push it to GitHub
git push --set-upstream origin backport/backport-7776-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-7776-to-2.x.

@andy-townsend
Copy link

andy-townsend commented Sep 13, 2024

Code changes look good to me, I'm just curious where to find theme:enableUserControl on the UI? Somehow I couldn't find it in advance settings page.

Did you manage to resolve this @ruanyl ? I'm running 2.16 but can't see this under advanced settings either.

If I try adding this to my opensearch_dashboards.yaml and restart

    uiSettings:
      overrides:
        "theme:enableUserControl": true

Then for me it appears under General (rather than appearance) and doesn't give the same warnings;
image

@joshuali925
Copy link
Member

@andy-townsend the enableUserControl comes from a new theme feature introduced in #5652, it was reverted in 2.x by #6978 due to some bugs and lack of test coverage, so it didn't get released in 2.16.

To release this feature, we should

  1. add some integration tests
  2. merge theme relevant PRs to main first: [BUG] Allow user Theme Selection retain theme selection #7787, Support overriding theme through URL search parameter #7791
  3. in 2.x undo Revert 815d2bd7c612450e2a15a6543c7e74558adf4a81 "[Manual Backport 2.x] Feat (core): Make theme settings user-specific and user-configurable (#5652) (#6681)" #6978, then backport this PR and the two PRs mentioned above

@AMoo-Miki
Copy link
Collaborator

Decided against backporting.

@ananzh ananzh removed the bug Something isn't working label Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distinguished-contributor Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry v2.17.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Startup fails with theme overrides
7 participants