Add user preference persistence framework #1024

brontolosone · 2024-09-17T18:54:10Z

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.

src/request-data/resources.js

brontolosone · 2024-09-24T10:52:48Z

src/request-data/resources.js

+      _container,
+      abortControllers: {},
+      instanceID: crypto.randomUUID(),
+      propagate: (k, v, projectId) => {


@matthew-white the other week you mentioned you'd like to see the locking dance factored out, I'll have a shot at that soon.

Sorry, I know I promised you some comments! I'll try to get those to you ASAP. I'm hoping that we can reuse some of our existing logic around sending and aborting requests in particular. And yeah, if there's a problem around locking, I suspect that will be relevant in places outside this code. It'd be awesome if you could file an issue describing the race condition you're thinking of, and then we can figure out how we want to address it (ideally in a generic way). I think the part I don't understand yet is why locking is necessary if we also have abort logic.

I think the part I don't understand yet is why locking is necessary if we also have abort logic.

The outer locking serves for the use case you'd expect when you see locking: to protect a critical section (or at least, it looks like a critical section to me, I'm used to reasoning with threads but these are coroutines, so... maybe not? I still need to dive into that.)

The inner locking serves as an easy bookkeeping way to determine whether there's another request (for the same grouping key) in flight. The nice thing is that a lock is guaranteed to be released (as soon as the request resolves or throws) with no further work from my side. So it saves me bookkeeping busywork-code.

I forgot to say that I started out trying to modify the existing aborter implementation - basically I envisioned I'd adapt it so I could call it with a grouping key for my requests (by default initialized to 'default', thus backwards compatible) and then use an object {grouping_key: aborter} for bookkeeping of the aborters, as now you could have N rather than just 0 or 1.
I honestly tried ;-) but it resisted my attemps at malling it (not the code's fault, it's just made for a different use case — there's a big difference between 0|1 and N). And then I thought "hmmm this bookkeeping could be more care-free anyway using locks", plus I thought I saw a race condition, ... and here we are.

When we have time we should walk through it together, that'd probably give the best mutual insights!

matthew-white

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

src/components/form/trash-list.vue

matthew-white · 2024-09-26T14:26:21Z

test/data/users.js

+    preferences = {
+      site: {},
+      projects: {},
+    },


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.

src/components/project/list.vue

See https://github.com/getodk/central-frontend/pull/1024\#pullrequestreview-2332522640

…d privatize some class members

See getodk#1024 (comment)

matthew-white · 2024-10-25T14:30:21Z

src/request-data/resources.js

@@ -15,18 +15,21 @@ import { mergeDeepLeft } from 'ramda';
 import configDefaults from '../config';
 import { computeIfExists, hasVerbs, setupOption, transformForm } from './util';
 import { noargs } from '../util/util';
+import { _container } from './resource';
+import UserPreferences from './user-preferences/preferences';

 export default ({ i18n }, createResource) => {


The first argument here, from which i18n is destructured, is the container. You can get http from there as well.

matthew-white · 2024-10-25T14:32:44Z

src/request-data/resources.js

@@ -15,18 +15,21 @@ import { mergeDeepLeft } from 'ramda';
 import configDefaults from '../config';
 import { computeIfExists, hasVerbs, setupOption, transformForm } from './util';
 import { noargs } from '../util/util';
+import { _container } from './resource';
+import UserPreferences from './user-preferences/preferences';

 export default ({ i18n }, createResource) => {
  // Resources related to the session
  createResource('session');


If you assign this resource to a variable, you can reference it below.

Suggested change

createResource('session');

const session = createResource('session');

matthew-white · 2024-10-25T14:35:02Z

src/request-data/resources.js

    /* eslint-disable no-param-reassign */
    transformResponse: ({ data }) => {
      data.verbs = new Set(data.verbs);
      data.can = hasVerbs;
+      const { requestData, http } = self[_container];


See above: you should be able to access session and http without referencing _container.

matthew-white · 2024-10-25T14:36:31Z

src/request-data/resource.js

@@ -51,7 +51,7 @@ class BaseResource {
  }
 }

-const _container = Symbol('container');
+export const _container = Symbol('container');


_container is supposed to work like a private property, so it'd be great to avoid exporting it.

matthew-white · 2024-10-25T14:40:02Z

src/request-data/resources.js

      return shallowReactive(data);
    }
-    /* eslint-enable no-param-reassign */


It'd be good to re-enable this rule.

matthew-white · 2024-10-25T14:43:00Z

src/request-data/user-preferences.js

Does this file still need to exist? It looks like the code was moved elsewhere.

Add user preference framework

edb8b8e

Part of the currentUser resource. Uses JS proxies to enable some — hopefully comfortable — abstractions for working with preferences.

brontolosone marked this pull request as draft September 17, 2024 18:54

brontolosone mentioned this pull request Sep 17, 2024

Serverside-persisted user preferences #1021

Closed

matthew-white self-requested a review September 18, 2024 13:28

add empty user preferences to user testdata

53d422e

brontolosone commented Sep 19, 2024

View reviewed changes

src/request-data/resources.js Outdated Show resolved Hide resolved

brontolosone commented Sep 24, 2024

View reviewed changes

matthew-white reviewed Sep 26, 2024

View reviewed changes

src/components/project/list.vue Outdated Show resolved Hide resolved

brontolosone mentioned this pull request Oct 1, 2024

Persist and serve user preferences getodk/central-backend#1184

Merged

brontolosone added 3 commits October 1, 2024 12:52

account for potentially missing preferences

58c09ad

See https://github.com/getodk/central-frontend/pull/1024\#pullrequestreview-2332522640

Merge branch 'master' into central_689-userpreferences-rb-02

4467c8b

factor out UserPreferences

4bb37a6

brontolosone force-pushed the central_689-userpreferences-rb-02 branch from 669a062 to 4bb37a6 Compare October 7, 2024 15:55

brontolosone added 2 commits October 7, 2024 20:08

add centralized preference registration/validation/defaultization, an…

35b03d6

…d privatize some class members

remove superfluous check

d1b6cfa

See getodk#1024 (comment)

brontolosone requested a review from matthew-white October 7, 2024 20:17

brontolosone added 2 commits October 10, 2024 18:56

remove V8-specific stack trace fiddling

69bafe5

modularize normalizers for easy maintenance and mutation tracking

f3909a5

matthew-white reviewed Oct 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add user preference persistence framework #1024

Add user preference persistence framework #1024

brontolosone commented Sep 17, 2024

brontolosone Sep 24, 2024

matthew-white Sep 24, 2024

brontolosone Sep 24, 2024

matthew-white left a comment

matthew-white Sep 26, 2024

matthew-white Oct 25, 2024

matthew-white Oct 25, 2024

matthew-white Oct 25, 2024

matthew-white Oct 25, 2024

matthew-white Oct 25, 2024

matthew-white Oct 25, 2024

	createResource('session');
	const session = createResource('session');

Add user preference persistence framework #1024

Are you sure you want to change the base?

Add user preference persistence framework #1024

Conversation

brontolosone commented Sep 17, 2024

Implemented preferences:

Todo:

For later:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-white left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment