From 9351bf290d848a6631b98c1793b62d17860fba45 Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Wed, 9 Oct 2024 10:20:34 +0000 Subject: [PATCH] Persist and serve user preferences (#1184) --- lib/http/service.js | 1 + lib/model/container.js | 3 +- .../20241008-01-add-user_preferences.js | 38 ++++ lib/model/query/user-preferences.js | 123 ++++++++++++ lib/resources/user-preferences.js | 61 ++++++ lib/resources/users.js | 5 +- lib/util/problem.js | 3 + test/integration/api/user-preferences.js | 182 ++++++++++++++++++ 8 files changed, 413 insertions(+), 3 deletions(-) create mode 100644 lib/model/migrations/20241008-01-add-user_preferences.js create mode 100644 lib/model/query/user-preferences.js create mode 100644 lib/resources/user-preferences.js create mode 100644 test/integration/api/user-preferences.js diff --git a/lib/http/service.js b/lib/http/service.js index 8a6256c7c..033246d3d 100644 --- a/lib/http/service.js +++ b/lib/http/service.js @@ -90,6 +90,7 @@ module.exports = (container) => { require('../resources/datasets')(service, endpoint); require('../resources/entities')(service, endpoint); require('../resources/oidc')(service, endpoint); + require('../resources/user-preferences')(service, endpoint); //////////////////////////////////////////////////////////////////////////////// // POSTRESOURCE HANDLERS diff --git a/lib/model/container.js b/lib/model/container.js index 305e7cac9..2b9e23af1 100644 --- a/lib/model/container.js +++ b/lib/model/container.js @@ -110,7 +110,8 @@ const withDefaults = (base, queries) => { SubmissionAttachments: require('./query/submission-attachments'), Users: require('./query/users'), Datasets: require('./query/datasets'), - Entities: require('./query/entities') + Entities: require('./query/entities'), + UserPreferences: require('./query/user-preferences') }; return injector(base, mergeRight(defaultQueries, queries)); diff --git a/lib/model/migrations/20241008-01-add-user_preferences.js b/lib/model/migrations/20241008-01-add-user_preferences.js new file mode 100644 index 000000000..94862cfba --- /dev/null +++ b/lib/model/migrations/20241008-01-add-user_preferences.js @@ -0,0 +1,38 @@ +// Copyright 2024 ODK Central Developers +// See the NOTICE file at the top-level directory of this distribution and at +// https://github.com/getodk/central-backend/blob/master/NOTICE. +// This file is part of ODK Central. It is subject to the license terms in +// the LICENSE file found in the top-level directory of this distribution and at +// 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. + +const up = (db) => db.raw(` + CREATE TABLE user_site_preferences ( + "userId" integer NOT NULL REFERENCES users ("actorId"), + "propertyName" text NOT NULL CHECK (length("propertyName") > 0), + "propertyValue" jsonb NOT NULL, + CONSTRAINT "user_site_preferences_primary_key" PRIMARY KEY ("userId", "propertyName") + ); + + CREATE TABLE user_project_preferences ( + "userId" integer NOT NULL REFERENCES users ("actorId"), + "projectId" integer NOT NULL REFERENCES projects ("id"), + "propertyName" text NOT NULL CHECK (length("propertyName") > 0), + "propertyValue" jsonb NOT NULL, + CONSTRAINT "user_project_preferences_primary_key" PRIMARY KEY ("userId", "projectId", "propertyName") + ); + + -- Primary key indices are used for PUTing/DELETE-ing individual rows — but the below indices are + -- used when aggregating all of a user's preferences. + CREATE INDEX ON "user_site_preferences" ("userId"); + CREATE INDEX ON "user_project_preferences" ("userId"); +`); + +const down = (db) => db.raw(` + DROP TABLE user_site_preferences; + DROP TABLE user_project_preferences; +`); + +module.exports = { up, down }; + diff --git a/lib/model/query/user-preferences.js b/lib/model/query/user-preferences.js new file mode 100644 index 000000000..234662a21 --- /dev/null +++ b/lib/model/query/user-preferences.js @@ -0,0 +1,123 @@ +// Copyright 2024 ODK Central Developers +// See the NOTICE file at the top-level directory of this distribution and at +// https://github.com/getodk/central-backend/blob/master/NOTICE. +// This file is part of ODK Central. It is subject to the license terms in +// the LICENSE file found in the top-level directory of this distribution and at +// 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. + +const { sql } = require('slonik'); + + +const getForUser = (userId) => ({ one }) => + one(sql` + SELECT + ( + SELECT + jsonb_build_object( + 'projects', + coalesce( + jsonb_object_agg( + projprops."projectId", + projprops.props + ), + jsonb_build_object() + ) + ) + FROM + ( + SELECT + "projectId", + jsonb_object_agg("propertyName", "propertyValue") AS props + FROM + user_project_preferences + WHERE + "userId" = ${userId} + GROUP BY + "projectId" + ) AS projprops + ) + || + ( + SELECT + jsonb_build_object( + 'site', + coalesce( + jsonb_object_agg( + user_site_preferences."propertyName", + user_site_preferences."propertyValue" + ), + jsonb_build_object() + ) + ) + FROM + user_site_preferences + WHERE + "userId" = ${userId} + ) + AS preferences + `); + + +const _writeProperty = (tablename, subject, userId, propertyName, propertyValue) => ({ one }) => { + const targetColumns = ['userId', 'propertyName', 'propertyValue'] + .concat((subject === null) ? [] : ['projectId']) + .map(el => sql.identifier([el])); + + // Work around null confusion (potential Slonik bug?). + // sql.json(null) doesn't produce what we need, it results in an exception + // "Error: Required parameter propertyValue missing." + // Yet the string 'null' (as distinct from the *jsonb* string '"null"' one would get with sql.json('null') !) + // gets properly casted by PostgreSQL to a jsonb null (as distinct from an SQL NULL), so we use that in this case. + const preparedPropertyValue = (propertyValue === null) ? 'null': sql.json(propertyValue); + const values = [userId, propertyName, preparedPropertyValue] + .concat((subject === null) ? [] : [subject]); + + return one(sql` + INSERT INTO ${sql.identifier([tablename])} + (${sql.join(targetColumns, `, `)}) + VALUES + (${sql.join(values, `, `)}) + ON CONFLICT ON CONSTRAINT ${sql.identifier([`${tablename}_primary_key`])} + DO UPDATE + SET "propertyValue" = ${preparedPropertyValue} + RETURNING + 1 AS "modified_count" + `); +}; + + +const _removeProperty = (tablename, subject, userId, propertyName) => ({ maybeOne }) => { + const targetColumns = ['userId', 'propertyName'] + .concat((subject === null) ? [] : ['projectId']) + .map(el => sql.identifier([el])); + + const values = [userId, propertyName] + .concat((subject === null) ? [] : [subject]); + + return maybeOne(sql` + DELETE FROM ${sql.identifier([tablename])} + WHERE + (${sql.join(targetColumns, `, `)}) + = + (${sql.join(values, `, `)}) + RETURNING + 1 AS "deleted_count" + `); +}; + + +const writeSiteProperty = (userId, propertyName, propertyValue) => ({ one }) => + _writeProperty('user_site_preferences', null, userId, propertyName, propertyValue)({ one }); + +const removeSiteProperty = (userId, propertyName) => ({ maybeOne }) => + _removeProperty('user_site_preferences', null, userId, propertyName)({ maybeOne }); + +const writeProjectProperty = (userId, projectId, propertyName, propertyValue) => ({ one }) => + _writeProperty('user_project_preferences', projectId, userId, propertyName, propertyValue)({ one }); + +const removeProjectProperty = (userId, projectId, propertyName) => ({ maybeOne }) => + _removeProperty('user_project_preferences', projectId, userId, propertyName)({ maybeOne }); + +module.exports = { removeSiteProperty, writeSiteProperty, writeProjectProperty, removeProjectProperty, getForUser }; diff --git a/lib/resources/user-preferences.js b/lib/resources/user-preferences.js new file mode 100644 index 000000000..4d804ddab --- /dev/null +++ b/lib/resources/user-preferences.js @@ -0,0 +1,61 @@ +// Copyright 2024 ODK Central Developers +// See the NOTICE file at the top-level directory of this distribution and at +// https://github.com/getodk/central-backend/blob/master/NOTICE. +// This file is part of ODK Central. It is subject to the license terms in +// the LICENSE file found in the top-level directory of this distribution and at +// 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. + +const Problem = require('../util/problem'); +const { getOrNotFound } = require('../util/promise'); +const { success } = require('../util/http'); + +const checkBody = (body) => { + // Expects a body of {"propertyValue": X}, where X will go into the propertyValue column. + const bodyKeys = Object.keys(body); + if (!bodyKeys.includes('propertyValue')) throw Problem.user.propertyNotFound({ property: 'propertyValue' }); + if (bodyKeys.length > 1) throw Problem.user.unexpectedProperties({ expected: ['propertyValue'], actual: bodyKeys }); +}; + +const checkAuth = (auth) => { + if (auth.actor.value === undefined) throw Problem.user.insufficientRights(); +}; + +module.exports = (service, endpoint) => { + + //////////////////////////////////////////////////////////////////////////////// + // User preferences (UI settings) + // There are no endpoints to retrieve preferences here. Rather, the collection + // of preferences are served out through the extended version of /users/current. + + ////////////// + // Per-project + service.put('/user-preferences/project/:projectId/:propertyName', endpoint(({ UserPreferences }, { body, auth, params }) => { + checkAuth(auth); + checkBody(body); + return UserPreferences.writeProjectProperty(auth.actor.value.id, params.projectId, params.propertyName, body.propertyValue) + .then(success); + })); + + service.delete('/user-preferences/project/:projectId/:propertyName', endpoint(({ UserPreferences }, { auth, params }) => { + checkAuth(auth); + return UserPreferences.removeProjectProperty(auth.actor.value.id, params.projectId, params.propertyName) + .then(getOrNotFound); + })); + + /////////// + // Sitewide + service.put('/user-preferences/site/:propertyName', endpoint(({ UserPreferences }, { body, auth, params }) => { + checkAuth(auth); + checkBody(body); + return UserPreferences.writeSiteProperty(auth.actor.value.id, params.propertyName, body.propertyValue) + .then(success); + })); + + service.delete('/user-preferences/site/:propertyName', endpoint(({ UserPreferences }, { auth, params }) => { + checkAuth(auth); + return UserPreferences.removeSiteProperty(auth.actor.value.id, params.propertyName) + .then(getOrNotFound); + })); +}; diff --git a/lib/resources/users.js b/lib/resources/users.js index 91477effa..db4a1e95d 100644 --- a/lib/resources/users.js +++ b/lib/resources/users.js @@ -109,11 +109,12 @@ module.exports = (service, endpoint) => { } // Returns the currently authed actor. - service.get('/users/current', endpoint(({ Auth, Users }, { auth, queryOptions }) => + service.get('/users/current', endpoint(({ Auth, Users, UserPreferences }, { auth, queryOptions }) => auth.actor.map((actor) => ((queryOptions.extended === true) ? Promise.all([ Users.getByActorId(actor.id).then(getOrNotFound), Auth.verbsOn(actor.id, '*') ]) - .then(([ user, verbs ]) => Object.assign({ verbs }, user.forApi())) + .then(([ user, verbs ]) => UserPreferences.getForUser(user.actorId) + .then((preferences) => Object.assign({ verbs }, preferences, user.forApi()))) : Users.getByActorId(actor.id).then(getOrNotFound))) .orElse(Problem.user.notFound()))); diff --git a/lib/util/problem.js b/lib/util/problem.js index c5c99c61c..4efc90e08 100644 --- a/lib/util/problem.js +++ b/lib/util/problem.js @@ -124,6 +124,9 @@ const problems = { noConflictEntity: problem(400.32, () => 'The Entity doesn\'t have any conflict'), + // { expected: "list of expected properties", actual: "list of provided properties" } + unexpectedProperties: problem(400.33, ({ expected, actual }) => `Expected properties: (${expected.join(', ')}). Got (${actual.join(', ')}).`), + // no detail information for security reasons. authenticationFailed: problem(401.2, () => 'Could not authenticate with the provided credentials.'), diff --git a/test/integration/api/user-preferences.js b/test/integration/api/user-preferences.js new file mode 100644 index 000000000..8fbc163be --- /dev/null +++ b/test/integration/api/user-preferences.js @@ -0,0 +1,182 @@ +const { testService } = require('../setup'); + +describe('api: user-preferences', () => { + + it('validates the request body stringently', testService(async (service) => { + const asAlice = await service.login('alice'); + + await asAlice.put('/v1/user-preferences/site/someProperty') + .send({ notPropertyValue: 100 }) + .expect(400) + .then(({ body }) => { + body.code.should.eql(400.23); + body.details.property.should.eql('propertyValue'); + }); + + await asAlice.put('/v1/user-preferences/project/1/someProperty') + .send({ spuriousProperty: 'ouch', propertyValue: 100, pancakes: true }) + .expect(400) + .then(({ body }) => { + body.code.should.eql(400.33); + body.details.expected.should.eql(['propertyValue']); + body.details.actual.should.eql(['spuriousProperty', 'propertyValue', 'pancakes']); + }); + })); + + + it('can store a JS null propertyValue', testService(async (service) => { + const asAlice = await service.login('alice'); + + await asAlice.put('/v1/user-preferences/site/let-us-store-a-null') + .send({ propertyValue: null }) + .expect(200); + + await asAlice.get('/v1/users/current') + .set('X-Extended-Metadata', 'true') + .expect(200) + .then(({ body }) => { + body.preferences.should.eql({ + site: { + 'let-us-store-a-null': null, + }, + projects: { + }, + }); + }); + })); + + it('should not allow storing preferences for nonexistent projects', testService(async (service) => { + const asAlice = await service.login('alice'); + + await asAlice.put('/v1/user-preferences/project/123/someProperty') + .send({ propertyValue: 'someValue' }) + .expect(400); // project 123 doesn't exist + })); + + it('PUT/DELETE preference storage operations', testService(async (service) => { + const asAlice = await service.login('alice'); + + // Storage of simple property value + await asAlice.put('/v1/user-preferences/site/someSimpleSitePref') + .send({ propertyValue: true }) + .expect(200); + + // Storage of property with complex value + await asAlice.put('/v1/user-preferences/site/someComplexSitePref') + .send({ propertyValue: { 1: 2, 3: 4 } }) + .expect(200); + + // Overwriting should work; later we'll check whether we retrieve this value + await asAlice.put('/v1/user-preferences/site/someComplexSitePref') + .send({ propertyValue: [1, 2, 3] }) + .expect(200); + + // Store some pref to delete later on + await asAlice.put('/v1/user-preferences/site/toBeDeletedPref') + .send({ propertyValue: 'troep' }) + .expect(200); + + // Store a *project* property (simple value) + await asAlice.put('/v1/user-preferences/project/1/someSimpleProjectPref') + .send({ propertyValue: false }) + .expect(200); + + // Store a *project* property (complex value) + await asAlice.put('/v1/user-preferences/project/1/someComplexProjectPref') + .send({ propertyValue: [1, 2, 'many'] }) + .expect(200); + + // make a new project and read its ID + let newProjectID = null; + await asAlice.post('/v1/projects') + .send({ name: 'preferences test project' }) + .expect(200) + .then(({ body }) => { + newProjectID = body.id; + }); + + // we should be able to store a preference for the new project + await asAlice.put(`/v1/user-preferences/project/${newProjectID}/prefForSomeOtherProject`) + .send({ propertyValue: 9000 }) + .expect(200); + + // store a project preference on this new project to be deleted later on + await asAlice.put(`/v1/user-preferences/project/${newProjectID}/toBeDeletedPref`) + .send({ propertyValue: 'troep' }) + .expect(200); + + // insert a preference for Bob, which we don't want to find in the + // preferences for Alice that we will subsequently fetch. + await service.login('bob').then((asBob) => + asBob.put('/v1/user-preferences/site/koosje-likes-milk') + .send({ propertyValue: true }) + .expect(200) + ); + + // expected properties of the new project + const newProjectProps = {}; + newProjectProps[newProjectID] = { + prefForSomeOtherProject: 9000, + toBeDeletedPref: 'troep', + }; + + // check whether the built-up state is sane (eg stores and overwrites applied) + await asAlice.get('/v1/users/current') + .set('X-Extended-Metadata', 'true') + .expect(200) + .then(({ body }) => { + body.preferences.should.eql({ + site: { + someSimpleSitePref: true, + someComplexSitePref: [1, 2, 3], + toBeDeletedPref: 'troep', + }, + projects: Object.assign( + { + 1: { + someSimpleProjectPref: false, + someComplexProjectPref: [1, 2, 'many'], + }, + }, + newProjectProps, + ), + }); + }); + + // check deletion of a site preference + await asAlice.delete('/v1/user-preferences/site/toBeDeletedPref') + .expect(200); + + // check deletion of a project preference + await asAlice.delete(`/v1/user-preferences/project/${newProjectID}/toBeDeletedPref`) + .expect(200); + + // check whether deleting something nonexistent results in an informative error response + await asAlice.delete(`/v1/user-preferences/project/${newProjectID}/toBeDeletedPref`) + .expect(404); // as we've just deleted it + + delete newProjectProps[newProjectID].toBeDeletedPref; + + // check whether the built-up state is sane (deletions applied) + await asAlice.get('/v1/users/current') + .expect(200) + .set('X-Extended-Metadata', 'true') + .then(({ body }) => { + body.preferences.should.eql({ + site: { + someSimpleSitePref: true, + someComplexSitePref: [1, 2, 3], + }, + projects: Object.assign( + { + 1: { + someSimpleProjectPref: false, + someComplexProjectPref: [1, 2, 'many'], + }, + }, + newProjectProps + ), + }); + }); + })); +});