-
Notifications
You must be signed in to change notification settings - Fork 7
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
Rn 588 data broker tei #4045
Rn 588 data broker tei #4045
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Early review on the two meaningful commits - looking good so far, will review the PR as a whole now
@@ -32,11 +32,12 @@ const mockFind = (array, criteria = {}) => | |||
|
|||
if (key.includes('->>')) { | |||
// Special case | |||
const lookupPath = key.split('->>'); | |||
if (lookupPath.length > 2) throw new Error('Not implemented yet...'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hooray, implemented now!
@@ -0,0 +1,3 @@ | |||
export const addTrackedEntityCodes = async (models, events) => { | |||
return events; | |||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah didn't spot this in my once-over, removed
* Data pulls sometimes return tracked entity instance ids. This translator | ||
* converts these ids to entity codes. | ||
*/ | ||
export class TrackedEntityInstanceTranslator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some great deep work in here, thanks for doing so much code quality improvement as part of a hotfix
* It detects such instances, and swaps all the codes it is given to ids for the fetch, then does the same for | ||
* the data coming back (swapping ids in the response data for codes). | ||
*/ | ||
export class DhisCodeToIdTranslator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huge name improvement, and great comment above
import { expect } from 'chai'; | ||
import { createModelsStub } from '../../testUtilities'; | ||
|
||
describe('createModelsStub', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So meta
* @param {MockData} mockData | ||
* @return {} a partial implementation of ModelRegistry, to be used in its place | ||
*/ | ||
export const createModelsStub = mockData => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good idea doing this
Goodbye terribly long name...
d76455e
to
1cfb222
Compare
Rebased on master so we can deploy this separately as a hotfix, the only commits that were dropped out of this PR were: These were part of the feature branch when testing. Looking through them, none of them have any changes related to DHIS or data pulling, so should be safe to pull out. |
RN-588
A bunch of refactors around testing, and then two last commits contain: