Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Persist and serve user preferences #1184

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions lib/http/service.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion lib/model/container.js
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
38 changes: 38 additions & 0 deletions lib/model/migrations/20241008-01-add-user_preferences.js
Original file line number Diff line number Diff line change
@@ -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 };

123 changes: 123 additions & 0 deletions lib/model/query/user-preferences.js
Original file line number Diff line number Diff line change
@@ -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]
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering if you wanted to trim the propertyName. I noticed i'm able to make properties named ' ' and ' '.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 No, in the end I think that would be bad. I don't think I should treat the property name as anything else but an opaque token, as it's just an identifier. Spaces in the URL path component for the property name (encoded though of course) are valid.

Tangentially, Github markdown squished (trimmed) your ' ' and ' ' whitespace property name examples. That's sort of illustrative: it's because Markdown is for presentation, and it is documented to do that. Yet for the property names in the DB and API I don't see a benefit in constraining the property name (beyond refusing a zero-length string). They're chosen by developers, and of course a developer can choose a bad property name (including ' ') just like they can choose bad variable names, but presumably we'd catch that in review, just like we catch bad variable names.

.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 };
61 changes: 61 additions & 0 deletions lib/resources/user-preferences.js
Original file line number Diff line number Diff line change
@@ -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);
}));
};
5 changes: 3 additions & 2 deletions lib/resources/users.js
Original file line number Diff line number Diff line change
Expand Up @@ -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())));

Expand Down
3 changes: 3 additions & 0 deletions lib/util/problem.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(', ')}).`),

Comment on lines +127 to +129
Copy link
Member

Choose a reason for hiding this comment

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

Is there another existing Problem we could use instead? I see this only comes up if you pass propertyValue AND extra stuff like {'propertyValue': 'meow', 'cats': ' '}. Maybe we just accept propertyValue and ignore the rest.

I'm also ok leaving it as is with this new Problem because there is a test for it :)

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 did look at the existing Problems, and they had their problems. There's unexpectedAttributes but its comments and problem string then proceed to talk about arguments rather than attributes. I didn't feel like fixing that because due to the ambiguity some use sites will use it "because it's about arguments" and some others will use it "because it's about attributes" so that would require unraveling the original intent at each use site.

I could probably have lived with using the existing unexpectedAttributes error even though its name implies we're talking about "attributes" rather than "properties" ("properties" is what we're dealing with in json), I'm not that zealous, but I drew the line at "arguments" as in unexpectedAttributes's error text. Because talking about "arguments" will make for a confusing error message in the context of posting a JSON body, and I value a precise and appropriate error message more than I value saving a line of code in this case ;-)

// no detail information for security reasons.
authenticationFailed: problem(401.2, () => 'Could not authenticate with the provided credentials.'),

Expand Down
Loading