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

2002 async deletion #2004

Merged
merged 10 commits into from
Jul 21, 2021
Merged

2002 async deletion #2004

merged 10 commits into from
Jul 21, 2021

Conversation

abhidg
Copy link
Contributor

@abhidg abhidg commented Jul 16, 2021

This doesn't have the workflow/Batch action to actually periodically run, will add it once this is merged and list is set to True on prod/dev.

@codecov-commenter
Copy link

codecov-commenter commented Jul 16, 2021

Codecov Report

Merging #2004 (5a7e13d) into main (cf2689b) will decrease coverage by 8.67%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2004      +/-   ##
==========================================
- Coverage   75.42%   66.74%   -8.68%     
==========================================
  Files          24      129     +105     
  Lines         940     4421    +3481     
  Branches      150     1181    +1031     
==========================================
+ Hits          709     2951    +2242     
- Misses        231     1470    +1239     
Impacted Files Coverage Δ
...rving/data-service/src/controllers/preprocessor.ts 95.71% <ø> (ø)
data-serving/data-service/src/index.ts 95.45% <ø> (ø)
data-serving/data-service/src/model/case.ts 100.00% <ø> (ø)
verification/curator-service/api/src/index.ts 88.52% <ø> (-0.28%) ⬇️
...curator-service/ui/src/components/BulkCaseForm.tsx 10.21% <ø> (ø)
...ion/curator-service/ui/src/components/CaseForm.tsx 30.55% <ø> (ø)
data-serving/data-service/src/controllers/case.ts 82.02% <100.00%> (ø)
...ion/curator-service/api/src/controllers/sources.ts 65.94% <0.00%> (-16.22%) ⬇️
...rving/data-service/src/geocoding/remoteGeocoder.ts 100.00% <0.00%> (ø)
...n/curator-service/ui/src/components/TermsOfUse.tsx 25.00% <0.00%> (ø)
... and 103 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf2689b...5a7e13d. Read the comment docs.

abhidg added 2 commits July 20, 2021 15:04
Deletion will be done asynchronously by a periodic script, so
this is not required now.

Part of #2002
@abhidg abhidg force-pushed the 2002-async-deletion branch from 9b48185 to cce3dc7 Compare July 20, 2021 15:54
@abhidg abhidg force-pushed the 2002-async-deletion branch from cce3dc7 to a4bc559 Compare July 20, 2021 16:09
@abhidg abhidg force-pushed the 2002-async-deletion branch 3 times, most recently from 29ba3dd to a7757b9 Compare July 20, 2021 21:46
@abhidg abhidg marked this pull request as ready for review July 20, 2021 22:01
@abhidg abhidg requested review from iamleeg and jim-sheldon July 21, 2021 07:16
@abhidg abhidg force-pushed the 2002-async-deletion branch 2 times, most recently from 32a55c0 to ddb049e Compare July 21, 2021 07:21
@@ -0,0 +1 @@
pymongo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we want to use poetry everywhere now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(besides which, it'd be good to pin the version)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer requirements.txt for simple one liner scripts with few dependencies, can change to poetry if we want to maintain consistency. Advantage of requirements.txt is the setup code is one-liner unlike poetry which uses a curl | python style of installation.

@iamleeg
Copy link
Contributor

iamleeg commented Jul 21, 2021

It doesn't look like there's any default value for list supplied anywhere in the API layer. Should there be? I'm guessing it should be false (or the type should be bool?). Then no cases are displayed until the periodic script has fixed up the values by connecting them to the correct uploads. And when we land this, some manual step will need to list:true all the existing cases.

Is that correct?

@abhidg abhidg force-pushed the 2002-async-deletion branch from ddb049e to 5a7e13d Compare July 21, 2021 15:40
@abhidg
Copy link
Contributor Author

abhidg commented Jul 21, 2021

It doesn't look like there's any default value for list supplied anywhere in the API layer. Should there be? I'm guessing it should be false (or the type should be bool?).

Currently there is no default value. Only list=true is considered true; this avoids changing a lot of code which has no knowledge of the list parameter; with the idea being that prune_uploads orchestrates the line list marking by itself. The curator UI has been changed to use list=true when adding or uploading cases. I couldn't see any optional type marker for the current fields, it seems that is set using required: true in the schema.

Then no cases are displayed until the periodic script has fixed up the values by connecting them to the correct uploads. And when we land this, some manual step will need to list:true all the existing cases.

Yes, tracking this in #2002

@abhidg abhidg requested a review from iamleeg July 21, 2021 15:46
@abhidg abhidg requested a review from jim-sheldon July 21, 2021 15:46
@abhidg abhidg merged commit df84024 into main Jul 21, 2021
@abhidg abhidg deleted the 2002-async-deletion branch July 21, 2021 17:10
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.

4 participants