feat(#8986) add api for getting single user #9016
Conversation
…act, we need to handle them gracefully
Co-authored-by: Joshua Kuestersteffen <[email protected]>
Co-authored-by: Joshua Kuestersteffen <[email protected]>
… `getUsersByIds` behaviors
jkuester
force-pushed
the
8986-lookup-single-user
branch
from
edf6d86 to
de5846f
Compare
| @@ -3020,14 +3149,27 @@ describe('Users service', () => { | |||
| sinon.stub(roles, 'isOffline').returns(false); | |||
|
|
|||
| const updates = { token_login: true, phone: '+40 755 89-89-89' }; | |||
| db.medic.get.resolves({ | |||
| db.medic.get.onFirstCall().resolves({ | |||
There was a problem hiding this comment.
This particular test had some very sketchy things happening behind the scenes due to some pass-by-reference effects. Basically, the asserts below were "working" because the arg objects were getting passed around and continued to be updated as the logic flow continued (because of the Object.assign calls in getUpdatedUserDoc and getUpdatedSettingsDoc). When I replaced the Object.assign calls by spreading the data into a new object, that broke this test.
However, I think the updated test code is more explicit and accurate anyway.
# Conflicts: # api/tests/mocha/controllers/users.spec.js # shared-libs/user-management/src/users.js # shared-libs/user-management/test/unit/users.spec.js
| return; | ||
| } | ||
|
|
||
| expect.fail('Should have thrown an error'); |
There was a problem hiding this comment.
If you move this to the try block, after the await utils.request, you don't have to return in your catch.
There was a problem hiding this comment.
So, what I don't like about doing it that way is that the error generated by the fail gets caught in the catch block. Then, if request call does succeed and the expected error is not thrown, the test will fail, but it will fail with an error like "Expected: Failed to find user... but got Should have thrown an error". Not a huge thing, but can be a bit confusing when you go to start debugging.
This approach with the return is the most convenient I could think of, but open to other ideas!
There was a problem hiding this comment.
It's probably going to throw an error for cannot read properties of undefined (reading 'code') since the error being caught is destructured
The early return doesn't throw me off that much 😅
There was a problem hiding this comment.
I logically prefer expect.fail-ing withing the try block. when I read this code, I immediately know: oh, this request is supposed to fail, and I prepare my brain for "why is it supposed to fail" and "how does the error look like".
If I read the code the way it is now, top down, I see: oh, I'm OK with this request request not failing, but if it fails I run some assertions. Oh, I am not fine with this request failing.
If I read bottom up, I see an expect.fail at the end of the test and I think: oh, this is weird, so this test is not supposed to run until the end. Now I have to search for all the returns and throws within the test.
I totally understand that when reading error messages when the test fails, you get a prettier error message. My comment is about actually reading and understanding the test code.
This is pretty minor and I'm not having a super strong opinion about this, it's just a preference about what makes sense and is easiest to follow for me when I read code.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Description
Closes #8986
Docs PR: medic/cht-docs#1350
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.