Delete incomplete file uploads

[yaswanthsaivendra]

Proposed Changes

• delete incomplete file uploads on s3 as well as their entries on db.

Associated Issue

• Fixes Peroidic tasks to remove incomplete file uploads #1326

@coronasafe/code-reviewers

Merge Checklist

• Tests added/fixed
• Update docs in /docs
• Linting Complete

[yaswanthsaivendra]

@rithviknishad @vigneshhari review pls

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[yaswanthsaivendra]

Updated the code - to make bulk delete from s3, thereby reducing the number of api requests.

[codecov-commenter]

Codecov Report

Patch coverage: 40.00% and project coverage change: -0.06 ⚠️

Comparison is base (96222fc) 56.71% compared to head (897ec15) 56.65%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1329      +/-   ##
==========================================
- Coverage   56.71%   56.65%   -0.06%     
==========================================
  Files         199      195       -4     
  Lines        9905     9866      -39     
  Branches     1654     1656       +2     
==========================================
- Hits         5618     5590      -28     
+ Misses       4232     4222      -10     
+ Partials       55       54       -1     

Impacted Files	Coverage Δ
care/facility/models/file_upload.py	65.15% <40.00%> (-2.07%)	⬇️

... and 106 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[yaswanthsaivendra]

@sainak Review please

[vigneshhari]

@khavinshankar @sainak can you guys check if this affects discharge summaries or ABDM uploads.

[sainak]

@vigneshhari it affects all uploaded files

[vigneshhari]

@sainak I know that it affects all uploaded files, i want to know if it affects any of our existing workflows, since we use files in HCX and discharge summaries and other stuff like that.

[khavinshankar]

@vigneshhari this shouldn't be a problem for HCX or discharge summary.

[vigneshhari]

Please add unit tests, unit tests are required for every PR's.

[vigneshhari]

Updated the code - to make bulk delete from s3, thereby reducing the number of api requests.

Can you check if the bulk API has an upper limit, This might create an issue when the first time it's run.

[yaswanthsaivendra]

Updated the code - to make bulk delete from s3, thereby reducing the number of api requests.

Can you check if the bulk API has an upper limit, This might create an issue when the first time it's run.

Yes, the upper limit is 1000 keys per a request. I updated code to handle the keys in batches. Btw just to confirm , our bucket is not object versioned rgt?

[yaswanthsaivendra]

Please add unit tests, unit tests are required for every PR's.

Actually the reason I haven't added the tests is that I haven't tests for most of the existing code expect for some APIs in /care/facility/tests/ and for 1 or 2 models in care/facility/models/tests/ . Am I missing something?
Can u suggest me a good place where can I include tests for the celery_task and bulk_delete_objects ?

[vigneshhari]

@mathew-alex are S3 objects versioned in any deployment ?

[vigneshhari]

Please add unit tests, unit tests are required for every PR's.

Actually the reason I haven't added the tests is that I haven't tests for most of the existing code expect for some APIs in /care/facility/tests/ and for 1 or 2 models in care/facility/models/tests/ . Am I missing something? Can u suggest me a good place where can I include tests for the celery_task and bulk_delete_objects ?

We don't have unit tests for most ( >90% ) of the code base, but recently we started enforcing unit testing, you can create unit tests like existing ones and call the celery methods directly to test them. You have the option to test the S3 operations via localstack, if you don't want to go that route, you can mock the S3 operations.

[yaswanthsaivendra]

Please add unit tests, unit tests are required for every PR's.

Actually the reason I haven't added the tests is that I haven't tests for most of the existing code expect for some APIs in /care/facility/tests/ and for 1 or 2 models in care/facility/models/tests/ . Am I missing something? Can u suggest me a good place where can I include tests for the celery_task and bulk_delete_objects ?

We don't have unit tests for most ( >90% ) of the code base, but recently we started enforcing unit testing, you can create unit tests like existing ones and call the celery methods directly to test them. You have the option to test the S3 operations via localstack, if you don't want to go that route, you can mock the S3 operations.

@vigneshhari Okay, added the tests. can you please check now!

[yaswanthsaivendra]

@mathew-alex are S3 objects versioned in any deployment ?

Hey I have gone through the code base quite a few times, wherever s3 configs or s3 methods made use of. No where Versioning details are mentioned. if versioning exists, it has to dealt while creation and retrieving of objects. So, ig none of our existing deployments are using versioned s3 buckets.

[mathew-alex]

@mathew-alex are S3 objects versioned in any deployment ?

Hey I have gone through the code base quite a few times, wherever s3 configs or s3 methods made use of. No where Versioning details are mentioned. if versioning exists, it has to dealt while creation and retrieving of objects. So, ig none of our existing deployments are using versioned s3 buckets.

@yaswanthsaivendra - bucket versioning is enabled in cloud bucket settings.

[vigneshhari]

@yaswanthsaivendra please reply to @mathew-alex 's comment