-
Notifications
You must be signed in to change notification settings - Fork 92
chore: Cleaned up the national ID integration code #7938
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
Conversation
|
Oops! Looks like you forgot to update the changelog. When updating CHANGELOG.md, please consider the following:
|
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.
👍 Thanks, looks good.
| MOSIP |
we could also remove it in these GraphQL schemas and then run frontend type generation:
cd packages/client
TOKEN="" yarn generate-gateway-typeshttps://is-my-opencrvs-up.netlify.app/
you can find the registrar's token in the left panel when you are running the app locally
|
@PathumN99 I think you might have to take care of these cases as well https://github.com/opencrvs/opencrvs-core/blob/5c402ea/packages/webhooks/src/features/event/service.ts#L59-L60 If you get rid of these cases. then the functions Then in the file - const transformedBundle = transformBirthBundle(
- bundle,
- webhookToNotify.createdBy.type,
- permissions
- )
+ let finalBundle: RegisteredRecord
+ if (webhookToNotify.createdBy.type === 'webhook') {
+ finalBundle = getPermissionsBundle(bundle, permissions)
+ } else {
+ finalBundle = bundle
+ }
if (webhookToNotify.trigger === TRIGGERS[TRIGGERS.BIRTH_REGISTERED]) {
const payload = {
timestamp: new Date().toISOString(),
@@ -80,7 +80,7 @@ export async function birthRegisteredHandler(
hub: {
topic: TRIGGERS[TRIGGERS.BIRTH_REGISTERED]
},
- context: [transformedBundle]
+ context: [finalBundle]
}
}Consider making similar changes to both |
tahmidrahman-dsi
left a comment
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.
@PathumN99 Nice work! Please have a look at the comment I made about the webhooks handlers
tahmidrahman-dsi
left a comment
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.
Good one @PathumN99, thank you
|
@PathumN99 one small test case needs to be removed to pass unit tests for webhooks opencrvs-core/packages/webhooks/src/features/event/service.test.ts Lines 1607 to 1611 in 764d1b0
Also after you remove this test case, please consider removing subsequent unused codes |
|
@PathumN99 on approval / ready to merge label, you can merge the pull request to develop if the tests pass. |
|
Your environment is deployed to https://ocrvs-7909.opencrvs.dev |
#7909