feat(#9194): add api support for getting place by uuid #9177
Conversation
…nabled `composite`.
…atasource object is a nested hierarchy.
…5_get_contact_by_id
… have app-settings service be injected
# Conflicts: # sentinel/src/lib/purging.js
… to be more useful.
Switched back to fetch api for now. Also made url optional for remote datasoure (allowing for relative requests from webapp/admin).
# Conflicts: # shared-libs/cht-datasource/src/place.ts # tests/integration/api/controllers/person.spec.js # webapp/src/ts/services/enketo.service.ts
| @@ -17,7 +16,7 @@ export namespace v1 { | |||
| return false; | |||
| } | |||
| const hasPersonType = contactTypeUtils.isPerson(settings.getAll(), doc); | |||
| if (!hasPersonType || !isNormalizedParent(doc)) { | |||
There was a problem hiding this comment.
I got too scared of this isNormalizedParent check since it is validating the contents of parent. I don't think, especially in v1, we want to block reading a contact because of problems in their parent lineage.
There was a problem hiding this comment.
Also, it is worth noting that the original logic when reading a person in shared-libs/contacts was just to check the type and nothing else. So we are staying consistent here and avoiding new validations.
|
@lorerod I was hoping you could just give a quick review of this PR since it is so close in functionality to the get-person and get-person-with-lineage PRs you reviewed recently! 🙏 (There are a lot of other file changes, but the only behavior change is the addition of the get-place REST endpoint. That is why that is the only integration/e2e test change. Everything else should be passive.) |
| const getPlace = Place.v1.get(dataContext); | ||
| const getPlaceWithLineage = Place.v1.getWithLineage(dataContext); |
There was a problem hiding this comment.
What would happen if the dataContext is invalid? Do you think this could happen?
There was a problem hiding this comment.
In general, if you try to curry in an invalid dataContext (e.g. null, or some other value), this line will throw an exception.
However, in this case, dataContext cannot be invalid here. The call above on L30 to getRemoteDataContext will either return a valid data context or it will throw an exception.
There was a problem hiding this comment.
We have a number of unit tests for these error scenarios in cht-datasource, but I did not think it was worth adding any integration tests since the validation logic all happens in cht-datasource and so there is not really anything to "integrate" with in those test cases.
A separate kind of test would be to create a remote data context with an invalid URL (one that does not actually point to a CHT instance). In that case, we would not fail until below when we call the getPlaceWithLineage function and that will throw an HTTP error (maybe a timeout error trying to connect to a server that does not exist??) We don't have any test like that yet, (and probably does not belong in the place controller tests), but maybe we should consider some more generic cht-datasource integration tests... 🤔 Let me know if you want to see that as a part of this PR!
webapp/tests/karma/ts/services/transitions/create-user-for-contacts.transition.spec.ts
Outdated
Show resolved
Hide resolved
jkuester
changed the title
| @@ -0,0 +1,5 @@ | |||
| module.exports = { | |||
| init: function(dataContext) { | |||
| Object.assign(module.exports, dataContext, { init: this.init }); | |||
There was a problem hiding this comment.
I'm not a fan of this pattern because it's a bit obscure, it's not immediately clear what it does.
I have yet to come up with a clearer alternative. The first thing I thought of involved using a global variable to store dataContext but it's a worse approach imo as it would conflict with other instances of dataContext spawned within the same process but for different purposes...
I see this pattern is used in 10 different places in the code base already so let's keep it this way for now and revisit later. I suspect it won't be problematic until we want to switch to typescript
There was a problem hiding this comment.
100% agree. This is basically just a janky hacked-up form of dependency injection. However, as you noted, any alternative approach seems way worse. At lease until we come up with a more proper DI solution, it seems like this weirdness gives us the best balance of testing flexibility while localizing most of the weirdness to just these files.
Description
Closes #9194
Docs PR: medic/cht-docs#1423
This PR adds support for get-place and get-place-with-lineage workflows in both cht-datasource and api REST endpoint.
Additionally, I have "refactored" some code throughout the codebase to use the cht-datasource functionality. My goal here was not to update every possible instance, but to try and snag all of the easy/simple cases where we are getting a person/place. The only places I ended up adding additional logic/tests for this re-factoring was when it made sense to throw an error if the person/place was not found (the previous
PouchDB.getcode had this behavior, but often we seem to have relied on it without testing it).When deciding what code I could refactor to use the new APIs, the main consideration was how confident we could be that we knew specifically in that code that the contact we were getting was a person or a place (in many cases we just retrieve a generic contact and do not know what it is until the doc comes back. See #9065 (comment) for more discussion of this challenge.) When deciding a particular piece of code could be refactored, I weighed it against these considerations:
shared-lib/contactslogic where it was clear from the function names and surround logic, exactly what we were trying to do.)personand the facility should always be a placeparentfor any contact should always be a placeIf any of these assumptions seems like too much of a stretch or some code change feels risky. call it out! I think for this PR, it would be better to just roll back the change and move on for now than get caught up too deep in logic/refactoring investigations....
Related code changes:
Code review checklist
Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.