Soft delete assets from deleted facilities/hospitals #1996

hrit2773 · 2024-03-20T18:27:13Z

Fixes #1982 issue, enabled soft delete of assets by overriding delete method in facility model

rithviknishad

No changes to the fields in the model were made. But why are there migrations for it?

hrit2773 · 2024-03-21T01:11:55Z

@rithviknishad I was initially using models.cascade but later I realised I need to perform soft delete so I changed it back to original ones so that's why you can see those redundant migrations.But the functionality is working. Should I remove those redundant migrations?

hrit2773 · 2024-03-21T02:15:30Z

@rithviknishad I made a second commit i removed those redundant migration history so effectively i just added the delete method in facility model

rithviknishad

Custom migration needed to delete existing such assets of deleted facility.

https://docs.djangoproject.com/en/5.0/ref/migration-operations/#runpython

rithviknishad · 2024-03-21T05:29:34Z

Also test cases needs to be added. It'd be great if you could follow the PR template 😄

hrit2773 · 2024-03-21T05:33:48Z

@rithviknishad ya sure I'll make the changes by today

hrit2773 · 2024-03-24T07:57:26Z

@rithviknishad I added custom migrations and also added api test case but the tests are not running in my machine because im using windows and im probably getting some connection errors with the test db like some dbus connection errors and no such bucket found. It is giving a failed test for medibase api maybe some db connection issues in windows

codecov · 2024-03-24T10:38:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.38%. Comparing base (51ce81e) to head (1cfed7f).
Report is 61 commits behind head on develop.

❗ Current head 1cfed7f differs from pull request most recent head 691f9e8. Consider uploading reports for the commit 691f9e8 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1996      +/-   ##
===========================================
+ Coverage    62.20%   62.38%   +0.18%     
===========================================
  Files          221      221              
  Lines        12204    12229      +25     
  Branches      1742     1746       +4     
===========================================
+ Hits          7591     7629      +38     
+ Misses        4305     4284      -21     
- Partials       308      316       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sainak · 2024-03-25T15:07:51Z

care/facility/models/facility.py

+    def delete(self, *args, **kwargs):
+        from care.facility.models.asset import Asset, AssetLocation
+
+        Asset.objects.filter(current_location__facility=self).update(deleted=True)
+        super().delete(self, *args, **kwargs)
+


this will skip

care/care/facility/models/asset.py

Lines 175 to 179 in ce7073e

def delete(self, *args, **kwargs):

from care.facility.models.bed import AssetBed

AssetBed.objects.filter(asset=self).update(deleted=True)

super().delete(*args, **kwargs)

a better approach would be to make a cron job that regularly checks for deleted objects and recursively soft deletes related objects

@sainak but if you check recursively it'll slow down and degrade the performance

@sainak and that cron job would run in Linux but not in windows I guess so I can't test it instead what I can do is to delete it in an atomic way maybe

by cron job, I mean celerybeat cron task

@sainak ok how frequent the check should be every minute or every hour or every 24 hrs?

let's run it every 24 hour

@sainak okay I will do it

hrit2773 · 2024-03-25T19:34:23Z

@sainak plz review the commit...

hrit2773 · 2024-03-26T05:23:03Z

@sainak @rithviknishad i added the cron job and should i remove the tests also for soft delete that i created because now that wont work right and maybe the custom migration is also not needed because every midnight the assets will be soft deleted and should i add logging infos and warnings in the task function? plz review it and tell me if any changes required

rithviknishad · 2024-03-27T04:26:42Z

@hrit2773 You can remove the custom migration since it'd be taken care by the celery task itself right.

sainak · 2024-03-27T04:28:28Z

care/facility/tasks/soft_delete_assets.py

+    facilities = Facility.objects.all().values_list("name", flat=True)
+    Asset.objects.exclude(current_location__facility__name__in=facilities).update(


you can directly use the id instead of name

@sainak yes yes we can I'll do it

hrit2773 · 2024-03-27T04:30:38Z

@hrit2773 You can remove the custom migration since it'd be taken care by the celery task itself right.

@rithviknishad yes I'll remove it and I'll remove the api test case also because I feel that is not needed

hrit2773 · 2024-03-27T18:23:30Z

@rithviknishad @sainak made the required changes plz review these changes

rithviknishad · 2024-03-29T09:31:18Z

care/facility/tasks/soft_delete_assets.py

+def soft_delete_assets_schedule():
+    facilities = Facility.objects.all().values_list("external_id", flat=True)
+    Asset.objects.exclude(
+        current_location__facility__external_id__in=facilities
+    ).update(deleted=True)


We could simply rely on the PK instead of the UUID right? @sainak

Suggested change

def soft_delete_assets_schedule():

facilities = Facility.objects.all().values_list("external_id", flat=True)

Asset.objects.exclude(

current_location__facility__external_id__in=facilities

).update(deleted=True)

def soft_delete_assets_schedule():

facilities = Facility.objects.all().values_list("id", flat=True)

Asset.objects.exclude(

current_location__facility__id__in=facilities

).update(deleted=True)

@rithviknishad we can do that also but as the filterset class also takes the external_id I considered external id just to avoid any existing wrong entry in the database to spoil the functionality otherwise we can do that

use id as we try to stick with id for internal queries

sainak · 2024-03-30T08:03:08Z

celerybeat_schedule.py

we register tasks in care/facility/tasks/__init__.py

@sainak so should i delete soft_delete_assets.py file and write the same function in the init.py?

no delete the celerybeat_schecule.py and revert changes in celery_app.py

@sainak ok i will do that

@sainak should i change the settings config also because how will it identify that the task should run every 24 hours. I have gone through the init.py file

I should make changes in the init.py also right and in the soft_delete_assets.py i should mark the function using shared task decorator because it should identify that this task should run every 24 hours right?

I will make the changes by today

hrit2773 · 2024-03-30T13:18:08Z

@sainak view the changes.. and also replaced external_id with id

hrit2773 · 2024-04-01T06:25:41Z

@sainak can you review the changes

hrit2773 · 2024-04-04T08:20:23Z

@rithviknishad @sainak lets close this issue today

hrit2773 · 2024-04-24T04:01:54Z

@sainak @vigneshhari can you review this PR

Soft delete assets from deleted facilities/hospitals

b7423f4

rithviknishad requested changes Mar 20, 2024

View reviewed changes

soft delete assets on deleting facility-removed redundant migrations

9025140

rithviknishad requested changes Mar 21, 2024

View reviewed changes

hrit2773 and others added 2 commits March 24, 2024 13:20

added custom migrations and test cases

e95842f

Merge branch 'develop' into develop

ebfed1b

hrit2773 marked this pull request as draft March 24, 2024 10:25

hrit2773 requested a review from rithviknishad March 24, 2024 10:26

Updated soft delete problem

ad66e21

hrit2773 marked this pull request as ready for review March 24, 2024 10:50

hrit2773 and others added 5 commits March 24, 2024 16:32

fix back

1cfed7f

fix

24f3eec

Discard changes to Makefile

cc8b5c5

Discard changes to config/celery_app.py

e9afef8

Merge branch 'develop' into develop

b6f4a99

sainak requested changes Mar 25, 2024

View reviewed changes

hrit2773 and others added 2 commits March 25, 2024 23:21

Merge branch 'coronasafe:develop' into develop

ea7786e

added cron job to soft delete

7330609

rithviknishad requested a review from sainak March 27, 2024 04:26

sainak reviewed Mar 27, 2024

View reviewed changes

changed soft delete assets

5f01058

rithviknishad approved these changes Mar 29, 2024

View reviewed changes

rithviknishad requested a review from sainak March 29, 2024 09:31

sainak reviewed Mar 30, 2024

View reviewed changes

hrit2773 and others added 2 commits March 30, 2024 18:23

Merge branch 'coronasafe:develop' into develop

288422b

Changed

19415d0

hrit2773 requested review from rithviknishad and sainak March 31, 2024 03:51

rithviknishad approved these changes Mar 31, 2024

View reviewed changes

Merge branch 'coronasafe:develop' into develop

9da57c5

hrit2773 requested a review from a team as a code owner April 4, 2024 08:16

sainak and others added 4 commits April 8, 2024 11:50

Discard changes to config/celery_app.py

5a11110

Merge branch 'develop' into develop

9473388

Merge branch 'develop' into develop

02e7382

Merge branch 'develop' into develop

691f9e8

	def delete(self, args, *kwargs):
	from care.facility.models.bed import AssetBed

	AssetBed.objects.filter(asset=self).update(deleted=True)
	super().delete(args, *kwargs)

		facilities = Facility.objects.all().values_list("name", flat=True)
		Asset.objects.exclude(current_location__facility__name__in=facilities).update(

Soft delete assets from deleted facilities/hospitals #1996

Are you sure you want to change the base?

Soft delete assets from deleted facilities/hospitals #1996

Conversation

hrit2773 commented Mar 20, 2024

rithviknishad left a comment • edited Loading

Choose a reason for hiding this comment

hrit2773 commented Mar 21, 2024 • edited Loading

hrit2773 commented Mar 21, 2024

rithviknishad left a comment

Choose a reason for hiding this comment

rithviknishad commented Mar 21, 2024

hrit2773 commented Mar 21, 2024

hrit2773 commented Mar 24, 2024 • edited Loading

codecov bot commented Mar 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrit2773 commented Mar 25, 2024

hrit2773 commented Mar 26, 2024

rithviknishad commented Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrit2773 commented Mar 27, 2024

hrit2773 commented Mar 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sainak Mar 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrit2773 commented Mar 30, 2024

hrit2773 commented Apr 1, 2024

hrit2773 commented Apr 4, 2024

hrit2773 commented Apr 24, 2024

rithviknishad left a comment •

edited

Loading

hrit2773 commented Mar 21, 2024 •

edited

Loading

hrit2773 commented Mar 24, 2024 •

edited

Loading

codecov bot commented Mar 24, 2024 •

edited

Loading

rithviknishad commented Mar 27, 2024 •

edited

Loading

sainak Mar 30, 2024 •

edited

Loading