Skip to content
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

fix Object Creation error on graphQl object #OP-1670 #187

Merged
merged 2 commits into from
Dec 15, 2023
Merged

Conversation

mngoe
Copy link
Contributor

@mngoe mngoe commented Dec 1, 2023

Please check carefully before merging.

This fix can have some implication we don't know.

This solve the problem in our implementation, but our local policies mobile app is still on testing. Also some we don't implement some kind of use cases (update of insuree for example).

@mngoe mngoe changed the title fix Object Creation error on graphQl object fix Object Creation error on graphQl object #OP-1670 Dec 1, 2023
@delcroip
Copy link
Member

delcroip commented Dec 6, 2023

Thanks,

I understand why you removed id, but why removing uuid too ?

I think we must implement the identifier in json_ext where the mobile uuid/id could be saved along with the server ones @dragos-dobre

@mngoe
Copy link
Contributor Author

mngoe commented Dec 6, 2023

Sorry , I didn't work on that one personnally (and don't have a lot of time to check deeper).

From what I understood, if you define ID/UUID on the graphQL it will try to update an existing object (and as it's not existing in the database already it fail on python side even if it return 200).
As UUID is a default value on models maybe the dev just removed it to avoid complexity.

My developper in charge is on holiday right now, but I will ask him to explain better asap.

@tdethier
Copy link
Member

tdethier commented Dec 7, 2023

So I took a look at this with Benjamin. We believe you can discard this PR because it's not going to solve the issue.
The main reason behind this is that currently the GraphQL calls are synchronous instead of asynchronous, so we'll change to change that and this will solve the issue.

@delcroip
Copy link
Member

delcroip commented Dec 14, 2023

I was looking on the FHIR API ..., for graphql only the id need to be removed, the FE send uuid and it works fine. Sending the id is "dangerous" because it may work: you will update a record that have nothing to do because one record with this very id might have been created in the meantime (id on phone != id on server, that why we had the offline flag on the RESTAPI)

@mngoe Can you check by just removing the id ?

Same may happen with UUID but it is VERY unlikely

[low priority] Ideally we should send a "source/namespace" with the mutation so the server will create the element unless it finds it with that namespace/UUID (FHIR references) Also when sync the phone might find back the resource even if the UUID has been updated

@delcroip
Copy link
Member

I checked create premium gql and it was NOT supporting uuid too

so I created this PR to allow it

openimis/openimis-be-contribution_py#59

br

@kevel-dev
Copy link

@delcroip I retrieved the PR that you submitted and tested it, it now behaves correctly, when i enter an UUID, it is that UUID that is saved in the database. With this we can no cancel the code that we removed on the mobile for the premuim.

On the other hand it seems that you have to do the same work on the insuree (and family), and policy modules because when you enter the UUID in these modules. The system searches for the record with a get(). and when the element is not found the python returns a bug ("Insuree matching query does not exist."). Here is attached a capture of the source code for the insuree module which causes the bug.
Screenshot from 2023-12-15 17-11-32

@delcroip
Copy link
Member

I am doing it, pr should come any minute

@delcroip
Copy link
Member

All PR merged

@delcroip delcroip merged commit c10cac9 into develop Dec 15, 2023
2 checks passed
@kevel-dev
Copy link

Thank you, it seems that you forgot to adjust in the policy module ?

@dragos-dobre dragos-dobre deleted the fixOP-167 branch June 27, 2024 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants