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

Add user preference persistence framework #1024

Merged

Conversation

brontolosone
Copy link
Contributor

Towards getodk/central#689
Supersedes #1021 which superseded #1018
Related: getodk/central-backend#1184 (backend)

This introduces persistent user preferences as per getodk/central#689.

They are exposed on the currentUser resource as currentUser.preferences, split up in site and projects objects. These objects are proxied to enable some — hopefully comfortable — abstractions for working with preferences, eg currentUser.preferences.projects[someProjectID].someproperty = "camembert" will work even if projects[someProjectId] doesn't actually exist yet (it will be autocreated).
The PUT and DELETE HTTP requests necessary for propagating values to the backend are implemented as side effects via these proxy objects, too.

The semantics of the preferences values are completely up to the frontend, but the reactiveness enabled is shallow, thus complex preference values (eg objects, arrays) are discouraged — those are inconvenient anyway; more granular key-value pairs are preferred as those will result in less clobbering under concurrent activity from multiple sessions.

Implemented preferences:

  • Project listing sort order
  • Per-project deleted forms hiding (trash collapsed/expanded state)

Todo:

  • fixup existing tests
  • add tests for the new functionality
  • create end-user documentation on potential surprises when one loads preferences which have been merged from multiple sessions

For later:

  • Use named constants as preference keys, and validate the keys (clientside). The file of constants will serve as a versioned registry of what settings are in use (and through git, what settings have been in use over time), this decreases the potential for confusion (such as disparate corners of the frontend using the same preference key for different purposes).

Part of the currentUser resource.
Uses JS proxies to enable some — hopefully comfortable — abstractions
for working with preferences.
@brontolosone brontolosone marked this pull request as draft September 17, 2024 18:54
@matthew-white matthew-white self-requested a review September 18, 2024 13:28
Copy link
Member

@matthew-white matthew-white left a comment

Choose a reason for hiding this comment

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

I still haven't had a chance to write up my idea about reusing existing request logic, but I had a few unrelated comments that I thought I could share sooner.

I mentioned this before, but I really like how the currentUser.preferences proxy works. 👏 Sending requests under the hood, using shallow reactivity, and returning a nice default even when the user doesn't have any preferences for a specific project. 👍

src/components/project/list.vue Outdated Show resolved Hide resolved
src/components/form/trash-list.vue Outdated Show resolved Hide resolved
src/components/form/trash-list.vue Outdated Show resolved Hide resolved
Comment on lines +28 to +31
preferences = {
site: {},
projects: {},
},
Copy link
Member

Choose a reason for hiding this comment

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

Rather than expecting the full preferences object here, maybe it would be useful to allow tests to specify a partial object, then merge that partial object with the full object below. So here, it would be:

preferences = {},

And below, it would be:

// mergeDeepLeft() is from ramda
preferences: mergeDeepLeft(preferences, {
  site: {},
  projects: {}
}),

That would allow a test to easily specify a site preference without also specifying projects: {}, e.g., something like:

// test/components/project/list.spec.js
it('uses the projectSortMode preference', () => {
  // Specifying site without projects.
  mockLogin({ preferences: { site: { projectSortMode: 'alphabetical' } } });
  createProjects([
    { name: 'A' },
    { name: 'B', lastSubmission: ago({ days: 5 }).toISO() }
  ]);
  const component = mountComponent();

  const blocks = mountComponent().findAllComponents(ProjectHomeBlock);
  // ['B', 'A'] is the default, as B has the latest data.
  blocks.map(block => block.props().project.name).should.eql(['A', 'B']);

  const select = component.getComponent(ProjectSort);
  select.props().modelValue.should.equal('alphabetical');
});

Just an idea! Could also come later once tests are added.

@brontolosone brontolosone force-pushed the central_689-userpreferences-rb-02 branch from 669a062 to 4bb37a6 Compare October 7, 2024 15:55
src/request-data/resources.js Outdated Show resolved Hide resolved
src/request-data/resources.js Outdated Show resolved Hide resolved
src/request-data/resources.js Outdated Show resolved Hide resolved
src/request-data/resource.js Outdated Show resolved Hide resolved
src/request-data/resources.js Show resolved Hide resolved
src/request-data/user-preferences.js Outdated Show resolved Hide resolved
@brontolosone brontolosone force-pushed the central_689-userpreferences-rb-02 branch from f3909a5 to 0f8404b Compare October 28, 2024 11:48
@matthew-white
Copy link
Member

When tests are run locally or in CircleCI, this warning is shown:

WARN LOG: '[Vue warn]: Invalid prop: type check failed for prop "modelValue". Expected String with value "undefined", got Undefined ', '
', ' at <ProjectSort', 'modelValue=undefined', 'onUpdate:modelValue=fn', '>', '
', ' at <PageSection>', '
', ' at <ProjectList>', '
', ' at <PageBody>', '
', ' at <Home', 'key=2', '>',

I took a look at what was going on. From the warning, it seems like ProjectSort is being passed undefined for its modelValue prop, which means that sortMode.value is still becoming undefined in ProjectList somehow.

While looking at the tests that logged the warning, I noticed that many rendered the UserEdit component (e.g., after navigating to /account/edit). That component is interesting because it can modify currentUser: if the component requests data about the current user, it will merge that data into currentUser. In production, that data won't include preferences, because (I think) preferences is only returned from /v1/users/current. However, in testing, the mocked response did include preferences. That led to the UserPreferences object being overwritten with a simple object. Unlike UserPreferences, that simple object doesn't include logic to return defaults for user preferences that are not set. I think that's what's causing sortMode.value to end up being undefined.

I think the solution to this is just to make sure that preferences isn't included in the mocked response, matching how things actually work. Then the UserPreferences object won't be overwritten. We already exclude verbs from the mocked response, so I did something similar for preferences. I went ahead and pushed a commit along those lines. (Hope you don't mind, @brontolosone!)

Copy link
Member

@matthew-white matthew-white left a comment

Choose a reason for hiding this comment

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

Just a few more small comments! I may add a few more tomorrow, but I thought I'd go ahead and leave these now.

src/request-data/user-preferences/preferences.js Outdated Show resolved Hide resolved
src/request-data/user-preferences/normalizers.js Outdated Show resolved Hide resolved
src/components/form/trash-list.vue Outdated Show resolved Hide resolved
@matthew-white
Copy link
Member

matthew-white commented Oct 29, 2024

I'm still seeing a slight issue with tests, in test/components/project/list.spec.js. I see things like this being logged:

ERROR LOG: Object{method: 'PUT', url: '/v1/user-preferences/site/projectSortMode', headers: Object{Content-Type: 'application/json'}, data: Object{propertyValue: 'alphabetical'}, signal: AbortSignal{}}
ERROR LOG: 'A request was sent, but not handled. Are you using mockHttp() or load()?'

That happens because tests in that file change the selection for the project sort. That didn't used to send a request, but now it does. Tests expect that a response is specified for every request that's sent.

Interestingly, this isn't leading to tests failing, which I thought it would. It looks like test/run.sh fails if it sees WARN LOG:, but doesn't fail if it sees ERROR LOG:. I'll file an issue about that. (UPDATE: Filed here.)

For the tests that are logging errors, I updated them locally to handle the new requests. However, I had to do something special to account for navigator.locks, which mockHttp() isn't handling quite right. Given that, I think we should wait to update these tests until I take a look at #1044. I don't think we need to update these tests as part of this PR.

@matthew-white
Copy link
Member

@brontolosone and I have discussed merging this PR first, then following up with another PR with tests. Given that, I think this PR can be marked as ready for review?

@matthew-white matthew-white marked this pull request as ready for review October 30, 2024 17:14
Copy link
Member

@matthew-white matthew-white left a comment

Choose a reason for hiding this comment

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

OK, these are all my comments for now, mostly just small things. It's looking great overall. 👍

src/request-data/user-preferences/normalizer.js Outdated Show resolved Hide resolved
src/request-data/user-preferences/preferences.js Outdated Show resolved Hide resolved
src/request-data/user-preferences/preferences.js Outdated Show resolved Hide resolved
src/util/request.js Outdated Show resolved Hide resolved
src/request-data/user-preferences/preferences.js Outdated Show resolved Hide resolved
src/request-data/user-preferences/preferences.js Outdated Show resolved Hide resolved
src/request-data/user-preferences/preferences.js Outdated Show resolved Hide resolved
src/request-data/user-preferences/preferences.js Outdated Show resolved Hide resolved
Copy link
Member

@matthew-white matthew-white left a comment

Choose a reason for hiding this comment

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

Looking good, LGTM! 🚀

I added one last comment, but it's not a blocker for merging.

src/request-data/user-preferences/preferences.js Outdated Show resolved Hide resolved
@brontolosone brontolosone merged commit e80cbb6 into getodk:master Nov 1, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants