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

feat(#8877): Look up users from their facility_id or contact_id #8928

Merged
merged 66 commits into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
66 commits
Select commit Hold shift + click to select a range
4b3a3a1
move the auth check up in the parent function
m5r Mar 13, 2024
5819151
support `facility_id` query parameter
m5r Mar 13, 2024
a007db9
support `facility_id` query parameter for user settings too
m5r Mar 14, 2024
14281f6
support filtering with `contact_id` query parameter
m5r Mar 14, 2024
c6ac0f3
cleaner approach with/without filters
m5r Mar 18, 2024
5684089
good catch sonar
m5r Mar 18, 2024
a993c0f
fix unit tests
m5r Mar 18, 2024
fd18a9c
unit test `getList()` with filters
m5r Mar 18, 2024
d117a49
test api controller with filters
m5r Mar 18, 2024
e93c001
remove .only
m5r Mar 18, 2024
f78e568
Merge remote-tracking branch 'origin/master' into 8877-lookup-single-…
m5r Mar 20, 2024
0beaf25
bulk create users instead of creating them one by one
m5r Mar 20, 2024
039f0cb
oops how could I miss that
m5r Mar 20, 2024
f2cfda3
fix facility.spec.js
m5r Mar 20, 2024
48030f1
add integration tests for `GET /api/v2/users` with filters
m5r Mar 20, 2024
3fba6d8
log how long it takes in CI
m5r Mar 20, 2024
f08a34c
debug savedUser
m5r Mar 21, 2024
4b0bf8a
lint
m5r Mar 21, 2024
3d5455c
okay other tests in the suite are leaving users with no place or cont…
m5r Mar 21, 2024
4ddab32
try speeding it up by running filtered queries in parallel
m5r Mar 21, 2024
b0ddc30
remove logs
m5r Mar 21, 2024
c319cad
comment the lodash magic
m5r Mar 21, 2024
cd0dff7
Update shared-libs/user-management/src/users.js
m5r Mar 25, 2024
a9967a1
switch to `setting.facility_id`
m5r Mar 25, 2024
0e4b82a
add other unsupported query parameters to unit test
m5r Mar 25, 2024
5a605d1
Update shared-libs/user-management/test/unit/users.spec.js
m5r Mar 25, 2024
7d44261
fix user-management shared lib unit tests
m5r Mar 25, 2024
61cb17b
stub lower level db calls to cover functions `getAllUserSettings` and…
m5r Mar 25, 2024
06f15c8
integration test for `GET /api/v2/users` with filters
m5r Mar 27, 2024
6ae5c45
g checkout origin/master `ddocs/medic-db/medic-client/views/doc_by_ty…
m5r Apr 1, 2024
4dbe6b1
query with `facility_id` & `contact_id` filters on `_users` instead o…
m5r Apr 1, 2024
ab09351
`add-contact-id-to-user-docs` migration
m5r Apr 2, 2024
1c0a1cb
okay I think I found the place where all the magic happens
m5r Apr 3, 2024
8a703cd
fix tests
m5r Apr 4, 2024
16c39e3
reduce function cognitive coplexity
m5r Apr 4, 2024
42fc2f4
use async await
m5r Apr 4, 2024
bab9d56
test that a deleted user doesn't get returned by the route
m5r Apr 4, 2024
fa55975
remove .only
m5r Apr 4, 2024
302730f
test that both user and user-settings documents get updated with `fac…
m5r Apr 4, 2024
0165b84
move `auth.check()` up the chain
m5r Apr 4, 2024
08d24bc
use `contact_id` from `user` doc
m5r Apr 4, 2024
4667bec
increase batch size to 100
m5r Apr 4, 2024
3ce0587
remove duplicate test
m5r Apr 4, 2024
9703957
Add integration tests for migration
jkuester Apr 9, 2024
6a4cfd9
Update whole batch of users at the same time in migration
jkuester Apr 9, 2024
13475b6
Add users_by_facility_and_contact view to _users db
jkuester Apr 10, 2024
03ab927
Split out into two facility/contact views. Call views from user-manag…
jkuester Apr 10, 2024
5729789
Fix unit tests to expect queries
jkuester Apr 10, 2024
11994ae
Fix api unit tests
jkuester Apr 11, 2024
b32fcf0
Tighten up code
jkuester Apr 11, 2024
6f720ea
Add additional user-management assertions for new functionality
jkuester Apr 11, 2024
9c6ed2e
Add user-settings contact_id assertions back in
jkuester Apr 11, 2024
febf3cc
Simplify from chained lodash calls
jkuester Apr 15, 2024
0ea6ac1
Fix indent
jkuester Apr 15, 2024
a8acee8
Use allDocs instead of bulkGet
jkuester Apr 15, 2024
6902ddd
Clean up allDocs call
jkuester Apr 15, 2024
348afc8
Filter allDocs call on _users db to avoid ddocs
jkuester Apr 15, 2024
f99cd6d
Fix indent
jkuester Apr 15, 2024
5325724
Reorder facilityId filtering logic
jkuester Apr 15, 2024
a7fbff1
Fix formatting
jkuester Apr 15, 2024
2671032
Clean up getUserSettings function and fix tests
jkuester Apr 15, 2024
2c5f740
Add assertion that facility_id not modified by migration
jkuester Apr 15, 2024
903a636
Fix broken api controller tests
jkuester Apr 15, 2024
d420d48
Combine views into users_by_field
jkuester Apr 16, 2024
3b75cca
Add unit tests for migration
jkuester Apr 16, 2024
fe7cf0c
Fix filtering bug in user-management
jkuester Apr 17, 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
15 changes: 10 additions & 5 deletions api/src/controllers/users.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,8 @@ const getAllowedDocsCounts = async (userCtx) => {
// In express4, req.host strips off the port number: https://expressjs.com/en/guide/migrating-5.html#req.host
const getAppUrl = (req) => `${req.protocol}://${req.hostname}`;

const getUserList = async (req) => {
await auth.check(req, 'can_view_users');
return await users.getList();
const getUserList = async (filters = {}) => {
return await users.getList(filters);
};

const getType = user => {
Expand All @@ -132,7 +131,8 @@ const convertUserListToV1 = (users=[]) => {

module.exports = {
get: (req, res) => {
return getUserList(req)
return auth.check(req, 'can_view_users')
.then(() => getUserList())
.then(list => convertUserListToV1(list))
.then(body => res.json(body))
.catch(err => serverUtils.error(err, req, res));
Expand Down Expand Up @@ -233,7 +233,12 @@ module.exports = {
v2: {
get: async (req, res) => {
try {
const body = await getUserList(req, res);
await auth.check(req, 'can_view_users');
const filters = _.chain(req.query)
.pick(['facility_id', 'contact_id']) // keep only supported filtering properties
.mapKeys((value, key) => _.camelCase(key)) // rename them to keep consistent variable names
.value();
jkuester marked this conversation as resolved.
Show resolved Hide resolved
const body = await getUserList(filters);
res.json(body);
} catch (err) {
serverUtils.error(err, req, res);
Expand Down
69 changes: 62 additions & 7 deletions api/tests/mocha/controllers/users.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,24 @@ describe('Users Controller', () => {
afterEach(() => sinon.restore());

describe('get users list', () => {
let userList;

beforeEach(() => {
userList = [
{ id: 'org.couchdb.user:admin', roles: ['_admin'] },
{
id: 'org.couchdb.user:chw',
roles: ['chw', 'district-admin'],
contact: {
_id: 'chw-contact',
parent: { _id: 'chw-facility' },
}
},
{ id: 'org.couchdb.user:unknown' },
];
req = { };
res = { json: sinon.stub() };
sinon.stub(users, 'getList').resolves([
{ id: 'org.couchdb.user:admin', roles: [ '_admin' ] },
{ id: 'org.couchdb.user:chw', roles: [ 'chw', 'district-admin' ] },
{ id: 'org.couchdb.user:unknown' },
]);
sinon.stub(users, 'getList').resolves(userList);
});

describe('v1', () => {
Expand Down Expand Up @@ -69,14 +78,13 @@ describe('Users Controller', () => {
});

describe('v2', () => {

it('rejects if not permitted', async () => {
sinon.stub(auth, 'check').rejects(new Error('nope'));
await controller.v2.get(req, res);
chai.expect(serverUtils.error.callCount).to.equal(1);
});

it('gets the list of users', async () => {
it('gets the list of users without filters', async () => {
sinon.stub(auth, 'check').resolves();

await controller.v2.get(req, res);
Expand All @@ -92,6 +100,53 @@ describe('Users Controller', () => {
chai.expect(result[2].roles).to.be.undefined;
});

it('gets the list of users with facility_id filter', async () => {
sinon.stub(auth, 'check').resolves();
users.getList.resolves([userList[1]]);
req.query = { facility_id: 'chw-facility' };
m5r marked this conversation as resolved.
Show resolved Hide resolved

await controller.v2.get(req, res);
chai.expect(users.getList.firstCall.args[0]).to.deep.equal({ facilityId: 'chw-facility' });
const result = res.json.args[0][0];
chai.expect(result.length).to.equal(1);
chai.expect(result[0].id).to.equal('org.couchdb.user:chw');
chai.expect(result[0].type).to.be.undefined;
chai.expect(result[0].roles).to.deep.equal(['chw', 'district-admin']);
chai.expect(result[0].contact._id).to.equal('chw-contact');
chai.expect(result[0].contact.parent._id).to.equal('chw-facility');
});

it('gets the list of users with facility_id and contact_id filters', async () => {
sinon.stub(auth, 'check').resolves();
users.getList.resolves([userList[1]]);
req.query = { facility_id: 'chw-facility', contact_id: 'chw-contact' };

await controller.v2.get(req, res);
chai.expect(users.getList.firstCall.args[0]).to.deep.equal({
contactId: 'chw-contact',
facilityId: 'chw-facility',
});
const result = res.json.args[0][0];
chai.expect(result.length).to.equal(1);
chai.expect(result[0].id).to.equal('org.couchdb.user:chw');
chai.expect(result[0].type).to.be.undefined;
chai.expect(result[0].roles).to.deep.equal(['chw', 'district-admin']);
chai.expect(result[0].contact._id).to.equal('chw-contact');
chai.expect(result[0].contact.parent._id).to.equal('chw-facility');
});

it('gets the list of users and ignores unexpected filters', async () => {
sinon.stub(auth, 'check').resolves();
req.query = { roles: ['chw'], name: 'admin' };

await controller.v2.get(req, res);
chai.expect(users.getList.firstCall.args[0]).to.deep.equal({});
const result = res.json.args[0][0];
chai.expect(result.length).to.equal(3);
chai.expect(result[0].id).to.equal('org.couchdb.user:admin');
chai.expect(result[1].id).to.equal('org.couchdb.user:chw');
chai.expect(result[2].id).to.equal('org.couchdb.user:unknown');
});
});

});
Expand Down
7 changes: 7 additions & 0 deletions ddocs/medic-db/medic-client/views/doc_by_type/map.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,12 @@ function(doc) {
});
return;
}
if (doc.type === 'user-settings') {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little worried by this change. This will mean that all clients (users' phones) will need to index this massive view again due to this change that only gets used on the server side.
I find it hard to see a context where we would need this change on the client. I think it makes more sense to put this new view code in _design/medic somehow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch there. I don't see any existing view in _design/medic we could piggyback off of so we'd have to make a new view.
The way I see it, I could either duplicate the medic-client/doc_by_type code to a medic/doc_by_type view and pop the changes there (and potentially redirect all the medic-client/doc_by_type queries in API to this view) or make a specific view named like medic/user_settings_by_facility_and_contact that would look like this:

function(doc) {
  if (doc.type === 'user-settings') {
    emit(doc.facility_id);
    emit(doc.contact_id);
    emit([doc.facility_id, doc.contact_id]);
  }
}

I'm leaning towards the first option and duplicate the view and re-map API queries from the client view to have a distinct API-only doc_by_type view so that we can make changes to it without forcing clients to index it again.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, we probably only want to do the first option if it would mean that we could avoid having to build the medic-client/doc_by_type on the server at all. Otherwise, having both medic-client/doc_by_type and medic/doc_by_type would mean a non-trivial addition to the disk space being used on the server. Do we support having views on the server that do not get warmed?

Copy link
Member

Choose a reason for hiding this comment

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

Do we support having views on the server that do not get warmed?
We index views when we install, and then, with CouchDb3, views get indexed automatically in the background. This can be disabled for a whole ddoc - but requires editing properties on the ddoc, which we have not done.

I think the best deal would be to:

  • have both facility_id and contact_id saved on the doc from the _users database
  • add a new ddoc to the _users database that contains the view that indexes users by facility and contact
  • change code so the contact_id is also saved in the _users document.
  • write a migration that iterates over all _users docs, gets the contact_id from the user-settings doc and saves it on the _users doc.

I can't think of any legitimate reason for not storing the contact_id on the _users doc. We already know we need to keep the _users doc and the user-settings doc in sync to prevent all sorts of weirdness in the app.

emit([ doc.type ]);
emit([ doc.type, doc.facility_id ]);
emit([ doc.type, doc.contact_id ]);
emit([ doc.type, doc.facility_id, doc.contact_id ]);
return;
}
emit([ doc.type ]);
}
2 changes: 1 addition & 1 deletion shared-libs/user-management/src/libs/facility.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const db = require('./db');
const list = async (users, settings) => {
const ids = new Set();
for (const user of users) {
ids.add(user?.doc?.facility_id);
ids.add(user?.facility_id);
}
for (const setting of settings) {
ids.add(setting?.contact_id);
Expand Down
52 changes: 39 additions & 13 deletions shared-libs/user-management/src/users.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,18 +105,34 @@ const getDocID = doc => {
}
};

const getAllUserSettings = () => {
const getAllUserSettings = ({ facilityId, contactId } = {}) => {
const key = ['user-settings'];
if (facilityId) {
key.push(facilityId);
}
if (contactId) {
key.push(contactId);
}

const opts = {
include_docs: true,
key: ['user-settings']
key,
};
return db.medic.query('medic-client/doc_by_type', opts)
.then(result => result.rows.map(row => row.doc));
};

const getAllUsers = () => {
return db.users.allDocs({ include_docs: true })
.then(result => result.rows);
.then(result => result.rows.map(({ doc }) => doc));
};

const getUsersByIds = async (ids) => {
const docs = ids.map(id => ({ id }));
const { results } = await db.users.bulkGet({ docs });
return results
.map(result => result.docs && result.docs[0] && result.docs[0].ok)
m5r marked this conversation as resolved.
Show resolved Hide resolved
.filter(doc => doc);
};

const validateContact = (id, placeID) => {
Expand Down Expand Up @@ -335,21 +351,21 @@ const hasParent = (facility, id) => {
const mapUsers = (users, settings, facilities) => {
users = users || [];
return users
.filter(user => user.id.indexOf(USER_PREFIX) === 0)
.filter(user => user._id.indexOf(USER_PREFIX) === 0)
.map(user => {
const setting = getDoc(user.id, settings) || {};
const setting = getDoc(user._id, settings) || {};
return {
id: user.id,
rev: user.doc._rev,
username: user.doc.name,
id: user._id,
rev: user._rev,
username: user.name,
fullname: setting.fullname,
email: setting.email,
phone: setting.phone,
place: getDoc(user.doc.facility_id, facilities),
roles: user.doc.roles,
place: getDoc(user.facility_id, facilities),
m5r marked this conversation as resolved.
Show resolved Hide resolved
roles: user.roles,
contact: getDoc(setting.contact_id, facilities),
external_id: setting.external_id,
known: user.doc.known
known: user.known
};
});
};
Expand Down Expand Up @@ -770,8 +786,18 @@ const getUserSettings = async({ name }) => {
*/
module.exports = {
deleteUser: username => deleteUser(createID(username)),
getList: async () => {
const [ users, settings ] = await Promise.all([ getAllUsers(), getAllUserSettings() ]);
getList: async (filters) => {
let users;
let settings;

if (_.isEmpty(filters)) {
[users, settings] = await Promise.all([getAllUsers(), getAllUserSettings()]);
} else {
settings = await getAllUserSettings(filters);
const ids = settings.map(({ _id }) => _id);
users = await getUsersByIds(ids);
jkuester marked this conversation as resolved.
Show resolved Hide resolved
}

const facilities = await facility.list(users, settings);
return mapUsers(users, settings, facilities);
},
Expand Down
4 changes: 2 additions & 2 deletions shared-libs/user-management/test/unit/libs/facility.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ const db = require('../../../src/libs/db');

describe('facility', () => {

const userA = { doc: { facility_id: 'a' } };
const userB = { doc: { facility_id: 'b' } };
const userA = { facility_id: 'a' };
const userB = { facility_id: 'b' };

const settingA = { contact_id: 'a' };
const settingB = { contact_id: 'e' };
Expand Down
Loading
Loading