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

Occupation dropdown backend #2055

Merged
merged 17 commits into from
Apr 22, 2024
Merged

Conversation

hrit2773
Copy link
Contributor

@hrit2773 hrit2773 commented Apr 4, 2024

Proposed Changes

  • Changed the enum class

@coronasafe/care-backend-maintainers @coronasafe/care-backend-admins

Copy link
Member

@rithviknishad rithviknishad left a comment

Choose a reason for hiding this comment

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

Missing migrations. Run python manage.py makemigrations

Makefile Outdated Show resolved Hide resolved
@hrit2773
Copy link
Contributor Author

hrit2773 commented Apr 5, 2024

@rithviknishad I changed this in my windows machine just to run the command and pushed it by mistake I changed it back and i guess its fine now added migration also

(8, "Business_related"),
(9, "ENGINEER"),
(10, "TEACHER"),
(11, "Other_Professions"),
Copy link
Member

Choose a reason for hiding this comment

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

Do we need both "Other_Pressions" and "OTHERS"

Copy link
Contributor Author

@hrit2773 hrit2773 Apr 5, 2024

Choose a reason for hiding this comment

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

@rithviknishad maybe we can consider other skilled professions that was mentioned in the dropdown suggstions list so i added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rithviknishad in the frontend Other professional occupations were mentioned and professional occupation is a separate type and the corresponding value is OTHER_PROFESSIONS not to be confused with OTHERS and its not the same. This is needed.

Copy link
Member

Choose a reason for hiding this comment

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

Rename it in backend too. It could lead to confusions.

Also change to use uppercase for all

@hrit2773
Copy link
Contributor Author

hrit2773 commented Apr 5, 2024

@rithviknishad I capitalized the names and also capitalized in the frontend

@hrit2773 hrit2773 requested a review from rithviknishad April 5, 2024 06:38
@sainak
Copy link
Member

sainak commented Apr 8, 2024

@hrit2773 delete the migrations and rerun makemigrations

@hrit2773
Copy link
Contributor Author

@sainak @rithviknishad plz review this pr

Comment on lines 573 to 599
OTHERS = 6
HEALTHCARE_PRACTITIONER = 6
PARADEMICS = 7
BUSINESS_RELATED = 8
ENGINEER = 9
TEACHER = 10
OTHER_PROFESSIONAL_OCCUPATIONS = 11
OFFICE_ADMINISTRATIVE = 12
CHEF = 13
PROTECTIVE_SERVICE = 14
HOSPITALITY = 15
CUSTODIAL = 16
CUSTOMER_SERVICE = 17
SALES_SUPERVISOR = 18
RETAIL_SALES_WORKER = 19
INSURANCE_SALES_AGENT = 20
SALES_REPRESENTATIVE = 21
REAL_ESTATE = 22
CONSTRUCTION_EXTRACTION = 23
AGRI_NATURAL = 24
PRODUCTION_OCCUPATION = 25
PILOT_FLIGHT = 26
VEHICLE_DRIVER = 27
MILITARY = 28
HOMEMAKER = 29
OTHERS = 30
UNKNOWN = 31
NOT_APPLICABLE = 32
Copy link
Member

Choose a reason for hiding this comment

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

OTHERS was previously 6 and is now 30. And 6 is now Healthcare Practitioner. This would mean, all previously entered patients with occupation as "Other" before this PR would become a Healthcare Practitioner once this is merged.

@hrit2773
Copy link
Contributor Author

hrit2773 commented Apr 13, 2024

@rithviknishad yes you are right OTHERS would be misread as HEALTHCARE PRACTIOTIONER so changed it. And i made changes in the frontend also

@hrit2773 hrit2773 requested a review from rithviknishad April 13, 2024 05:40
@rithviknishad rithviknishad requested a review from sainak April 16, 2024 10:48
@nihal467
Copy link
Member

@hrit2773 there is migration issue, fix it

@hrit2773
Copy link
Contributor Author

@nihal467 but it's working fine when I'm running it the occupation is being added and displayed properly. Should I re-run the migrations?

@hrit2773
Copy link
Contributor Author

@nihal467 can you check it now maybe there were some conflicting migrations before

@vigneshhari vigneshhari merged commit 8b7baf5 into ohcnetwork:develop Apr 22, 2024
1 of 3 checks 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.

5 participants