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

Remove notes field, fixes #2673 #2680

Merged
merged 6 commits into from
Apr 29, 2022
Merged

Remove notes field, fixes #2673 #2680

merged 6 commits into from
Apr 29, 2022

Conversation

abhidg
Copy link
Contributor

@abhidg abhidg commented Apr 25, 2022

This removes the notes field in the UI, API and exports. It doesn't actually delete the notes field in the database yet, which we can do at a later point, or create a separate access control to only give access to notes.

@abhidg abhidg changed the title 2673 remove notes Remove notes field, fixes #2673 Apr 25, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 25, 2022

Codecov Report

Merging #2680 (3ccce55) into main (a6f5b73) will decrease coverage by 4.16%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2680      +/-   ##
==========================================
- Coverage   62.15%   57.98%   -4.17%     
==========================================
  Files          17       94      +77     
  Lines        1197     3180    +1983     
  Branches      189     1021     +832     
==========================================
+ Hits          744     1844    +1100     
- Misses        453     1336     +883     
Impacted Files Coverage Δ
...ion/curator-service/ui/src/components/ViewCase.tsx 80.00% <ø> (ø)
...ation/curator-service/api/src/controllers/cases.ts
verification/curator-service/api/src/index.ts
...ation/curator-service/api/src/util/validate-env.ts
...on/curator-service/api/src/clients/email-client.ts
...fication/curator-service/api/src/model/database.ts
...ion/curator-service/api/src/controllers/uploads.ts
...erification/curator-service/api/src/util/logger.ts
...ation/curator-service/api/src/controllers/users.ts
...erification/curator-service/api/src/model/token.ts
... and 102 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 112215e...3ccce55. Read the comment docs.

@abhidg abhidg force-pushed the 2673-remove-notes branch 3 times, most recently from d9e6e98 to cb3d370 Compare April 27, 2022 10:38
@abhidg abhidg force-pushed the 2673-remove-notes branch 2 times, most recently from 789f79f to 58d15f9 Compare April 27, 2022 15:57
Remove bit of test that checks whether the first hit of the text
search term is in highlighted class, as it actually gets a
MuiChip-label pill.
@abhidg abhidg marked this pull request as ready for review April 28, 2022 15:58
Copy link
Contributor

@iamleeg iamleeg left a comment

Choose a reason for hiding this comment

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

Approved with some optional bonus tasks :)

Comment on lines 80 to +81
delete dto.restrictedNotes;
delete dto.notes;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is a "not yet" task but we could migrate restrictedNotes back over to notes, as all notes are now restricted. Then we would be able to build a single approach to exposing notes later.

["_id","caseReference.additionalSources","caseReference.sourceEntryId","caseReference.sourceId","caseReference.sourceUrl","caseReference.uploadIds","caseReference.verificationStatus","demographics.ageRange.end","demographics.ageRange.start","demographics.ethnicity","demographics.gender","demographics.nationalities","demographics.occupation","events","location.administrativeAreaLevel1","location.administrativeAreaLevel2","location.administrativeAreaLevel3","location.country","location.geoResolution","location.geometry.latitude","location.geometry.longitude","location.name","location.place","location.query","pathogens","preexistingConditions.hasPreexistingConditions","preexistingConditions.values","revisionMetadata.creationMetadata.curator","revisionMetadata.creationMetadata.date","revisionMetadata.creationMetadata.notes","revisionMetadata.editMetadata.curator","revisionMetadata.editMetadata.date","revisionMetadata.editMetadata.notes","revisionMetadata.revisionNumber", "SGTF", "symptoms.status","symptoms.values","transmission.linkedCaseIds","transmission.places","transmission.routes","travelHistory.travel.dateRange.end","travelHistory.travel.dateRange.start","travelHistory.travel.location.name","travelHistory.travel.methods","travelHistory.travel.purpose","travelHistory.traveledPrior30Days","vaccines.0.name","vaccines.0.batch","vaccines.0.date","vaccines.0.sideEffects","vaccines.1.name","vaccines.1.batch","vaccines.1.date","vaccines.1.sideEffects","vaccines.2.name","vaccines.2.batch","vaccines.2.date","vaccines.2.sideEffects","vaccines.3.name","vaccines.3.batch","vaccines.3.date","vaccines.3.sideEffects",""]
Copy link
Contributor

Choose a reason for hiding this comment

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

this would be easier to diff if we put each element on its own line :)

@@ -200,41 +207,21 @@ describe('GET', () => {
.expect(200);
expect(res.body.cases).toHaveLength(1);
});
it('should query results', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any text that the query should work over instead of notes, or is there any free-text search facility at all in the platform now? If not we can also drop the text index, which will speed up ingestion and save 13.5G in the prod cluster (the main collection is only 10.1G!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, in #2687

@abhidg abhidg merged commit da2b105 into main Apr 29, 2022
@abhidg abhidg deleted the 2673-remove-notes branch April 29, 2022 11: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.

None yet

3 participants