-
Notifications
You must be signed in to change notification settings - Fork 311
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 console_preferences update #7423
base: v3.33
Are you sure you want to change the base?
Conversation
case "console_preferences.dashboard_layouts.api_key", | ||
"console_preferences.dashboard_layouts.application", | ||
"console_preferences.dashboard_layouts.collaborator", | ||
"console_preferences.dashboard_layouts.end_device", | ||
"console_preferences.dashboard_layouts.gateway", | ||
"console_preferences.dashboard_layouts.organization", | ||
"console_preferences.dashboard_layouts.overview", | ||
"console_preferences.dashboard_layouts.user": | ||
updateConsolePreferences = true | ||
consolePreferences.DashboardLayouts = pb.ConsolePreferences.GetDashboardLayouts() | ||
case "console_preferences.console_theme": | ||
updateConsolePreferences = true | ||
consolePreferences.ConsoleTheme = pb.ConsolePreferences.ConsoleTheme | ||
case "console_preferences.dashboard_layouts": | ||
updateConsolePreferences = true | ||
consolePreferences.DashboardLayouts = pb.ConsolePreferences.GetDashboardLayouts() | ||
case "console_preferences.sort_by": | ||
case "console_preferences.sort_by.api_key", | ||
"console_preferences.sort_by.application", | ||
"console_preferences.sort_by.collaborator", | ||
"console_preferences.sort_by.end_device", | ||
"console_preferences.sort_by.gateway", | ||
"console_preferences.sort_by.organization", | ||
"console_preferences.sort_by.user": |
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.
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.
My apologies for the delay in reviewing this PR but I do have a question regarding the utility of this change.
Unlike other fields that are introduced via the API, the ConsolePreferences
doesn't support the update of a subspecific field on a store level.
With the existent fieldmask on the updateUserModel
you should be able to update any field of the console_preferences.sort_by
by only passing this fieldmask, although you would have to pass the entire JSON object of SortBy every time you want to update its sub field.
With this context in mind, why do we need to add the subfileds of dashboard_layouts
and sort_by
to the updateUserModel store method?
Couldn't we just set the field mask to be console_preferences.sort_by
and pass the entire sort_by
JSON?
To provide more context, when we are updating the ConsolePreferences of a user we are fetching all of the existent ConsolePreferences JSON from the database, then we are updating the specific fields according to the submask, to finally rewrite the entire ConsolePreference JSON on the DB again. Meaning that the only use we have for specific fieldmasks is to decrease the total size of the message sent by the Console to the IS (send only the subfield instead of the entirety of |
@nicholaspcr Good point, I will try sending the entire json and make sure that works. |
Summary
Unable to update console preferences because the exact field mask was being ignored
Changes
Testing
Steps
There are no console preferences currently being set anywhere so I have been using this function from the frontend
Results
Regressions
...
Notes for Reviewers
This was the minimum to get this to work, I not sure if this now means we override previously set preferences if not set in request. Let me if you would like me to continue to test.
Checklist
README.md
for the chosen target branch.CHANGELOG.md
.CONTRIBUTING.md
, there are no fixup commits left.