Improve dashboard admin UI settings implementation #10869
Conversation
FriedhelmWS
requested review from
AMoo-Miki,
BionIT,
Flyingliuhub,
LDrago27,
Maosaic,
SuZhou-Joe,
ZilongX,
abbyhu2000,
ananzh,
angle943,
ashwin-pc,
bandinib-amzn,
curq,
d-buckner,
huyaboo,
joshuali925,
joshuarrrr,
kavilla,
manasvinibs,
mengweieric,
ps48,
raintygao,
ruanyl,
sejli,
virajsanghvi,
wanglam,
xinruiba,
zengyan-amazon,
zhongnansu and
zhyuanqi
as code owners
FriedhelmWS
requested review from
TackAdam,
paulstn,
ruchidh and
sumukhswamy
as code owners
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10869 +/- ##
==========================================
- Coverage 60.76% 60.76% -0.01%
==========================================
Files 4533 4533
Lines 122127 122128 +1
Branches 20459 20460 +1
==========================================
Hits 74207 74207
- Misses 42687 42688 +1
Partials 5233 5233
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
Skip-Changelog
| isOverridden: config.isOverridden(setting[0]), | ||
| isPermissionControlled: | ||
| all[setting[0]].scope === UiSettingScope.DASHBOARD_ADMIN && !isDashboardAdmin, | ||
| ((typeof all[setting[0]].scope === 'string' && |
There was a problem hiding this comment.
would be nice to add a comment to describe the logic, it's hard to read
| getAvailableLanguages$(http, data).pipe( | ||
| map((languages) => languages.length > 0 && assistantEnabled$.value) | ||
| combineLatest([getAvailableLanguages$(http, data), assistantEnabled$]).pipe( | ||
| map(([languages, enabled]) => languages.length > 0 && enabled) |
There was a problem hiding this comment.
Nit: just for readability
| map(([languages, enabled]) => languages.length > 0 && enabled) | |
| map(([languages, assistantEnabled]) => languages.length > 0 && assistantEnabled) |
| // Use uiSettings.defaults from the config file | ||
| this.validateAndUpdateConfiguredDefaults(config.uiSettingsConfig.defaults); | ||
|
|
||
| this.register(getAIFeaturesSetting()); |
There was a problem hiding this comment.
Just curious, does the order matter?
There was a problem hiding this comment.
It is just a minor improvement as for settings we should define what exists before we define how to control access to it
| groupedChanges[UiSettingScope.USER][key] = val; | ||
| } else if (this.adminUiSettingsKeys.includes(key)) { | ||
| groupedChanges[UiSettingScope.DASHBOARD_ADMIN][key] = val; | ||
| } else if (this.userLevelSettingsKeys.includes(key)) { |
There was a problem hiding this comment.
Does the order matter? If yes, would be nice to add a comment or unit test
There was a problem hiding this comment.
This is to maintain the existing logic hierarchy that the admin-level configurations should has higher privilege level as initiatively admin setting > user settings > workspace settings > global settings.
Signed-off-by: Owen Wang <[email protected]>
3 participants
Description
Issues Resolved
NA
Screenshot
NA
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integration