From edb8b8e7c03b11b0ba032f1296ce2a7293d2dfa8 Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Thu, 8 Aug 2024 15:57:37 +0000 Subject: [PATCH 01/21] Add user preference framework MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part of the currentUser resource. Uses JS proxies to enable some — hopefully comfortable — abstractions for working with preferences. --- src/components/form/trash-list.vue | 60 ++++++++++----- src/components/project/list.vue | 12 ++- src/request-data/resource.js | 2 +- src/request-data/resources.js | 114 ++++++++++++++++++++++++++++- src/util/request.js | 4 +- 5 files changed, 166 insertions(+), 26 deletions(-) diff --git a/src/components/form/trash-list.vue b/src/components/form/trash-list.vue index 6d0793ccf..0850ee36f 100644 --- a/src/components/form/trash-list.vue +++ b/src/components/form/trash-list.vue @@ -11,22 +11,27 @@ except according to the terms contained in the LICENSE file. --> @@ -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() { @@ -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); + }, }, created() { this.fetchDeletedForms(false); @@ -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; + }, } }; @@ -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; } diff --git a/src/components/project/list.vue b/src/components/project/list.vue index fafc51c36..e12246224 100644 --- a/src/components/project/list.vue +++ b/src/components/project/list.vue @@ -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'; + }, + set(val) { + currentUser.preferences.site.projectSortMode = val; + }, + }); + const sortFunction = computed(() => sortFunctions[sortMode.value]); const activeProjects = ref(null); @@ -164,7 +172,7 @@ export default { const message = this.$t('alert.create'); this.$router.push(this.projectPath(project.id)) .then(() => { this.alert.success(message); }); - } + }, } }; diff --git a/src/request-data/resource.js b/src/request-data/resource.js index fcb37ccf9..dcc7f92d2 100644 --- a/src/request-data/resource.js +++ b/src/request-data/resource.js @@ -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) { diff --git a/src/request-data/resources.js b/src/request-data/resources.js index 1430acf53..4d750d49e 100644 --- a/src/request-data/resources.js +++ b/src/request-data/resources.js @@ -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(), + propagate: (k, v, projectId) => { + // 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 */ })); diff --git a/src/util/request.js b/src/util/request.js index 64db1f539..a0914c8b6 100644 --- a/src/util/request.js +++ b/src/util/request.js @@ -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}`, }; From 53d422e23feb249e886f4829a73762a66a9f95be Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Thu, 19 Sep 2024 09:49:13 +0000 Subject: [PATCH 02/21] add empty user preferences to user testdata --- test/data/users.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/data/users.js b/test/data/users.js index 52d321911..cb3e973ea 100644 --- a/test/data/users.js +++ b/test/data/users.js @@ -24,7 +24,11 @@ export const extendedUsers = dataStore({ role = 'admin', verbs = verbsByRole(role), createdAt = undefined, - deletedAt = undefined + deletedAt = undefined, + preferences = { + site: {}, + projects: {}, + }, }) => ({ id, type: 'user', @@ -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) From 58c09ad7675f5bef7b406b905df223a1e9e47ec7 Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Tue, 1 Oct 2024 12:52:19 +0000 Subject: [PATCH 03/21] account for potentially missing preferences See https://github.com/getodk/central-frontend/pull/1024\#pullrequestreview-2332522640 --- src/components/project/list.vue | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/components/project/list.vue b/src/components/project/list.vue index e12246224..c00524a88 100644 --- a/src/components/project/list.vue +++ b/src/components/project/list.vue @@ -97,7 +97,8 @@ export default { const sortMode = computed({ get() { - return currentUser.preferences.site.projectSortMode || 'latest'; + // currentUser.preferences goes missing on logout, see https://github.com/getodk/central-frontend/pull/1024#pullrequestreview-2332522640 + return currentUser.preferences?.site?.projectSortMode || 'latest'; }, set(val) { currentUser.preferences.site.projectSortMode = val; From 4bb37a615e81fd7d96f2e26203fbd1ff615cb95f Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Mon, 7 Oct 2024 15:08:44 +0000 Subject: [PATCH 04/21] factor out UserPreferences --- src/request-data/resources.js | 115 +------------------------ src/request-data/user-preferences.js | 124 +++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 111 deletions(-) create mode 100644 src/request-data/user-preferences.js diff --git a/src/request-data/resources.js b/src/request-data/resources.js index 4d750d49e..acc04084e 100644 --- a/src/request-data/resources.js +++ b/src/request-data/resources.js @@ -9,14 +9,14 @@ 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, isReactive } from 'vue'; +import { computed, reactive, shallowReactive, watchSyncEffect } 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'; +import UserPreferences from './user-preferences'; export default ({ i18n }, createResource) => { // Resources related to the session @@ -26,117 +26,10 @@ export default ({ i18n }, createResource) => { 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]; - }, - } - ), - }; + const { requestData, http } = self[_container]; + data.preferences = new UserPreferences(data.preferences, requestData.session, http); return shallowReactive(data); - }, - preferenceOps: { - self, - _container, - abortControllers: {}, - instanceID: crypto.randomUUID(), - propagate: (k, v, projectId) => { - // 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 */ })); // Resources related to the system diff --git a/src/request-data/user-preferences.js b/src/request-data/user-preferences.js new file mode 100644 index 000000000..01ce43044 --- /dev/null +++ b/src/request-data/user-preferences.js @@ -0,0 +1,124 @@ +/* eslint-disable no-param-reassign */ +import { shallowReactive, isReactive } from 'vue'; +import { apiPaths, withAuth } from '../util/request'; + + +export default class UserPreferences { + constructor(preferenceData, session, http) { + this.abortControllers = {}; + this.instanceID = crypto.randomUUID(); + this.site = this.makeSiteProxy(preferenceData.site); + this.projects = this.makeProjectsProxy(preferenceData.projects); + this.session = session; + this.http = http; + } + + propagate(k, v, projectId) { + // 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-${this.instanceID}-keystack-${projectId}-${k}`; + navigator.locks.request( + `userPreferences-${this.instanceID}-lockops`, + () => { + navigator.locks.request( + keyLockName, + { ifAvailable: true }, + (lockForKey) => { + const aborter = new AbortController(); + if (!lockForKey) { + // Cancel the preceding request, a new one supersedes it. + this.abortControllers[k].abort(); + return navigator.locks.request( + keyLockName, + () => { + this.abortControllers[k] = aborter; + return this.request(k, v, projectId, aborter); + } + ); + } + this.abortControllers[k] = aborter; + return this.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) { + return this.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, + }, + this.session.token + ) + ); + } + + makeSiteProxy(sitePreferenceData) { + const userPreferences = this; + return new Proxy( + shallowReactive(sitePreferenceData), + { + deleteProperty(target, prop) { + const retval = (delete target[prop]); + userPreferences.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); + userPreferences.propagate(prop, value, null); // PUT to backend + return retval; + }, + } + ); + } + + makeProjectsProxy(projectsPreferenceData) { + const userPreferences = this; + return new Proxy( + projectsPreferenceData, + { + 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]); + userPreferences.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); + userPreferences.propagate(prop, propval, projectId); // PUT to backend + return retval; + }, + } + ); + } + return target[projectId]; + }, + } + ); + } +} From 35b03d61dcc12866d56aebc2aa9b05a762a201cb Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Mon, 7 Oct 2024 20:08:28 +0000 Subject: [PATCH 05/21] add centralized preference registration/validation/defaultization, and privatize some class members --- src/components/project/list.vue | 2 +- src/request-data/user-preferences.js | 138 ++++++++++++++++++++++----- 2 files changed, 114 insertions(+), 26 deletions(-) diff --git a/src/components/project/list.vue b/src/components/project/list.vue index 5181c6d8b..95b69db16 100644 --- a/src/components/project/list.vue +++ b/src/components/project/list.vue @@ -98,7 +98,7 @@ export default { const sortMode = computed({ get() { // currentUser.preferences goes missing on logout, see https://github.com/getodk/central-frontend/pull/1024#pullrequestreview-2332522640 - return currentUser.preferences?.site?.projectSortMode || 'latest'; + return currentUser.preferences?.site?.projectSortMode; }, set(val) { currentUser.preferences.site.projectSortMode = val; diff --git a/src/request-data/user-preferences.js b/src/request-data/user-preferences.js index 01ce43044..690650225 100644 --- a/src/request-data/user-preferences.js +++ b/src/request-data/user-preferences.js @@ -3,21 +3,99 @@ import { shallowReactive, isReactive } from 'vue'; import { apiPaths, withAuth } from '../util/request'; +// The SitePreferenceNormalizer and ProjectPreferenceNormalizer classes are used to: +// a) verify that the preference key has been declared here. +// Such might seem persnickety, but it allows us to have a central +// registry of which keys are in use. +// b) normalize the value as per the normalization function with the name +// of the preference. This also allows supplying a default. +// Preferences serverside may have been created by some frontend version that +// used different semantics (different values, perhaps differently typed). +// Writing a validator function here makes it so one does not have to be defensive +// for that eventuality in *every single usage site of the setting*. +// +// As such, any newly introduced preference will need a normalization function added +// to one of those classes, even if it's just a straight passthrough. +// Furthermore, the answer to "why can't I set an arbitrary value for a certain preference" +// can be found there. + + +const VUE_PROPERTY_PREFIX = '__v_'; // Empirically established. I couldn't find documentation on it. + + +class PreferenceNotRegisteredError extends Error { + constructor(prop, whatclass, ...params) { + super(...params); + if (Error.captureStackTrace) { + // on V8, we can have the stack trace start from where the error was thrown + Error.captureStackTrace(this, PreferenceNotRegisteredError); + } + this.name = 'PreferencesNotRegisteredError'; + this.message = `Property "${prop}" has not been registered in ${whatclass.name}`; + } +} + + +class PreferenceNormalizer { + static _normalize(target, prop, val) { + const normalizer = this.normalizeFn(prop); + const theVal = (target === undefined ? val : target[prop]); + return normalizer(theVal); + } + + static normalizeFn(prop) { + const normalizer = Object.prototype.hasOwnProperty.call(this, prop) ? this[prop] : undefined; + if (normalizer !== undefined) return normalizer; + throw new PreferenceNotRegisteredError(prop, this); + } + + static normalize(prop, val) { + return this._normalize(undefined, prop, val); + } + + static getProp(target, prop) { + if (typeof (prop) === 'string' && !prop.startsWith(VUE_PROPERTY_PREFIX)) { + return this._normalize(target, prop); + } + return target[prop]; + } +} + + +class SitePreferenceNormalizer extends PreferenceNormalizer { + static projectSortMode(val) { + return ['alphabetical', 'latest', 'newest'].includes(val) ? val : 'latest'; + } +} + + +class ProjectPreferenceNormalizer extends PreferenceNormalizer { + static formTrashCollapsed(val) { + return Boolean(val); + } +} + + export default class UserPreferences { + #abortControllers; + #instanceID; + #session; + #http; + constructor(preferenceData, session, http) { - this.abortControllers = {}; - this.instanceID = crypto.randomUUID(); - this.site = this.makeSiteProxy(preferenceData.site); - this.projects = this.makeProjectsProxy(preferenceData.projects); - this.session = session; - this.http = http; + this.#abortControllers = {}; + this.#instanceID = crypto.randomUUID(); + this.site = this.#makeSiteProxy(preferenceData.site); + this.projects = this.#makeProjectsProxy(preferenceData.projects); + this.#session = session; + this.#http = http; } - propagate(k, v, projectId) { + #propagate(k, v, projectId) { // 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-${this.instanceID}-keystack-${projectId}-${k}`; + const keyLockName = `userPreferences-${this.#instanceID}-keystack-${projectId}-${k}`; navigator.locks.request( `userPreferences-${this.instanceID}-lockops`, () => { @@ -27,18 +105,18 @@ export default class UserPreferences { (lockForKey) => { const aborter = new AbortController(); if (!lockForKey) { - // Cancel the preceding request, a new one supersedes it. - this.abortControllers[k].abort(); + // Cancel the preceding HTTP request, a new one supersedes it. + this.#abortControllers[k].abort(); return navigator.locks.request( keyLockName, () => { - this.abortControllers[k] = aborter; - return this.request(k, v, projectId, aborter); + this.#abortControllers[k] = aborter; + return this.#request(k, v, projectId, aborter); } ); } - this.abortControllers[k] = aborter; - return this.request(k, v, projectId, aborter); + this.#abortControllers[k] = aborter; + return this.#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. @@ -46,8 +124,8 @@ export default class UserPreferences { ); } - request(k, v, projectId, aborter) { - return this.http.request( + #request(k, v, projectId, aborter) { + return this.#http.request( withAuth( { method: (v === null) ? 'DELETE' : 'PUT', @@ -58,32 +136,37 @@ export default class UserPreferences { data: (v === null) ? undefined : { propertyValue: v }, signal: aborter.signal, }, - this.session.token + this.#session.token ) ); } - makeSiteProxy(sitePreferenceData) { + #makeSiteProxy(sitePreferenceData) { const userPreferences = this; return new Proxy( shallowReactive(sitePreferenceData), { deleteProperty(target, prop) { + SitePreferenceNormalizer.normalizeFn(prop); // throws if prop is not registered const retval = (delete target[prop]); - userPreferences.propagate(prop, null, null); // DELETE to backend + userPreferences.#propagate(prop, null, null); // DELETE to backend return retval; }, set(target, prop, value) { + const normalizedValue = SitePreferenceNormalizer.normalize(prop, value); // eslint-disable-next-line no-multi-assign - const retval = (target[prop] = value); - userPreferences.propagate(prop, value, null); // PUT to backend + const retval = (target[prop] = normalizedValue); + userPreferences.#propagate(prop, normalizedValue, null); // PUT to backend return retval; }, + get(target, prop) { + return SitePreferenceNormalizer.getProp(target, prop); + } } ); } - makeProjectsProxy(projectsPreferenceData) { + #makeProjectsProxy(projectsPreferenceData) { const userPreferences = this; return new Proxy( projectsPreferenceData, @@ -103,16 +186,21 @@ export default class UserPreferences { shallowReactive(projectProps === undefined ? {} : projectProps), { deleteProperty(from, prop) { + ProjectPreferenceNormalizer.normalizeFn(prop); // throws if prop is not registered const retval = (delete from[prop]); - userPreferences.propagate(prop, null, projectId); // DELETE to backend + userPreferences.#propagate(prop, null, projectId); // DELETE to backend return retval; }, set(from, prop, propval) { + const normalizedValue = ProjectPreferenceNormalizer.normalize(prop, propval); // eslint-disable-next-line no-multi-assign - const retval = (from[prop] = propval); - userPreferences.propagate(prop, propval, projectId); // PUT to backend + const retval = (from[prop] = normalizedValue); + userPreferences.#propagate(prop, normalizedValue, projectId); // PUT to backend return retval; }, + get(projectTarget, prop) { + return ProjectPreferenceNormalizer.getProp(projectTarget, prop); + }, } ); } From d1b6cfa81070da252c509bc7cc41e2e9d3e03816 Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Mon, 7 Oct 2024 20:10:28 +0000 Subject: [PATCH 06/21] remove superfluous check See https://github.com/getodk/central-frontend/pull/1024#discussion_r1777131736 --- src/components/form/trash-list.vue | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/form/trash-list.vue b/src/components/form/trash-list.vue index 0850ee36f..deaf297ae 100644 --- a/src/components/form/trash-list.vue +++ b/src/components/form/trash-list.vue @@ -66,7 +66,7 @@ export default { return sortByDeletedAt(this.deletedForms.data); }, isFormTrashCollapsed() { - return (this.currentUser.dataExists && this.currentUser.preferences.projects[this.project.id].formTrashCollapsed); + return this.currentUser.preferences.projects[this.project.id].formTrashCollapsed; }, }, created() { From 69bafe5bfce5a82bc77143f0bf2866e55dda1bd7 Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Thu, 10 Oct 2024 18:56:24 +0000 Subject: [PATCH 07/21] remove V8-specific stack trace fiddling --- src/request-data/user-preferences.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/request-data/user-preferences.js b/src/request-data/user-preferences.js index 690650225..c0ff36736 100644 --- a/src/request-data/user-preferences.js +++ b/src/request-data/user-preferences.js @@ -26,10 +26,6 @@ const VUE_PROPERTY_PREFIX = '__v_'; // Empirically established. I couldn't find class PreferenceNotRegisteredError extends Error { constructor(prop, whatclass, ...params) { super(...params); - if (Error.captureStackTrace) { - // on V8, we can have the stack trace start from where the error was thrown - Error.captureStackTrace(this, PreferenceNotRegisteredError); - } this.name = 'PreferencesNotRegisteredError'; this.message = `Property "${prop}" has not been registered in ${whatclass.name}`; } From 0f8404bb2bf402739f816218b085ced5a63a75fc Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Thu, 10 Oct 2024 18:54:05 +0000 Subject: [PATCH 08/21] modularize normalizers for easy maintenance and mutation tracking --- src/request-data/resources.js | 2 +- .../user-preferences/normalizer.js | 36 ++++++++++ .../user-preferences/normalizers.js | 30 ++++++++ .../preferences.js} | 72 +------------------ 4 files changed, 69 insertions(+), 71 deletions(-) create mode 100644 src/request-data/user-preferences/normalizer.js create mode 100644 src/request-data/user-preferences/normalizers.js rename src/request-data/{user-preferences.js => user-preferences/preferences.js} (69%) diff --git a/src/request-data/resources.js b/src/request-data/resources.js index acc04084e..7982f0bc9 100644 --- a/src/request-data/resources.js +++ b/src/request-data/resources.js @@ -16,7 +16,7 @@ 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'; +import UserPreferences from './user-preferences/preferences'; export default ({ i18n }, createResource) => { // Resources related to the session diff --git a/src/request-data/user-preferences/normalizer.js b/src/request-data/user-preferences/normalizer.js new file mode 100644 index 000000000..44c54df30 --- /dev/null +++ b/src/request-data/user-preferences/normalizer.js @@ -0,0 +1,36 @@ +const VUE_PROPERTY_PREFIX = '__v_'; // Empirically established. I couldn't find documentation on it. + + +class PreferenceNotRegisteredError extends Error { + constructor(prop, whatclass, ...params) { + super(...params); + this.name = 'PreferencesNotRegisteredError'; + this.message = `Property "${prop}" has not been registered in ${whatclass.name}`; + } +} + + +export default class PreferenceNormalizer { + static _normalize(target, prop, val) { + const normalizer = this.normalizeFn(prop); + const theVal = (target === undefined ? val : target[prop]); + return normalizer(theVal); + } + + static normalizeFn(prop) { + const normalizer = Object.prototype.hasOwnProperty.call(this, prop) ? this[prop] : undefined; + if (normalizer !== undefined) return normalizer; + throw new PreferenceNotRegisteredError(prop, this); + } + + static normalize(prop, val) { + return this._normalize(undefined, prop, val); + } + + static getProp(target, prop) { + if (typeof (prop) === 'string' && !prop.startsWith(VUE_PROPERTY_PREFIX)) { + return this._normalize(target, prop); + } + return target[prop]; + } +} diff --git a/src/request-data/user-preferences/normalizers.js b/src/request-data/user-preferences/normalizers.js new file mode 100644 index 000000000..64f33998e --- /dev/null +++ b/src/request-data/user-preferences/normalizers.js @@ -0,0 +1,30 @@ +import PreferenceNormalizer from './normalizer'; + +// The SitePreferenceNormalizer and ProjectPreferenceNormalizer classes are used to: +// a) verify that the preference key has been declared here. +// Such might seem persnickety, but it allows us to have a central +// registry of which keys are in use. +// b) normalize the value as per the normalization function with the name +// of the preference. This also allows supplying a default. +// Preferences serverside may have been created by some frontend version that +// used different semantics (different values, perhaps differently typed). +// Writing a validator function here makes it so one does not have to be defensive +// for that eventuality in *every single usage site of the setting*. +// +// As such, any newly introduced preference will need a normalization function added +// to one of those classes, even if it's just a straight passthrough. +// Furthermore, the answer to "why can't I set an arbitrary value for a certain preference" +// can be found there. + + +export class SitePreferenceNormalizer extends PreferenceNormalizer { + static projectSortMode(val) { + return ['alphabetical', 'latest', 'newest'].includes(val) ? val : 'latest'; + } +} + +export class ProjectPreferenceNormalizer extends PreferenceNormalizer { + static formTrashCollapsed(val) { + return Boolean(val); + } +} diff --git a/src/request-data/user-preferences.js b/src/request-data/user-preferences/preferences.js similarity index 69% rename from src/request-data/user-preferences.js rename to src/request-data/user-preferences/preferences.js index c0ff36736..440aeba9e 100644 --- a/src/request-data/user-preferences.js +++ b/src/request-data/user-preferences/preferences.js @@ -1,75 +1,7 @@ /* eslint-disable no-param-reassign */ import { shallowReactive, isReactive } from 'vue'; -import { apiPaths, withAuth } from '../util/request'; - - -// The SitePreferenceNormalizer and ProjectPreferenceNormalizer classes are used to: -// a) verify that the preference key has been declared here. -// Such might seem persnickety, but it allows us to have a central -// registry of which keys are in use. -// b) normalize the value as per the normalization function with the name -// of the preference. This also allows supplying a default. -// Preferences serverside may have been created by some frontend version that -// used different semantics (different values, perhaps differently typed). -// Writing a validator function here makes it so one does not have to be defensive -// for that eventuality in *every single usage site of the setting*. -// -// As such, any newly introduced preference will need a normalization function added -// to one of those classes, even if it's just a straight passthrough. -// Furthermore, the answer to "why can't I set an arbitrary value for a certain preference" -// can be found there. - - -const VUE_PROPERTY_PREFIX = '__v_'; // Empirically established. I couldn't find documentation on it. - - -class PreferenceNotRegisteredError extends Error { - constructor(prop, whatclass, ...params) { - super(...params); - this.name = 'PreferencesNotRegisteredError'; - this.message = `Property "${prop}" has not been registered in ${whatclass.name}`; - } -} - - -class PreferenceNormalizer { - static _normalize(target, prop, val) { - const normalizer = this.normalizeFn(prop); - const theVal = (target === undefined ? val : target[prop]); - return normalizer(theVal); - } - - static normalizeFn(prop) { - const normalizer = Object.prototype.hasOwnProperty.call(this, prop) ? this[prop] : undefined; - if (normalizer !== undefined) return normalizer; - throw new PreferenceNotRegisteredError(prop, this); - } - - static normalize(prop, val) { - return this._normalize(undefined, prop, val); - } - - static getProp(target, prop) { - if (typeof (prop) === 'string' && !prop.startsWith(VUE_PROPERTY_PREFIX)) { - return this._normalize(target, prop); - } - return target[prop]; - } -} - - -class SitePreferenceNormalizer extends PreferenceNormalizer { - static projectSortMode(val) { - return ['alphabetical', 'latest', 'newest'].includes(val) ? val : 'latest'; - } -} - - -class ProjectPreferenceNormalizer extends PreferenceNormalizer { - static formTrashCollapsed(val) { - return Boolean(val); - } -} +import { apiPaths, withAuth } from '../../util/request'; +import { SitePreferenceNormalizer, ProjectPreferenceNormalizer } from './normalizers'; export default class UserPreferences { From 2022da7791aaf1921d00febe9a41af6bff4855c2 Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Mon, 28 Oct 2024 12:18:40 +0000 Subject: [PATCH 09/21] post-PR fixups & cleanups --- src/request-data/resources.js | 13 ++++++------- src/request-data/user-preferences/preferences.js | 5 ++++- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/request-data/resources.js b/src/request-data/resources.js index 7982f0bc9..b77a3c3ba 100644 --- a/src/request-data/resources.js +++ b/src/request-data/resources.js @@ -15,19 +15,18 @@ 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) => { +export default ({ i18n, http }, createResource) => { // Resources related to the session - createResource('session'); - createResource('currentUser', (self) => ({ - /* eslint-disable no-param-reassign */ + const session = createResource('session'); + createResource('currentUser', () => ({ transformResponse: ({ data }) => { + /* eslint-disable no-param-reassign */ data.verbs = new Set(data.verbs); data.can = hasVerbs; - const { requestData, http } = self[_container]; - data.preferences = new UserPreferences(data.preferences, requestData.session, http); + data.preferences = new UserPreferences(data.preferences, session, http); + /* eslint-enable no-param-reassign */ return shallowReactive(data); } })); diff --git a/src/request-data/user-preferences/preferences.js b/src/request-data/user-preferences/preferences.js index 440aeba9e..3c2b1b063 100644 --- a/src/request-data/user-preferences/preferences.js +++ b/src/request-data/user-preferences/preferences.js @@ -1,4 +1,3 @@ -/* eslint-disable no-param-reassign */ import { shallowReactive, isReactive } from 'vue'; import { apiPaths, withAuth } from '../../util/request'; import { SitePreferenceNormalizer, ProjectPreferenceNormalizer } from './normalizers'; @@ -74,6 +73,7 @@ export default class UserPreferences { return new Proxy( shallowReactive(sitePreferenceData), { + /* eslint-disable no-param-reassign */ deleteProperty(target, prop) { SitePreferenceNormalizer.normalizeFn(prop); // throws if prop is not registered const retval = (delete target[prop]); @@ -87,6 +87,7 @@ export default class UserPreferences { userPreferences.#propagate(prop, normalizedValue, null); // PUT to backend return retval; }, + /* eslint-enable no-param-reassign */ get(target, prop) { return SitePreferenceNormalizer.getProp(target, prop); } @@ -109,6 +110,7 @@ export default class UserPreferences { 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 + /* eslint-disable no-param-reassign */ target[projectId] = new Proxy( // make (potentially autovivicated) props reactive, and front them with a proxy to enable our setters/deleters shallowReactive(projectProps === undefined ? {} : projectProps), @@ -131,6 +133,7 @@ export default class UserPreferences { }, } ); + /* eslint-enable no-param-reassign */ } return target[projectId]; }, From ed97d9e38e9fa05cf22c3da23facf57ff38ed965 Mon Sep 17 00:00:00 2001 From: Matthew White Date: Tue, 29 Oct 2024 00:24:05 -0400 Subject: [PATCH 10/21] Omit preferences from testData.standardUsers --- test/data/users.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/data/users.js b/test/data/users.js index cb3e973ea..194f5d367 100644 --- a/test/data/users.js +++ b/test/data/users.js @@ -46,4 +46,7 @@ export const extendedUsers = dataStore({ administrator1.email.localeCompare(administrator2.email) }); -export const standardUsers = view(extendedUsers, omit(['verbs'])); +export const standardUsers = view( + extendedUsers, + omit(['verbs', 'preferences']) +); From 24b1a3d77bc1c817c51dde8462ef960042623c14 Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Wed, 30 Oct 2024 11:28:41 +0000 Subject: [PATCH 11/21] post-PR fixups & cleanups 2 --- src/components/form/trash-list.vue | 2 +- src/request-data/resource.js | 2 +- src/request-data/user-preferences/normalizers.js | 2 +- src/request-data/user-preferences/preferences.js | 9 ++++----- 4 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/components/form/trash-list.vue b/src/components/form/trash-list.vue index deaf297ae..62b05e1cb 100644 --- a/src/components/form/trash-list.vue +++ b/src/components/form/trash-list.vue @@ -95,7 +95,7 @@ export default { const projProps = this.currentUser.preferences.projects[this.project.id]; if (evt.newState === 'closed') { projProps.formTrashCollapsed = true; - } else if (projProps.formTrashCollapsed) delete projProps.formTrashCollapsed; + } else if (projProps.formTrashCollapsed) projProps.formTrashCollapsed = false; }, } }; diff --git a/src/request-data/resource.js b/src/request-data/resource.js index dcc7f92d2..fcb37ccf9 100644 --- a/src/request-data/resource.js +++ b/src/request-data/resource.js @@ -51,7 +51,7 @@ class BaseResource { } } -export const _container = Symbol('container'); +const _container = Symbol('container'); const _abortController = Symbol('abortController'); class Resource extends BaseResource { constructor(container, name, store) { diff --git a/src/request-data/user-preferences/normalizers.js b/src/request-data/user-preferences/normalizers.js index 64f33998e..5e69b60fc 100644 --- a/src/request-data/user-preferences/normalizers.js +++ b/src/request-data/user-preferences/normalizers.js @@ -25,6 +25,6 @@ export class SitePreferenceNormalizer extends PreferenceNormalizer { export class ProjectPreferenceNormalizer extends PreferenceNormalizer { static formTrashCollapsed(val) { - return Boolean(val); + return val === true; } } diff --git a/src/request-data/user-preferences/preferences.js b/src/request-data/user-preferences/preferences.js index 3c2b1b063..d8a938194 100644 --- a/src/request-data/user-preferences/preferences.js +++ b/src/request-data/user-preferences/preferences.js @@ -109,24 +109,23 @@ export default class UserPreferences { 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 + if (projectProps === undefined || (!isReactive(projectProps))) { /* eslint-disable no-param-reassign */ 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) { - ProjectPreferenceNormalizer.normalizeFn(prop); // throws if prop is not registered + ProjectPreferenceNormalizer.normalizeFn(prop); // we're calling it just so that it throws if prop is not registered in the form of a normalization function const retval = (delete from[prop]); userPreferences.#propagate(prop, null, projectId); // DELETE to backend return retval; }, set(from, prop, propval) { const normalizedValue = ProjectPreferenceNormalizer.normalize(prop, propval); - // eslint-disable-next-line no-multi-assign - const retval = (from[prop] = normalizedValue); + from[prop] = normalizedValue; userPreferences.#propagate(prop, normalizedValue, projectId); // PUT to backend - return retval; + return true; }, get(projectTarget, prop) { return ProjectPreferenceNormalizer.getProp(projectTarget, prop); From 781068f24242195a006a4dc376f3dfeb1430e896 Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Thu, 31 Oct 2024 09:57:01 +0000 Subject: [PATCH 12/21] naming --- src/components/form/trash-list.vue | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/form/trash-list.vue b/src/components/form/trash-list.vue index 62b05e1cb..fd46dec67 100644 --- a/src/components/form/trash-list.vue +++ b/src/components/form/trash-list.vue @@ -11,7 +11,7 @@ except according to the terms contained in the LICENSE file. -->