Skip to content

Remove recovery from patient disease status#541

Closed
divyagar wants to merge 1 commit intoohcnetwork:masterfrom
divyagar:disease_status
Closed

Remove recovery from patient disease status#541
divyagar wants to merge 1 commit intoohcnetwork:masterfrom
divyagar:disease_status

Conversation

@divyagar
Copy link
Copy Markdown
Contributor

Please check this issue for information.

@divyagar divyagar requested a review from a team as a code owner April 27, 2021 05:04
name='disease_status',
field=models.IntegerField(blank=True, choices=[(1, 'SUSPECTED'), (2, 'POSITIVE'), (3, 'NEGATIVE'), (4, 'RECOVERED'), (5, 'EXPIRED')], default=1, verbose_name='Disease Status'),
),
migrations.AlterField(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

]

operations = [
migrations.AlterField(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

from care.facility.models.patient import PatientRegistration


def change_disease_status(prev, new, qs):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Function change_disease_status has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.

Copy link
Copy Markdown
Member

@vigneshhari vigneshhari left a comment

Choose a reason for hiding this comment

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

Changes Required

RECOVERED = 5
EXPIRED = 6
RECOVERED = 4
EXPIRED = 5
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Never use an old choice integer for something else, use another integer

print(patient.disease_status)
return failed


Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we can just do patient.filter(disease_status=5).update(disease_status=4) right ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we can just do patient.filter(disease_status=5).update(disease_status=4) right ?

Yes we can but then we will not know updation of which rows are failed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

there will be one single mysql query, what failure are we talking about here ?

Copy link
Copy Markdown
Member

@vigneshhari vigneshhari left a comment

Choose a reason for hiding this comment

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

Changes Requested

print(patient.disease_status)
return failed


Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

there will be one single mysql query, what failure are we talking about here ?

@abhiabhi94
Copy link
Copy Markdown
Contributor

Hey @vigneshhari, I think this change to 'RECOVERY' to RECOVERED should ideally be handled by a custom migration. This way, we won't necessarily have to run the command at every end(production as well as for all developers).

If you agree, I can do that and then this one only needs to handle the addition of another value to the model.

]

operations = [
migrations.AlterField(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

name='disease_status',
field=models.IntegerField(blank=True, choices=[(1, 'SUSPECTED'), (2, 'POSITIVE'), (3, 'NEGATIVE'), (5, 'RECOVERED'), (6, 'EXPIRED')], default=1, verbose_name='Disease Status'),
),
migrations.AlterField(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

@qlty-cloud-legacy
Copy link
Copy Markdown

Code Climate has analyzed commit 783270f and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

View more on Code Climate.

@sonarqubecloud
Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!

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

No Coverage information No Coverage information
0.0% 0.0% Duplication

@vigneshhari
Copy link
Copy Markdown
Member

@abhiabhi94 you can take over this issue then, @divyagar after he is done with his work review it.

@vigneshhari
Copy link
Copy Markdown
Member

@abhiabhi94 close this PR when you start working on the issue

@abhiabhi94
Copy link
Copy Markdown
Contributor

abhiabhi94 commented Apr 27, 2021

Do we need to add an extra field EXPIRED?
Edit: EXPIRED is no longer present in this pull request.

@vigneshhari
Copy link
Copy Markdown
Member

ohcnetwork/care_fe#988
This is the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants