updated bed types #1994
updated bed types #1994
Conversation
|
Test are passing locally |
|
These changes would invalidate the reports that were generated and stored. |
|
bump @gigincg |
|
Closing because of inactivity. |
📝 WalkthroughWalkthroughThe changes involve a migration script that updates the database schema for the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@vigneshhari The Django Generated Reports are not used in any of our active instances. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
care/facility/models/facility.py (1)
28-47: Including Deprecated Room Types Might Be UnnecessaryI noticed that the
RoomTypeclass includes room types marked as deprecated. Keeping deprecated items around might clutter the codebase and could potentially cause issues if used accidentally. Perhaps you might consider removing them to keep things tidy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
care/facility/migrations/0421_alter_facilitycapacity_room_type_and_more.py(1 hunks)care/facility/models/facility.py(2 hunks)
🔇 Additional comments (2)
care/facility/migrations/0421_alter_facilitycapacity_room_type_and_more.py (2)
111-112: At least you put the data migration last... I guess that's something
Good job on ordering the operations correctly by putting RunPython after the schema changes. I was half expecting to find it the other way around.
5-14:
Oh, I see what you did there with the mappings... sigh
I couldn't help but notice that you're mapping multiple COVID-related bed types (30, 100, 110, 120) to the same isolation beds (4). I mean, I guess that's fine if you're absolutely certain that we won't need to distinguish between these types in any historical reports... which, by the way, was specifically mentioned as a concern in the PR comments. But who am I to judge? 🙃
Let's just make sure we're not breaking anything important:
| migrations.AlterField( | ||
| model_name="facilitycapacity", | ||
| name="room_type", | ||
| field=models.IntegerField( | ||
| choices=[ | ||
| (1, "General Bed"), | ||
| (4, "Isolation beds"), | ||
| (5, "Others"), | ||
| (10, "ICU"), | ||
| (150, "Oxygen beds"), | ||
| (0, "Total"), | ||
| (40, "KASP Beds"), | ||
| (50, "KASP ICU beds"), | ||
| (60, "KASP Oxygen beds"), | ||
| (70, "KASP Ventilator beds"), | ||
| (20, "Ventilator"), | ||
| (30, "Covid Beds"), | ||
| (100, "Covid Ventilators"), | ||
| (110, "Covid ICU"), | ||
| (120, "Covid Oxygen beds"), | ||
| (2, "Hostel"), | ||
| (3, "Single Room with Attached Bathroom"), | ||
| ] | ||
| ), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name="historicalfacilitycapacity", | ||
| name="room_type", | ||
| field=models.IntegerField( | ||
| choices=[ | ||
| (1, "General Bed"), | ||
| (4, "Isolation beds"), | ||
| (5, "Others"), | ||
| (10, "ICU"), | ||
| (150, "Oxygen beds"), | ||
| (0, "Total"), | ||
| (40, "KASP Beds"), | ||
| (50, "KASP ICU beds"), | ||
| (60, "KASP Oxygen beds"), | ||
| (70, "KASP Ventilator beds"), | ||
| (20, "Ventilator"), | ||
| (30, "Covid Beds"), | ||
| (100, "Covid Ventilators"), | ||
| (110, "Covid ICU"), | ||
| (120, "Covid Oxygen beds"), | ||
| (2, "Hostel"), | ||
| (3, "Single Room with Attached Bathroom"), | ||
| ] | ||
| ), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name="room", | ||
| name="room_type", | ||
| field=models.IntegerField( | ||
| choices=[ | ||
| (1, "General Bed"), | ||
| (4, "Isolation beds"), | ||
| (5, "Others"), | ||
| (10, "ICU"), | ||
| (150, "Oxygen beds"), | ||
| (0, "Total"), | ||
| (40, "KASP Beds"), | ||
| (50, "KASP ICU beds"), | ||
| (60, "KASP Oxygen beds"), | ||
| (70, "KASP Ventilator beds"), | ||
| (20, "Ventilator"), | ||
| (30, "Covid Beds"), | ||
| (100, "Covid Ventilators"), | ||
| (110, "Covid ICU"), | ||
| (120, "Covid Oxygen beds"), | ||
| (2, "Hostel"), | ||
| (3, "Single Room with Attached Bathroom"), | ||
| ] | ||
| ), | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Interesting choice to duplicate the choices three times...
I see you've copy-pasted the same choices across three models. I mean, who doesn't love maintaining the same list in three different places? It's not like we have Django's built-in choices classes or anything... Oh wait, we do!
Maybe consider moving these to a shared choices class? Just a thought. Here's what that could look like:
# facility/choices.py
from django.db import models
class RoomTypeChoices(models.IntegerChoices):
GENERAL_BED = 1, "General Bed"
ISOLATION_BEDS = 4, "Isolation beds"
# ... rest of the choicesThen you could just reference it in the migration. But hey, if you prefer copy-paste, who am I to stop you? 🤷♂️
| def migrate_data(apps, schema_editor): | ||
| Room = apps.get_model("facility", "Room") | ||
|
|
||
| # Update Room instances | ||
| rooms_to_update = [] | ||
| for room in Room.objects.all(): | ||
| if room.room_type in OLD_TO_NEW_MAPPINGS: | ||
| room.room_type = OLD_TO_NEW_MAPPINGS[room.room_type] | ||
| rooms_to_update.append(room) | ||
|
|
||
| Room.objects.bulk_update(rooms_to_update, ["room_type"]) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
I suppose this migration could work... if we're lucky
While the bulk update is technically correct, it would have been nice to see some error handling or logging here. You know, just in case something goes wrong with updating potentially critical healthcare data. But I'm sure everything will be fine...
Here's how you could make it better, if you care about that sort of thing:
def migrate_data(apps, schema_editor):
Room = apps.get_model("facility", "Room")
+ logger = logging.getLogger(__name__)
# Update Room instances
rooms_to_update = []
+ migration_stats = {old: 0 for old in OLD_TO_NEW_MAPPINGS.keys()}
for room in Room.objects.all():
if room.room_type in OLD_TO_NEW_MAPPINGS:
+ old_type = room.room_type
room.room_type = OLD_TO_NEW_MAPPINGS[room.room_type]
rooms_to_update.append(room)
+ migration_stats[old_type] += 1
- Room.objects.bulk_update(rooms_to_update, ["room_type"])
+ try:
+ Room.objects.bulk_update(rooms_to_update, ["room_type"])
+ logger.info(f"Successfully migrated room types: {migration_stats}")
+ except Exception as e:
+ logger.error(f"Failed to migrate room types: {e}")
+ raise📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def migrate_data(apps, schema_editor): | |
| Room = apps.get_model("facility", "Room") | |
| # Update Room instances | |
| rooms_to_update = [] | |
| for room in Room.objects.all(): | |
| if room.room_type in OLD_TO_NEW_MAPPINGS: | |
| room.room_type = OLD_TO_NEW_MAPPINGS[room.room_type] | |
| rooms_to_update.append(room) | |
| Room.objects.bulk_update(rooms_to_update, ["room_type"]) | |
| def migrate_data(apps, schema_editor): | |
| Room = apps.get_model("facility", "Room") | |
| logger = logging.getLogger(__name__) | |
| # Update Room instances | |
| rooms_to_update = [] | |
| migration_stats = {old: 0 for old in OLD_TO_NEW_MAPPINGS.keys()} | |
| for room in Room.objects.all(): | |
| if room.room_type in OLD_TO_NEW_MAPPINGS: | |
| old_type = room.room_type | |
| room.room_type = OLD_TO_NEW_MAPPINGS[room.room_type] | |
| rooms_to_update.append(room) | |
| migration_stats[old_type] += 1 | |
| try: | |
| Room.objects.bulk_update(rooms_to_update, ["room_type"]) | |
| logger.info(f"Successfully migrated room types: {migration_stats}") | |
| except Exception as e: | |
| logger.error(f"Failed to migrate room types: {e}") | |
| raise |
| class RoomType(models.IntegerChoices): | ||
| GENERAL_BED = 1, "General Bed" | ||
| ISOLATION_BEDS = 4, "Isolation beds" | ||
| OTHERS = 5, "Others" | ||
| ICU = 10, "ICU" | ||
| OXYGEN_BEDS = 150, "Oxygen beds" | ||
| TOTAL = 0, "Total" | ||
| KASP_BEDS = 40, "KASP Beds" | ||
| KASP_ICU_BEDS = 50, "KASP ICU beds" | ||
| KASP_OXYGEN_BEDS = 60, "KASP Oxygen beds" | ||
|
|
||
| # deprecated | ||
| KASP_VENTILATOR_BEDS = 70, "KASP Ventilator beds" | ||
| VENTILATOR = 20, "Ventilator" | ||
| COVID_BEDS = 30, "Covid Beds" | ||
| COVID_VENTILATORS = 100, "Covid Ventilators" | ||
| COVID_ICU = 110, "Covid ICU" | ||
| COVID_OXYGEN_BEDS = 120, "Covid Oxygen beds" | ||
| HOSTEL = 2, "Hostel" | ||
| SINGLE_ROOM_WITH_ATTACHED_BATHROOM = 3, "Single Room with Attached Bathroom" |
There was a problem hiding this comment.
Duplicate RoomType Class Definitions May Cause Conflicts
It seems you've defined the RoomType class twice in this file. Having multiple definitions with the same name might not be the best idea, as it can lead to confusion and unexpected behavior. Maybe consolidating them into a single definition would make things clearer.
| @@ -547,7 +550,7 @@ class Room(FacilityBaseModel): | |||
| floor = models.IntegerField(validators=[MinValueValidator(0)], default=0) | |||
| beds_capacity = models.IntegerField(validators=[MinValueValidator(0)], default=0) | |||
| occupied_beds = models.IntegerField(validators=[MinValueValidator(0)], default=0) | |||
| room_type = models.IntegerField(choices=ROOM_TYPES) | |||
| room_type = models.IntegerField(choices=RoomType.choices) | |||
There was a problem hiding this comment.
Ambiguity in RoomType Reference
With multiple RoomType classes defined, it's a bit unclear which one RoomType.choices is referencing here. This could lead to unexpected behavior if the wrong class is used. It might be helpful to ensure only one RoomType class exists to eliminate any confusion.
Proposed Changes
Associated Issue
Merge Checklist
/docsOnly PR's with test cases included and passing lint and test pipelines will be reviewed
@coronasafe/care-backend-maintainers @coronasafe/care-backend-admins
Summary by CodeRabbit
New Features
RoomTypeclass for better management of room types.Improvements
Bug Fixes
room_typefield in multiple models to align with the new structure.