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

Soft delete assets from deleted facilities/hospitals #1996

Merged
merged 26 commits into from
Sep 23, 2024

Conversation

hrit2773
Copy link
Contributor

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

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.

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

@hrit2773
Copy link
Contributor Author

hrit2773 commented Mar 21, 2024

@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
Copy link
Contributor Author

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

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.

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

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

@rithviknishad
Copy link
Member

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

@hrit2773
Copy link
Contributor Author

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

@hrit2773
Copy link
Contributor Author

hrit2773 commented Mar 24, 2024

@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

@hrit2773 hrit2773 marked this pull request as draft March 24, 2024 10:25
@hrit2773 hrit2773 requested a review from rithviknishad March 24, 2024 10:26
Copy link

codecov bot commented Mar 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.41%. Comparing base (12afd7a) to head (5850871).
Report is 5 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1996      +/-   ##
===========================================
+ Coverage    65.38%   65.41%   +0.02%     
===========================================
  Files          242      242              
  Lines        13976    13988      +12     
  Branches      1937     1938       +1     
===========================================
+ Hits          9138     9150      +12     
  Misses        4455     4455              
  Partials       383      383              

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

@hrit2773 hrit2773 marked this pull request as ready for review March 24, 2024 10:50
care/facility/models/facility.py Outdated Show resolved Hide resolved
@hrit2773
Copy link
Contributor Author

@sainak plz review the commit...

@hrit2773
Copy link
Contributor Author

@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 rithviknishad requested a review from sainak March 27, 2024 04:26
@rithviknishad
Copy link
Member

rithviknishad commented Mar 27, 2024

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

@hrit2773
Copy link
Contributor Author

@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

care/facility/tasks/soft_delete_assets.py Outdated Show resolved Hide resolved
@rithviknishad rithviknishad requested a review from sainak March 29, 2024 09:31
celerybeat_schedule.py Outdated Show resolved Hide resolved
@hrit2773
Copy link
Contributor Author

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

@hrit2773
Copy link
Contributor Author

hrit2773 commented Apr 1, 2024

@sainak can you review the changes

@hrit2773 hrit2773 requested a review from a team as a code owner April 4, 2024 08:16
@hrit2773
Copy link
Contributor Author

hrit2773 commented Apr 4, 2024

@rithviknishad @sainak lets close this issue today

@hrit2773
Copy link
Contributor Author

@sainak @vigneshhari can you review this PR

@vigneshhari
Copy link
Member

Why is this a background job? cant we just soft delete assets if the facility is archived? we cant add assets to a deleted facility right? @sainak

@hrit2773
Copy link
Contributor Author

Why is this a background job? cant we just soft delete assets if the facility is archived? we cant add assets to a deleted facility right? @sainak

@vigneshhari I did that initially but @sainak asked me to run an cron job

@sainak sainak self-assigned this Sep 22, 2024
@vigneshhari vigneshhari merged commit d6f9b51 into ohcnetwork:develop Sep 23, 2024
1 check failed
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.

Asset list- shows assets from deleted hospitals
4 participants