Fixed odoo.dob.field and odoo.weight.field properties and 'patientVoided' → 'voided'.
#23
Conversation
|
cc: @icrc-fdeniger |
| @@ -23,7 +23,7 @@ | |||
|
|
|||
| <choice> | |||
| <when> | |||
| <simple>${exchangeProperty.patient-odoo-id} == null && ${exchangeProperty.patient.get('patientVoided')} == true</simple> | |||
| <simple>${exchangeProperty.patient-odoo-id} == null && ${exchangeProperty.patient.get('voided')} == true</simple> | |||
There was a problem hiding this comment.
I think you might want to check both voided and patientVoided, a person can be linked to more than one entity e.g. User, Provider and Patient. Therefore, a patient account can get voided but not necessarily their person record, or user or provider accounts.
There was a problem hiding this comment.
@wluyima it seems like we have voided attribute in patient model but AFAIK we don't have any attribute as patientVoided
We have personVoided in person model.
CMIIW
There was a problem hiding this comment.
Sorry! That's what I meant to check both Person.personVoided and Patient.voided
There was a problem hiding this comment.
@VaishSiddharth @wluyima why doing test on both properties personVoided and Patient.voided as in this route we deal with patient data only ( and with patient identifier,...) and only patient are synchronized with Odoo.
IMHO, The use case "a patient is voided but the person is not voided" should be ignored as well as this route need patients only. If a paitent is voided, we will have an error when trying to get the patient identifier and it causes erros in this route when a patient is voided.
Why should the use case "a patient is voided but the person is not voided" be processed in this route ?
There was a problem hiding this comment.
There was definitely a bug that this PR seems to address. And in fact, these changes aligns with what am saying so I guess it's all good.
| @@ -10,10 +10,10 @@ | |||
| <simple>${properties:odoo.weight.field:null} != null && ${properties:odoo.weight.field:null} != ''</simple> | |||
|
|
|||
| <setProperty name="odooWeightField"> | |||
| <simple>{{odoo.weight.field}}</simple> | |||
| <simple>{{properties:odoo.weight.field}}</simple> | |||
There was a problem hiding this comment.
Is this meant to address a bug?
| @@ -10,7 +10,7 @@ | |||
| <simple>${properties:odoo.dob.field:null} != null && ${properties:odoo.dob.field:null} != ''</simple> | |||
|
|
|||
| <setProperty name="odooDobField"> | |||
| <simple>{{odoo.dob.field}}</simple> | |||
| <simple>{{properties:odoo.dob.field}}</simple> | |||
There was a problem hiding this comment.
Is this meant to address a bug?
There was a problem hiding this comment.
I think this is actually introducing a bug... @icrc-loliveira have you tested this? Or did you intend to do as below?
| <simple>{{properties:odoo.dob.field}}</simple> | |
| <simple>${properties:odoo.dob.field}</simple> |
Which kind of bug is being fixed here though?
| </setProperty> | ||
|
|
||
| <toD cacheSize="-1" uri="{{openmrs.baseUrl}}/ws/rest/v1/obs?concept={{emr.weight.concept}}&patient=${exchangeProperty.patient.get('uuid')}" /> | ||
| <toD cacheSize="-1" uri="{{openmrs.baseUrl}}/ws/rest/v1/obs?concept=${exchangeProperty.odooWeightField}&patient=${exchangeProperty.patient.get('uuid')}" /> |
There was a problem hiding this comment.
Is this meant to address a bug?
There was a problem hiding this comment.
Yes, everything I have done on this PR is to address bugs that I have encounter.
There was a problem hiding this comment.
Hi @icrc-loliveira, thanks for the PR. I just have a few concerns below.
| @@ -10,7 +10,7 @@ | |||
| <simple>${properties:odoo.dob.field:null} != null && ${properties:odoo.dob.field:null} != ''</simple> | |||
|
|
|||
| <setProperty name="odooDobField"> | |||
| <simple>{{odoo.dob.field}}</simple> | |||
| <simple>{{properties:odoo.dob.field}}</simple> | |||
There was a problem hiding this comment.
I think this is actually introducing a bug... @icrc-loliveira have you tested this? Or did you intend to do as below?
| <simple>{{properties:odoo.dob.field}}</simple> | |
| <simple>${properties:odoo.dob.field}</simple> |
Which kind of bug is being fixed here though?
| @@ -10,10 +10,10 @@ | |||
| <simple>${properties:odoo.weight.field:null} != null && ${properties:odoo.weight.field:null} != ''</simple> | |||
|
|
|||
| <setProperty name="odooWeightField"> | |||
| <simple>{{odoo.weight.field}}</simple> | |||
| <simple>{{properties:odoo.weight.field}}</simple> | |||
|
@wluyima @Ruhanga could you please review the changes done by @icrc-loliveira and confirm if it's ok now? thank you cc @icrc-fdeniger |
|
@wluyima20 @Ruhanga does it seems all good for you ? Do we need to modify other points :) |
|
Looks good to me |
|
@rbuisson do you believe we can merge so that I can test the integration in our deployments ? |
|
@wluyima I believe you should add your review here ( it seems you have 2 accounts :)) |
|
Thanks. We still need an approval from a maintainer. @Ruhanga @VaishSiddharth @wluyima or @rbuisson is it on you ? |
mks-d
changed the title
odoo.dob.field and odoo.weight.field properties and 'patientVoided' → 'voided'.
Fixed property access for:
Fixed property name for patient voided.