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

Support user's console_preferences storage #6890

Merged
merged 5 commits into from
Mar 5, 2024

Conversation

nicholaspcr
Copy link
Contributor

@nicholaspcr nicholaspcr commented Jan 29, 2024

Summary

References #6483

Changes

  • Add console_preferences migration
  • Update user_registry to support new user field console_preferences
  • Add tests to user_registry checking the updates to the new field

Testing

Unit tests and manual testing, more details on test steps below.

Test Steps

Create an user and perform the requests listed below. The expected response is attached to each request example.

# Update sort_by field
 curl -X PUT "http://localhost:1885/api/v3/users/<user_id>" \
        -H "Accept: application/json" \
        -H "Authorization: Bearer <token>" \
  -d '{ "user": { "console_preferences": {"console_theme": "DASHBOARD_LAYOUT_GRID"}}, "field_mask": { "paths": [ "console_preferences.console_theme" ] } }'
## Response
{
  "ids": {
    "user_id": "admin"
  },
  "created_at": "2024-02-26T16:07:11.726461Z",
  "updated_at": "2024-02-26T16:12:59.050315Z",
  "console_preferences": {
    "console_theme": "CONSOLE_THEME_LIGHT"
  }
}
# Update dashboard_layouts field
 curl -X PUT "http://localhost:1885/api/v3/users/<user_id>" \
        -H "Accept: application/json" \
        -H "Authorization: Bearer <token>" \
  -d '{ "user": { "console_preferences": {"dashboard_layouts": {"application": "DASHBOARD_LAYOUT_LIST"}}}, "field_mask": { "paths": [ "console_preferences.dashboard_layouts" ] } }'
## Response
{
  "ids": {
    "user_id": "admin"
  },
  "created_at": "2024-02-26T16:07:11.726461Z",
  "updated_at": "2024-02-26T16:10:13.265911Z",
  "console_preferences": {
    "dashboard_layouts": {
      "application": "DASHBOARD_LAYOUT_LIST"
    }
  }
}
# Update sort_by field
 curl -X PUT "http://localhost:1885/api/v3/users/<user_id>" \
        -H "Accept: application/json" \
        -H "Authorization: Bearer <token>" \
  -d '{ "user": { "console_preferences": {"sort_by": {"application": "name"}}}, "field_mask": { "paths": [ "console_preferences.sort_by" ] } }'
## Response
{
  "ids": {
    "user_id": "admin"
  },
  "created_at": "2024-02-26T16:07:11.726461Z",
  "updated_at": "2024-02-26T16:14:34.919360Z",
  "console_preferences": {
    "sort_by": {
      "application": "name"
    }
  }
}
## GET All console_preferences
curl -i -X GET "http://localhost:1885/api/v3/users/<user_id>?field_mask=name,console_preferences" \
        -H "Accept: application/json" \
        -H "Authorization: Bearer <token>"

# Response
{
  "ids": {
    "user_id": "admin"
  },
  "created_at": "2024-02-26T16:07:11.726461Z",
  "updated_at": "2024-02-26T16:14:34.919360Z",
  "console_preferences": {
    "console_theme": "CONSOLE_THEME_LIGHT",
    "dashboard_layouts": {
      "application": "DASHBOARD_LAYOUT_LIST"
    },
    "sort_by": {
      "application": "name"
    }
  }
}
Regressions

This is the addition of a new field on users table, it should have no impact on the existing features.

Notes for Reviewers

Changed this PR just to avoid having both features (console_preferences and bookmarks in the same PR), the reason for this is just to make it easier if the front-end team wants to check how the specific feature was introduced by inspecting this PR.

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • The steps/process to test this feature are clearly explained including testing for regressions.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@nicholaspcr nicholaspcr self-assigned this Jan 29, 2024
@github-actions github-actions bot added c/identity server This is related to the Identity Server ui/cli This is related to ttn-lw-cli compat/db This could affect Database compatibility labels Jan 29, 2024
@KrishnaIyer
Copy link
Member

@nicholaspcr: Please be aware that this PR should target the v3.30 branch. We'll create one soon.

@nicholaspcr
Copy link
Contributor Author

nicholaspcr commented Jan 30, 2024

https://github.com/TheThingsIndustries/lorawan-stack/issues/4009 relates to the comment that I left explaining why I left the end devices entity out. Meaning that after the mentioned issue is finished we could also include just the device_id in the user's bookmarks.

Previous comment disregarded as is not true due to the problem of app1.dev1 and app2.dev2 being possible, so its not possible to have the bookmark of a device without having the parent entity stored as well.

@nicholaspcr
Copy link
Contributor Author

@nicholaspcr: Please be aware that this PR should target the v3.30 branch. We'll create one soon.

@KrishnaIyer

Notes for Reviewers
Important notes:
This will be merged on v3.30, ignore the target branch while its a draft PR.

@nicholaspcr
Copy link
Contributor Author

Instead of even storing the dashboard_layouts and sort_order as a map<string,string>, it would be more efficient space wise to have each necessary field defined in the UserConsolePreferences. Something like this:

// UserConsolePreferences is the message that defines the user preferences for the Console.
message UserConsolePreferences {
  option (thethings.flags.message) = {
    select: true,
    set: true
  };
  bool dark_mode = 1;
  message DashboardLayouts {
    string ApplicationLayout = 1;
    string GatewayLayout = 2;
    string OrganizationLayout = 3;
  }
  DashboardLayouts dashboard_layouts = 2;
  message SortOrders {
    string ApplicationFieldOrder = 1;
    string GatewayFieldOrder = 2;
    string OrganizationFieldOrder = 3;
  }
  SortOrders sort_orders = 3;
}
{

  "dark_mode": true,
  "dashboard_layouts": {
    "application_layout": "name",
    "gateway_layout": "name",
    "organization_layout": "name"
  },
  "sort_orders": {
    "application_field_order": "name",
    "gateway_field_order": "name",
    "organization_field_order": "name"
  }
}

I believe this doesn't change much of the implementation on the front-end side of things but it makes the process of adding a new field a bit more extensive as you need to manually add each new key to the struct definition. The benefit is that on the DB side of things we wouldn't be storing the keys to the map for each user.

Additionally we can apply validation of an specific entity on the specific field, ensuring the something like application_field_order only has valid application field values before even applying the change.

@kschiffer with this in mind, the only thing missing is what are the initial fields that you would like to have for the ConsolePreferences.

@kschiffer
Copy link
Contributor

kschiffer commented Feb 6, 2024

This looks mainly good to me. Some annotations:

  • We can skip organization_layout as the new design of this will likely not have a dashboard/overview
  • I realized that dark_mode needs three possible values actually: on, off and auto (for using system preferences)
  • What's important for me is that we can make extensions to the preferences relatively easy but from what I see this is currently the case

Regarding the default values: these should be mostly empty where possible. The dark_mode would be off by default.

@nicholaspcr nicholaspcr force-pushed the issue/6483-user-console-preferences branch from 8f1fb12 to bde1ade Compare February 6, 2024 20:13
@github-actions github-actions bot added the ui/web This is related to a web interface label Feb 6, 2024
@nicholaspcr nicholaspcr force-pushed the issue/6483-user-console-preferences branch from bde1ade to fc4f044 Compare February 6, 2024 21:52
@nicholaspcr nicholaspcr changed the base branch from v3.29 to v3.30 February 12, 2024 18:32
@nicholaspcr nicholaspcr force-pushed the issue/6483-user-console-preferences branch 3 times, most recently from 77e9309 to 86548be Compare February 12, 2024 20:01
@nicholaspcr nicholaspcr marked this pull request as ready for review February 12, 2024 20:10
@nicholaspcr nicholaspcr requested review from a team as code owners February 12, 2024 20:10
@nicholaspcr nicholaspcr requested review from a team and adriansmares February 12, 2024 20:10
@nicholaspcr nicholaspcr force-pushed the issue/6483-user-console-preferences branch from 86548be to 49bdec8 Compare February 12, 2024 20:16
@kschiffer
Copy link
Contributor

What's left here to merge this?

Copy link
Contributor

@adriansmares adriansmares left a comment

Choose a reason for hiding this comment

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

I've left my comments in Enterprise.

@nicholaspcr nicholaspcr force-pushed the issue/6483-user-console-preferences branch from 49bdec8 to aa9ce37 Compare March 4, 2024 21:14
@nicholaspcr nicholaspcr requested a review from adriansmares March 4, 2024 22:14
@nicholaspcr nicholaspcr force-pushed the issue/6483-user-console-preferences branch from aa9ce37 to c5dfbef Compare March 5, 2024 13:34
@nicholaspcr nicholaspcr merged commit 45343ba into v3.30 Mar 5, 2024
15 of 16 checks passed
@nicholaspcr nicholaspcr deleted the issue/6483-user-console-preferences branch March 5, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/identity server This is related to the Identity Server compat/db This could affect Database compatibility ui/cli This is related to ttn-lw-cli ui/web This is related to a web interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants