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(9586): implement freetext search in cht datasource #9625

Open
wants to merge 51 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 43 commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
ac0ab67
Initial setup
sugat009 Nov 6, 2024
cd11adc
add /api/v1/contacts/:uuid endpoint
sugat009 Nov 7, 2024
b7c5d1f
add /api/v1/contact/:uuid?with_lineage=<option> endpoint
sugat009 Nov 7, 2024
f91e393
add missing files
sugat009 Nov 7, 2024
52ac11b
add missing files
sugat009 Nov 7, 2024
c1b68cf
add endpoint /api/v1/report
sugat009 Nov 8, 2024
45c0ccf
add additional checks for report validity
sugat009 Nov 11, 2024
acf903a
add /api/contact/id endpoint (not tested yet)
sugat009 Nov 12, 2024
89927b1
add endpoint /api/v1/report/id --untested
sugat009 Nov 13, 2024
45abf58
implement search feature in /api/contact/id endpoint
sugat009 Nov 15, 2024
6f5c326
add search functionality to /api/report/id endpoint
sugat009 Nov 15, 2024
192af16
add async API Contact.v1.getIdsAll
sugat009 Nov 15, 2024
bb98dcf
add async API Report.v1.getIds
sugat009 Nov 15, 2024
43efbef
add JSDocs for functions
sugat009 Nov 18, 2024
ede85fd
Merge remote-tracking branch 'origin/master' into 9586-implement-free…
sugat009 Nov 18, 2024
2b33c07
add unit tests for index.ts, qualifier.ts and contact.ts
sugat009 Nov 20, 2024
3bfcace
add unit tests for report.ts
sugat009 Nov 20, 2024
074b0c1
add tests for local/contact.js
sugat009 Nov 25, 2024
1907907
add some additional tests in local/contact.spec.ts
sugat009 Nov 25, 2024
a629116
add tests for local/report.ts
sugat009 Nov 25, 2024
c45cee8
add unit tests for remote/contact.ts
sugat009 Nov 25, 2024
5400736
add unit tests for remote/report.ts
sugat009 Nov 25, 2024
bf46e4a
add unit tests for contact-types.ts
sugat009 Nov 25, 2024
b1cf669
remove unused variables
sugat009 Nov 25, 2024
98fd4e2
add unit tests for api/src/controllers/contact.js
sugat009 Nov 26, 2024
ee32262
add tests for api/src/controllers/report.js
sugat009 Nov 26, 2024
b5bc6e6
Merge remote-tracking branch 'origin/master' into 9586-implement-free…
sugat009 Nov 27, 2024
4a18fc8
Merge remote-tracking branch 'origin/master' into 9586-implement-free…
sugat009 Nov 27, 2024
eba1b61
add integration test for api/src/controller/contact.js
sugat009 Nov 29, 2024
9157059
add integration tests for api/src/controllers/report.js
sugat009 Nov 29, 2024
d5fed9c
fix eslint issues
sugat009 Nov 29, 2024
30d058e
changes missed during copy-paste
sugat009 Dec 2, 2024
662598c
change title of tests
sugat009 Dec 2, 2024
89eb816
fix indentation
sugat009 Dec 2, 2024
3ea31ef
fix max-line 120 issue
sugat009 Dec 2, 2024
a7a9272
rename getIdsAll functions to getIds
sugat009 Dec 3, 2024
ef8b93f
add tests in controller unit tests for when limit is undefined or null
sugat009 Dec 3, 2024
43c3896
add page limits and end of alphabet marker to separate constants file
sugat009 Dec 4, 2024
f97f928
add function in qualifier.ts to check for key-value based freetext qu…
sugat009 Dec 4, 2024
2065125
separate out the endpoints integration tests and cht-datasource integ…
sugat009 Dec 5, 2024
f8087b9
remove unused variables
sugat009 Dec 5, 2024
e6aea4f
move src/constants.ts to src/libs/constants.ts
sugat009 Dec 5, 2024
c21e52a
change limit to number and stringified number in cht-datasource
sugat009 Dec 6, 2024
c487a7e
Update shared-libs/cht-datasource/src/qualifier.ts
sugat009 Dec 23, 2024
ce1013a
Update shared-libs/cht-datasource/test/contact.spec.ts
sugat009 Dec 23, 2024
19143b4
Update shared-libs/cht-datasource/src/remote/contact.ts
sugat009 Dec 24, 2024
8dc7de3
addressing PR review comments
sugat009 Dec 24, 2024
91a0125
Merge branch '9586-implement-freetext-search-in-cht-datasource' of gi…
sugat009 Dec 24, 2024
49553b7
address most of the comments
sugat009 Dec 30, 2024
22da4b9
fix integration tests
sugat009 Dec 30, 2024
4a0d2bb
fix api unit tests
sugat009 Dec 31, 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
50 changes: 50 additions & 0 deletions api/src/controllers/contact.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
const auth = require('../auth');
const { Contact, Qualifier } = require('@medic/cht-datasource');
const ctx = require('../services/data-context');
const serverUtils = require('../server-utils');

const getContact = ({ with_lineage }) => ctx.bind(with_lineage === 'true' ? Contact.v1.getWithLineage : Contact.v1.get);
const getContactIds = () => ctx.bind(Contact.v1.getIdsPage);

const checkUserPermissions = async (req) => {
const userCtx = await auth.getUserCtx(req);
if (!auth.isOnlineOnly(userCtx) || !auth.hasAllPermissions(userCtx, 'can_view_contacts')) {
return Promise.reject({ code: 403, message: 'Insufficient privileges' });
}
};

module.exports = {
v1: {
get: serverUtils.doOrError(async (req, res) => {
await checkUserPermissions(req);
const { uuid } = req.params;
const contact = await getContact(req.query)(Qualifier.byUuid(uuid));

if (!contact) {
return serverUtils.error({ status: 404, message: 'Contact not found' }, req, res);
}

return res.json(contact);
}),
getIds: serverUtils.doOrError(async (req, res) => {
await checkUserPermissions(req);

if (!req.query.freetext && !req.query.type) {
return serverUtils.error({ status: 400, message: 'Either query param freetext or type is required' }, req, res);
}
const qualifier = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

As I mentioned in an earlier comment, ideally we should be able to use a Qualifier.and function to combine qualifier objects. Something like:

const getSearchQualifier = (freetext, type) => {
  if (freetext && type) {
    return Qualifier.and(Qualifier.byFreetext(freetext), Qualifier.byContactType(type));
  }
  if (freetext) {
    return Qualifier.byFreetext(freetext);
  }
  if (type) {
    return Qualifier.byContactType(type);
  }

  const err = new Error('Either query param freetext or type is required');
  err.status = 400;
  throw err;
};


if (req.query.freetext) {
Object.assign(qualifier, Qualifier.byFreetext(req.query.freetext));
}

if (req.query.type) {
Object.assign(qualifier, Qualifier.byContactType(req.query.type));
}

const docs = await getContactIds()(qualifier, req.query.cursor, req.query.limit);

return res.json(docs);
}),
},
};
3 changes: 1 addition & 2 deletions api/src/controllers/person.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,8 @@ module.exports = {
await checkUserPermissions(req);

const personType = Qualifier.byContactType(req.query.type);
const limit = req.query.limit ? Number(req.query.limit) : req.query.limit;

const docs = await getPageByType()( personType, req.query.cursor, limit );
const docs = await getPageByType()( personType, req.query.cursor, req.query.limit );

return res.json(docs);
}),
Expand Down
3 changes: 1 addition & 2 deletions api/src/controllers/place.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,8 @@ module.exports = {
await checkUserPermissions(req);

const placeType = Qualifier.byContactType(req.query.type);
const limit = req.query.limit ? Number(req.query.limit) : req.query.limit;

const docs = await getPageByType()( placeType, req.query.cursor, limit );
const docs = await getPageByType()( placeType, req.query.cursor, req.query.limit );

return res.json(docs);
})
Expand Down
39 changes: 39 additions & 0 deletions api/src/controllers/report.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
const auth = require('../auth');
const ctx = require('../services/data-context');
const serverUtils = require('../server-utils');
const { Report, Qualifier } = require('@medic/cht-datasource');

const getReport = () => ctx.bind(Report.v1.get);
const getReportIds = () => ctx.bind(Report.v1.getIdsPage);

const checkUserPermissions = async (req) => {
const userCtx = await auth.getUserCtx(req);
if (!auth.isOnlineOnly(userCtx) || !auth.hasAllPermissions(userCtx, 'can_view_reports')) {
return Promise.reject({ code: 403, message: 'Insufficient privileges' });
}
};

module.exports = {
v1: {
get: serverUtils.doOrError(async (req, res) => {
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 not a big fan of this callback style.

Copy link
Member Author

Choose a reason for hiding this comment

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

We've been doing this pattern for all the REST endpoints that call cht-datasource. What's the alternative?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO doOrError is a nice way to reduce duplicated code and ensure we are handling errors consistently.

Copy link
Member

Choose a reason for hiding this comment

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

I think a try-catch block is not so much duplication, and it's more transparent than a nested callback.
I understand this already exists. I'm not a fan.

await checkUserPermissions(req);
const { uuid } = req.params;
const report = await getReport()(Qualifier.byUuid(uuid));

if (!report) {
return serverUtils.error({ status: 404, message: 'Report not found' }, req, res);
}

return res.json(report);
}),
getIds: serverUtils.doOrError(async (req, res) => {
await checkUserPermissions(req);

const qualifier = Qualifier.byFreetext(req.query.freetext);
Copy link
Member

Choose a reason for hiding this comment

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

So ... this endpoint .. if it doesn't get neither a freetext query param or a limit query param, it will end up returning ALL reports?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope. it returns a 400 - Bad request error because freetext is required whereas limit is set to a default of 10000.
image


const docs = await getReportIds()(qualifier, req.query.cursor, req.query.limit);

return res.json(docs);
})
}
};
8 changes: 8 additions & 0 deletions api/src/routing.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,10 @@ const exportData = require('./controllers/export-data');
const records = require('./controllers/records');
const forms = require('./controllers/forms');
const users = require('./controllers/users');
const contact = require('./controllers/contact');
const person = require('./controllers/person');
const place = require('./controllers/place');
const report = require('./controllers/report');
const { people, places } = require('@medic/contacts')(config, db, dataContext);
const upgrade = require('./controllers/upgrade');
const settings = require('./controllers/settings');
Expand Down Expand Up @@ -492,6 +494,12 @@ app.postJson('/api/v1/people', function(req, res) {
app.get('/api/v1/person', person.v1.getAll);
app.get('/api/v1/person/:uuid', person.v1.get);

app.get('/api/v1/contact/id', contact.v1.getIds);
Copy link
Member

Choose a reason for hiding this comment

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

maybe /api/v1/contact/ids is more suitable.
The idea is that the URL isn't suggestive at all, without reading the implementation, I would never guess what this endpoint does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, REST API conventions are to name the API endpoint in a plural way like /api/v1/contacts or /api/v1/contacts/ids but this design decision had already been taken even before this ticket. I couldn't find the link to the conversation for this though.

Copy link
Contributor

Choose a reason for hiding this comment

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

That discussion happened in the parent ticket before we spun off the child isssue: #9544 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @jkuester . Your argument here is that "we've already decided and your input is not welcome?"

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for being aggressive and confrontational in the above comment.

I maintain my comment about /api/v1/contact/id being quite unsuggestive, we shouldn't need thorough explanations and reasoning behind the naming choice in order for an api name to make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was just trying to provide the context for the discussion that Sugat referenced. 😬

I am happy to continue the design discussion here to come to an agreed upon approach. It will just be most efficient if we all understand what was already said to get us here. When starting work on new REST endpoints for the cht-datasource code, we chose to go with the pattern of singular entity names (so /api/v1/person instead of /api/v1/persons). When the endpoint can return 0-n entities I do not really see a compelling reason to prefer either singular or plural (since either might make more sense depending on the context). Two things seem clear to me though:

  • Under normal circumstances, we should not duplicate endpoints for the same resource (e.g. having both /api/v1/contact/id and /api/v1/contact/ids).
  • We should be consistent with our naming across our go-forward REST endpoints. Either using singular or plural, but not mixing both.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with being consistent.

I personally can't recall seeing APIs in the world that used this singular form, so for me this seems quite unintuitive.

app.get('/api/v1/contact/:uuid', contact.v1.get);

app.get('/api/v1/report/id', report.v1.getIds);
app.get('/api/v1/report/:uuid', report.v1.get);

app.postJson('/api/v1/bulk-delete', bulkDocs.bulkDelete);

// offline users are not allowed to hydrate documents via the hydrate API
Expand Down
Loading
Loading