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

OP-1199 - Voucher Scheme new features (Subservices and subitems) #44

Merged
merged 27 commits into from
Feb 16, 2024

Conversation

kevel-dev
Copy link
Contributor

No description provided.

@sonarcloud
Copy link

sonarcloud bot commented Aug 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 17 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@kevel-dev kevel-dev changed the title Cheque santé to merge OP-1199 - Voucher Scheme new features (Subservices and subitems) Aug 3, 2023
@delcroip
Copy link
Member

delcroip commented Aug 9, 2023

can you please fix the TC
test_mutation_update_service
test_no_right_services_query

@delcroip
Copy link
Member

delcroip commented Aug 9, 2023

And add test case for your features to make sure no one will break them

@delcroip
Copy link
Member

delcroip commented Aug 9, 2023

poke @mngoe

@sonarcloud
Copy link

sonarcloud bot commented Sep 15, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 17 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@kevel-dev
Copy link
Contributor Author

@delcroip i have added some test cases, all seem ok now

Copy link
Member

@delcroip delcroip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check my comments, I don't see anything blocking but the required GQL element,

]
for field in fields:
if hasattr(item_service, field):
setattr(item_service, field, None)


def update_or_create_item_or_service(data, user, item_service_model):
items = data.pop('items') if 'items' in data else []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to check here, for the product this code lead to removal of all item/services if the item and services were not loaded.
I aim to fix the product by having the default as None and in that case I could assume that no change was requested, this approach have the limitation that the FE should send the section items/services only when loaded beforehand; (FE part not implemented in Product therefore I consider [] as no change)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not blocking for the merge

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

medical/gql_mutations.py Outdated Show resolved Hide resolved
medical/gql_mutations.py Outdated Show resolved Hide resolved
medical/gql_mutations.py Outdated Show resolved Hide resolved
medical/gql_mutations.py Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this file required ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not. Just temporary files from Mac OS not present in the gitignore, sorry.
@kevel-dev Please remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -164,12 +166,14 @@ class Service(VersionedModel, ItemOrService):
code = models.CharField(db_column='ServCode', max_length=6)
name = models.CharField(db_column='ServName', max_length=100)
type = models.CharField(db_column='ServType', max_length=1)
packagetype = models.CharField(db_column='ServPackageType', max_length=1, default="S")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, a textchoice class will be more restrictive therefore avoid errors

here an example https://github.com/openimis/openimis-be-payroll_py/blob/3a862282e081356c1fa03a6868c606c294855894/payroll/models.py#L10

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

level = models.CharField(db_column='ServLevel', max_length=1)
price = models.DecimalField(db_column='ServPrice', max_digits=18, decimal_places=2)
maximum_amount = models.DecimalField(db_column='MaximumAmount', max_digits=18, decimal_places=2, blank=True, null=True)
care_type = models.CharField(db_column='ServCareType', max_length=1)
frequency = models.SmallIntegerField(db_column='ServFrequency', blank=True, null=True)
patient_category = models.SmallIntegerField(db_column='ServPatCat')
patient_category = models.SmallIntegerField(db_column='ServPatCat', default="15")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default should have a CONSTANT to make the code more readable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@delcroip
Copy link
Member

@kevel-dev @mngoe , in nepal we have a status that can be set to dead, there is a reason that use an external table InsureeStatusReason so somehow it replace your changes

medical/models.py Show resolved Hide resolved
@delcroip delcroip merged commit b1145ee into openimis:develop Feb 16, 2024
1 check passed
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.

None yet

4 participants