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.

Sign up for GitHub

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

Jump to bottom

Add user preference persistence framework #1024

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Add user preference persistence framework #1024

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
edb8b8e
Add user preference framework
brontolosone Aug 8, 2024
53d422e
add empty user preferences to user testdata
brontolosone Sep 19, 2024
58c09ad
account for potentially missing preferences
brontolosone Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 40 additions & 20 deletions src/components/form/trash-list.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,27 @@ except according to the terms contained in the LICENSE file.
-->
<template>
<div v-if="count > 0" id="form-trash-list">
<div id="form-trash-list-header">
<span id="form-trash-list-title">
<span class="icon-trash"></span>
<span>{{ $t('title') }}</span>
</span>
<span id="form-trash-list-count">{{ $t('trashCount', { count: $n(count, 'default') }) }}</span>
<span id="form-trash-list-note">{{ $t('message') }}</span>
</div>
<table id="form-trash-list-table" class="table">
<tbody>
<form-trash-row v-for="form of sortedDeletedForms" :key="form.id" :form="form"
@start-restore="restoreForm.show({ form: $event })"/>
</tbody>
</table>
<form-restore v-bind="restoreForm" @hide="restoreForm.hide()"
@success="afterRestore"/>
<details :open="!isFormTrashCollapsed" @toggle="toggleTrashExpansion">
<summary>
<div id="form-trash-list-header">
<span id="form-trash-list-title">
<span id="form-trash-expander" :class="{ 'icon-chevron-right': isFormTrashCollapsed, 'icon-chevron-down': !isFormTrashCollapsed }"></span>
<span class="icon-trash"></span>
<span>{{ $t('title') }}</span>
</span>
<span id="form-trash-list-count">{{ $t('trashCount', { count: $n(count, 'default') }) }}</span>
<span id="form-trash-list-note">{{ $t('message') }}</span>
</div>
</summary>
<table id="form-trash-list-table" class="table">
<tbody>
<form-trash-row v-for="form of sortedDeletedForms" :key="form.id" :form="form"
@start-restore="restoreForm.show({ form: $event })"/>
</tbody>
</table>
<form-restore v-bind="restoreForm" @hide="restoreForm.hide()"
@success="afterRestore"/>
</details>
</div>
</template>

Expand All @@ -49,8 +54,8 @@ export default {
setup() {
// The component does not assume that this data will exist when the
// component is created.
const { project, deletedForms } = useRequestData();
return { project, deletedForms, restoreForm: modalData() };
const { project, deletedForms, currentUser } = useRequestData();
return { project, deletedForms, currentUser, restoreForm: modalData() };
},
computed: {
count() {
Expand All @@ -59,7 +64,10 @@ export default {
sortedDeletedForms() {
const sortByDeletedAt = sortWith([ascend(entry => entry.deletedAt)]);
return sortByDeletedAt(this.deletedForms.data);
}
},
isFormTrashCollapsed() {
return (this.currentUser.dataExists && this.currentUser.preferences.projects[this.project.id].formTrashCollapsed);
Copy link
Member

Choose a reason for hiding this comment

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

currentUser is guaranteed to exist at this point, so I think the this.currentUser.dataExists condition can be removed.

Copy link
Member

Choose a reason for hiding this comment

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

Backend doesn't enforce that formTrashCollapsed is boolean, right? How about converting it to boolean here?

return this.currentUser.preferences.projects[this.project.id].formTrashCollapsed === true;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think a more thorough and consistent approach could be to have a module that defines possible keys (ticks the bill for having a central registry of what config keys are being used, thus without having to infer that list from grepping around), and their "readers" (that provide a default when data is absent, or invalid), and their validators. Just some functions, basically, called by the proxy object upon value read/write.
That would avoid having to write repetitive (and not necessarily consistent) defensive code in each and every usage site.
I'll get on it.

Copy link
Member

@matthew-white matthew-white Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That sounds quite nice to me! I think it's also something we could layer on in the future if it ends up feeling complicated to add now.

Copy link
Member

Choose a reason for hiding this comment

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

called by the proxy object upon value read/write

I also think it'd be OK if the proxy object only validated on read. If it's being written to by other code, that code should be doing its own validation. For example, the component ProjectSort will change the projectSortMode preference, but the component is limited to a set of <option>s. No strong preference by me, just wanted to throw that out as a potentially simplifying option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's say on write it just validates whether the key is declared in the registry (serves the purpose of having a registry, so we have some grip on what settings are in use, this will also prevent inadvertent collisions), and on read it does the validation where it returns a sensible default if a value doesn't exist or doesn't pass muster.

},
},
created() {
this.fetchDeletedForms(false);
Expand All @@ -82,7 +90,13 @@ export default {
// tell parent component (ProjectOverview) to refresh regular forms list
// (by emitting event to that component's parent)
this.$emit('restore');
}
},
toggleTrashExpansion(evt) {
const projProps = this.currentUser.preferences.projects[this.project.id];
if (evt.newState === 'closed') {
projProps.formTrashCollapsed = true;
} else if (projProps.formTrashCollapsed) delete projProps.formTrashCollapsed;
},
}
};
</script>
Expand All @@ -94,6 +108,12 @@ export default {
display: flex;
align-items: baseline;

#form-trash-expander {
// Fixate the width as icon-chevron-down and icon-chevron-right have unequal width :-(
display: inline-block;
width: 1em;
}

.icon-trash {
padding-right: 8px;
}
Expand Down
12 changes: 10 additions & 2 deletions src/components/project/list.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,15 @@ export default {
setup() {
const { currentUser, projects } = useRequestData();

const sortMode = ref('latest');
const sortMode = computed({
get() {
return currentUser.preferences.site.projectSortMode || 'latest';
Copy link
Member

Choose a reason for hiding this comment

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

If I understand the Backend correctly, it's possible for projectSortMode to be made something other than a string, right? The ProjectSort component will be expecting a string. What do you think about doing a type check here? Something like:

const { projectSortMode } = currentUser.preferences.site;
return typeof projectSortMode === 'string' ? projectSortMode : 'latest';

We could do even more validation, like checking that projectSortMode is one of 'alphabetical', 'latest', or 'newest'. But I don't think that's needed at this point given how the ProjectSort component uses its modelValue prop. I think the <select> element will just display the first <option> if it's passed a modelValue that isn't among the <option>s.

Copy link
Member

Choose a reason for hiding this comment

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

We could do even more validation. … But I don't think that's needed at this point given how the ProjectSort component uses its modelValue prop.

Ah I'm realizing that sortMode is also used to define sortFunction. If projectSortMode is some unknown/invalid string, then sortFunction.value will be undefined, which I'd guess would cause an error. Maybe sortFunction could also do some fallback?

const sortFunction = computed(() => sortFunctions[sortMode.value] ?? sortFunctions.latest);

},
set(val) {
currentUser.preferences.site.projectSortMode = val;
},
});
matthew-white marked this conversation as resolved.
Show resolved Hide resolved

const sortFunction = computed(() => sortFunctions[sortMode.value]);

const activeProjects = ref(null);
Expand Down Expand Up @@ -164,7 +172,7 @@ export default {
const message = this.$t('alert.create');
this.$router.push(this.projectPath(project.id))
.then(() => { this.alert.success(message); });
}
},
}
};
</script>
Expand Down
2 changes: 1 addition & 1 deletion src/request-data/resource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class BaseResource {
}
}

const _container = Symbol('container');
export const _container = Symbol('container');
const _abortController = Symbol('abortController');
class Resource extends BaseResource {
constructor(container, name, store) {
Expand Down
114 changes: 112 additions & 2 deletions src/request-data/resources.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,132 @@ https://www.apache.org/licenses/LICENSE-2.0. No part of ODK Central,
including this file, may be copied, modified, propagated, or distributed
except according to the terms contained in the LICENSE file.
*/
import { computed, reactive, shallowReactive, watchSyncEffect } from 'vue';
import { computed, reactive, shallowReactive, watchSyncEffect, isReactive } from 'vue';
import { mergeDeepLeft } from 'ramda';

import configDefaults from '../config';
import { computeIfExists, hasVerbs, setupOption, transformForm } from './util';
import { noargs } from '../util/util';
import { apiPaths, withAuth } from '../util/request';
import { _container } from './resource';

export default ({ i18n }, createResource) => {
// Resources related to the session
createResource('session');
createResource('currentUser', () => ({
createResource('currentUser', (self) => ({
/* eslint-disable no-param-reassign */
transformResponse: ({ data }) => {
data.verbs = new Set(data.verbs);
data.can = hasVerbs;
data.preferences = {
site: new Proxy(
shallowReactive(data.preferences.site),
{
deleteProperty(target, prop) {
const retval = (delete target[prop]);
self.preferenceOps.propagate(prop, null, null); // DELETE to backend
return retval;
},
set(target, prop, value) {
// eslint-disable-next-line no-multi-assign
const retval = (target[prop] = value);
self.preferenceOps.propagate(prop, value, null); // PUT to backend
return retval;
},
},
),
projects: new Proxy(
data.preferences.projects,
{
deleteProperty() {
throw new Error('Deleting a project\'s whole property collection is not supported. Delete each property individually, eg "delete preferences.projects[3].foo".');
},
set() {
throw new Error('Directly setting a project\'s whole property collection is not supported. Set each property individually, eg "preferences.projects[3].foo = \'bar\'"');
},
get(target, projectId) {
if (Number.isNaN(parseInt(projectId, 10))) throw new TypeError(`Not an integer project ID: "${projectId}"`);
const projectProps = target[projectId];
if (projectProps === undefined || (!isReactive(projectProps))) { // not reentrant (TOCTOU issue) but there's no real way to solve it — as this is supposed to be a synchronous method we can't simply wrap it in a Lock
target[projectId] = new Proxy(
// make (potentially autovivicated) props reactive, and front them with a proxy to enable our setters/deleters
shallowReactive(projectProps === undefined ? {} : projectProps),
{
deleteProperty(from, prop) {
const retval = (delete from[prop]);
self.preferenceOps.propagate(prop, null, projectId); // DELETE to backend
return retval;
},
set(from, prop, propval) {
// eslint-disable-next-line no-multi-assign
const retval = (from[prop] = propval);
self.preferenceOps.propagate(prop, propval, projectId); // PUT to backend
return retval;
},
}
);
}
return target[projectId];
},
}
),
};
return shallowReactive(data);
},
preferenceOps: {
self,
_container,
abortControllers: {},
instanceID: crypto.randomUUID(),
brontolosone marked this conversation as resolved.
Show resolved Hide resolved
propagate: (k, v, projectId) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

// As we need to be able to have multiple requests in-flight (not canceling eachother), we can't use resource.request() here.
// However, we want to avoid stacking requests for the same key, so we abort preceding requests for the same key, if any.
// Note that because locks are origin-scoped, we use a store instantiation identifier to scope them to this app instance.
const keyLockName = `userPreferences-${self.instanceID}-keystack-${projectId}-${k}`;
navigator.locks.request(
`userPreferences-${self.instanceID}-lockops`,
() => {
navigator.locks.request(
keyLockName,
{ ifAvailable: true },
(lockForKey) => {
const aborter = new AbortController();
if (!lockForKey) {
// Cancel the preceding request, a new one supersedes it.
self.preferenceOps.abortControllers[k].abort();
return navigator.locks.request(
keyLockName,
() => {
self.preferenceOps.abortControllers[k] = aborter;
return self.preferenceOps.request(k, v, projectId, aborter);
}
);
}
self.preferenceOps.abortControllers[k] = aborter;
return self.preferenceOps.request(k, v, projectId, aborter);
},
);
return Promise.resolve(); // return asap with a resolved promise so the outer lockops lock gets released; we don't wan't to wait here for the inner keylock-enveloped requests.
}
);
},
request: (k, v, projectId, aborter) => {
const { requestData, http } = self[self.preferenceOps._container];
return http.request(
withAuth(
{
method: (v === null) ? 'DELETE' : 'PUT',
url: (projectId === null) ? `${apiPaths.userSitePreferences(k)}` : `${apiPaths.userProjectPreferences(projectId, k)}`,
headers: {
'Content-Type': 'application/json',
},
data: (v === null) ? undefined : { propertyValue: v },
signal: aborter.signal,
},
requestData.session.token
)
);
},
}
/* eslint-enable no-param-reassign */
}));
Expand Down
4 changes: 3 additions & 1 deletion src/util/request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,9 @@ export const apiPaths = {
fieldKeys: projectPath('/app-users'),
serverUrlForFieldKey: (token, projectId) =>
`/v1/key/${token}/projects/${projectId}`,
audits: (query) => `/v1/audits${queryString(query)}`
audits: (query) => `/v1/audits${queryString(query)}`,
userSitePreferences: (k) => `/v1/user-preferences/site/${k}`,
userProjectPreferences: (projectId, k) => `/v1/user-preferences/project/${projectId}/${k}`,
};


Expand Down
9 changes: 7 additions & 2 deletions test/data/users.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@ export const extendedUsers = dataStore({
role = 'admin',
verbs = verbsByRole(role),
createdAt = undefined,
deletedAt = undefined
deletedAt = undefined,
preferences = {
site: {},
projects: {},
},
Comment on lines +28 to +31
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.

}) => ({
id,
type: 'user',
Expand All @@ -35,7 +39,8 @@ export const extendedUsers = dataStore({
? createdAt
: (inPast ? fakePastDate([lastCreatedAt]) : new Date().toISOString()),
updatedAt: null,
deletedAt
deletedAt,
preferences,
}),
sort: (administrator1, administrator2) =>
administrator1.email.localeCompare(administrator2.email)
Expand Down