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

Migration to Rename Skill and Add New Skill #1525

Merged
merged 2 commits into from
Aug 24, 2023
Merged

Conversation

Ashesh3
Copy link
Member

@Ashesh3 Ashesh3 commented Aug 17, 2023

Fixes ohcnetwork/care_fe#6091
Fixes ohcnetwork/care_fe#6090

This PR introduces a new migration for the 'users' app to address two tasks:

  1. Rename Skill: We had a typo in one of the skill names ("Genreal Surgeon"). This migration corrects it to "General Surgeon". The migration is designed to handle the case where the Skill object doesn't exist, preventing any potential exceptions.

  2. Add New Skill: This migration also adds a new skill named "General Medicine" to the Skill model. It uses Django's get_or_create() method to ensure that the new skill is only added if it doesn't already exist, preventing any duplicate entries.

@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.

How did we populate skills in the first place?

This could end up creating this skill on a fresh DB right during migrations right?

And later when we populate all skills on a new instance we would end up having a duplicate right? (Since skill with the typo one is added during population after the corrected one was added during migration)

@Ashesh3
Copy link
Member Author

Ashesh3 commented Aug 17, 2023

How did we populate skills in the first place?

This could end up creating this skill on a fresh DB right during migrations right?

And later when we populate all skills on a new instance we would end up having a duplicate right? (Since skill with the typo one is added during population after the corrected one was added during migration)

The skills for existing deployments were loaded in a migration: https://github.com/coronasafe/care/pull/1176/files#diff-abf84bc2d0fa38116d391f2eb563939f4bb958dbc2c19d72d774e1bff6390070R58-R59

Maybe we should separately load the Skills to the database from a JSON files like we do for the states for new deployments. This PR will correct the typo in existing deployments.

@rithviknishad
Copy link
Member

@Ashesh3 The migrations file that you've referred to seems to be part of old migrations and not present in the squashed/new migrations.

0 Skills present on a fresh instance :/

@Ashesh3
Copy link
Member Author

Ashesh3 commented Aug 17, 2023

@Ashesh3 The migrations file that you've referred to seems to be part of old migrations and not present in the squashed/new migrations.

0 Skills present on a fresh instance :/

I am sure this isn't the expected behavior. We need the same set of skills on every instance. How about we ship this migration (since this is a P1 issue) to fix the typo in existing instances then integrate loading skills into the load_data management command?

Edit: Otherwise, let's just do the latter approach if we have the time and discard this PR.

@Ashesh3
Copy link
Member Author

Ashesh3 commented Aug 23, 2023

We have decided to merge this for now. An additional PR will be created later to move loading skills into the load_data management comment.

@Ashesh3
Copy link
Member Author

Ashesh3 commented Aug 23, 2023

@vigneshhari could you review this?

…937.py

Co-authored-by: Aakash Singh <mail@singhaakash.dev>
@vigneshhari vigneshhari merged commit 20a575d into master Aug 24, 2023
5 checks passed
@vigneshhari vigneshhari deleted the user-skill-change branch August 24, 2023 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 High priority; urgent reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User skill addition Rename "Genreal Surgeon" to "General Surgeon" in user skills
4 participants