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.

Sign up for GitHub

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

Jump to bottom

Remove notes field, fixes #2673 #2680

Merged
merged 6 commits into from
Apr 29, 2022
Merged

Remove notes field, fixes #2673 #2680

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dda0830
export: remove notes field #2673
abhidg Apr 25, 2022
dee6881
data,curator: remove notes #2673
abhidg Apr 25, 2022
3ccce55
Remove cypress tests that use notes #2673
abhidg Apr 27, 2022
a4a738e
curator(api): Pin mongodb server to 5.0.8
abhidg Apr 27, 2022
2201142
cypress: change highlight test
abhidg Apr 27, 2022
46194bc
data-service: re-format fields.json for easier diffs
abhidg Apr 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions data-serving/data-service/api/openapi.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,10 +816,6 @@ components:
type: integer
format: int32
minimum: 0
notes:
type: string
restrictedNotes:
type: string
revisionMetadata:
type: object
properties:
Expand Down
9 changes: 4 additions & 5 deletions data-serving/data-service/src/controllers/case.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ const dtoFromCase = async (storedCase: LeanDocument<CaseDocument>) => {
delete (dto as unknown as { demographics: { ageBuckets?: [ObjectId] }}).demographics.ageBuckets;
}
delete dto.restrictedNotes;
delete dto.notes;
Comment on lines 80 to +81
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.


return dto;
}
Expand Down Expand Up @@ -117,9 +118,10 @@ export class CasesController {
return;
}

// don't export restricted notes
// don't export any note
c.forEach((aCase: LeanDocument<CaseDocument>) => {
delete aCase.restrictedNotes;
delete aCase.notes;
});

res.json(await Promise.all(c.map(aCase => dtoFromCase(aCase))));
Expand Down Expand Up @@ -242,10 +244,8 @@ export class CasesController {
doc = await cursor.next();
while (doc != null) {
delete doc.restrictedNotes;
delete doc.notes;
const normalizedDoc = denormalizeFields(doc);
if (!doc.hasOwnProperty('notes')) {
normalizedDoc.notes = '';
}
if (!doc.hasOwnProperty('SGTF')) {
normalizedDoc.SGTF = 'NA';
}
Expand Down Expand Up @@ -340,7 +340,6 @@ export class CasesController {
]);

const dtos = await Promise.all(docs.map(dtoFromCase));

logger.info('got results');
// total is actually stored in a count index in mongo, so the query is fast.
// however to maintain existing behaviour, only return the count limit
Expand Down
4 changes: 1 addition & 3 deletions data-serving/data-service/src/model/case.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,6 @@ caseSchema.methods.equalsJSON = function (jsonCase: any): boolean {
_.isEqual(thisJson.exclusionData, other.exclusionData) &&
_.isEqual(thisJson.genomeSequences, other.genomeSequences) &&
_.isEqual(thisJson.location, other.location) &&
_.isEqual(thisJson.notes, other.notes) &&
_.isEqual(thisJson.restrictedNotes, other.restrictedNotes) &&
_.isEqual(thisJson.pathogens, other.pathogens) &&
_.isEqual(
thisJson.preexistingConditions,
Expand All @@ -145,7 +143,7 @@ export type ICase = {
importedCase: unknown;
location: LocationDocument;
revisionMetadata: RevisionMetadataDocument;
notes: string;
notes?: string;
restrictedNotes?: string;
pathogens: [PathogenDocument];
list: boolean;
Expand Down
66 changes: 65 additions & 1 deletion data-serving/data-service/src/model/fields.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,65 @@
["_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","notes","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",""]
[
"_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",
""
]
55 changes: 25 additions & 30 deletions data-serving/data-service/test/controllers/case.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,13 @@ describe('GET', () => {
const res = await request(app).get(`/api/cases/${c._id}`).expect(200);
expect(res.body[0].restrictedNotes).toBeUndefined();
});
it('should not show the notes for a case', async () => {
const c = new Case(minimalCase);
c.notes = 'I want to tell you a secret';
await c.save();
const res = await request(app).get(`/api/cases/${c._id}`).expect(200);
expect(res.body[0].notes).toBeUndefined();
});
it('should convert age bucket to age range', async () => {
const c = new Case(minimalCase);
const bucket = await AgeBucket.findOne({});
Expand Down Expand Up @@ -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

// Simulate index creation used in unit tests, in production they are
// setup by the migrations and such indexes are not present by
// default in the in memory mongo spawned by unit tests.
await mongoose.connection.collection('cases').createIndex({
notes: 'text',
});

const c = new Case(minimalCase);
c.notes = 'got it at work';
await c.save();
// Search for non-matching notes.
const res = await request(app)
.get('/api/cases?page=1&limit=10&q=home')
.expect(200)
.expect('Content-Type', /json/);
expect(res.body.cases).toHaveLength(0);
expect(res.body.total).toEqual(0);
// Search for matching notes.
await request(app)
.get(`/api/cases?page=1&limit=10&q=${encodeURI('at work')}`)
.expect(200, /got it at work/)
.expect('Content-Type', /json/);
});
it('should use age buckets in results', async () => {
const c = new Case(minimalCase);
const aBucket = await AgeBucket.findOne({});
c.demographics.ageBuckets = [aBucket!._id];
await c.save();
const res = await request(app)
.get(`/api/cases?page=1&limit=10`)
.get('/api/cases?page=1&limit=10')
.expect(200)
.expect('Content-Type', /json/);
expect(res.body.cases[0].demographics.ageRange.start).toEqual(aBucket!.start);
expect(res.body.cases[0].demographics.ageRange.end).toEqual(aBucket!.end);
expect(res.body.cases[0].demographics.ageRange.start).toEqual(
aBucket!.start,
);
expect(res.body.cases[0].demographics.ageRange.end).toEqual(
aBucket!.end,
);
});
it('should ignore the restricted collection', async () => {
const r = new RestrictedCase(minimalCase);
Expand Down Expand Up @@ -265,6 +252,14 @@ describe('GET', () => {
expect(res.body.cases).toHaveLength(1);
expect(res.body.cases[0].restrictedNotes).toBeUndefined();
});
it('should strip out notes', async () => {
const c = new Case(minimalCase);
c.notes = 'Can you keep a secret?';
await c.save();
const res = await request(app).get('/api/cases').expect(200);
expect(res.body.cases).toHaveLength(1);
expect(res.body.cases[0].notes).toBeUndefined();
});
describe('keywords', () => {
beforeEach(async () => {
const c = new Case(minimalCase);
Expand Down Expand Up @@ -504,7 +499,7 @@ describe('POST', () => {
const theCase = await Case.findOne({});
// case has range 40-50, should be bucketed into 36-40, 41-45, 46-50
expect(theCase!.demographics.ageBuckets).toHaveLength(3);
})
});
it('GETting the POSTed case should return an age range', async () => {
const theCase = await request(app)
.post('/api/cases')
Expand Down Expand Up @@ -1596,11 +1591,11 @@ describe('PUT', () => {
.send({
...curatorMetadata,
cases: [
{
{
_id: c._id,
demographics: {
ageRange,
}
},
},
],
})
Expand Down
8 changes: 4 additions & 4 deletions data-serving/data-service/test/util/case.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ describe('Case', () => {
'demographics.gender','demographics.nationalities','demographics.occupation','events','genomeSequences',
'location.administrativeAreaLevel1','location.administrativeAreaLevel2','location.administrativeAreaLevel3',
'location.country','location.geoResolution','location.geometry.latitude','location.geometry.longitude',
'location.name','location.place','location.query','notes','pathogens',
'location.name','location.place','location.query','pathogens',
'preexistingConditions.hasPreexistingConditions','preexistingConditions.values',
'revisionMetadata.creationMetadata.curator','revisionMetadata.creationMetadata.date',
'revisionMetadata.creationMetadata.notes','revisionMetadata.editMetadata.curator',
Expand All @@ -114,7 +114,7 @@ describe('Case', () => {
'events.outcome.value','events.icuAdmission.value', 'genomeSequences','location.administrativeAreaLevel1',
'location.administrativeAreaLevel2','location.administrativeAreaLevel3',
'location.country','location.geoResolution','location.geometry.latitude','location.geometry.longitude',
'location.name','location.place','location.query','notes','pathogens',
'location.name','location.place','location.query','pathogens',
'preexistingConditions.hasPreexistingConditions','preexistingConditions.values',
'revisionMetadata.creationMetadata.curator','revisionMetadata.creationMetadata.date',
'revisionMetadata.creationMetadata.notes','revisionMetadata.editMetadata.curator',
Expand All @@ -139,7 +139,7 @@ describe('Case', () => {
'events.selfIsolation.value', 'genomeSequences','location.administrativeAreaLevel1',
'location.administrativeAreaLevel2','location.administrativeAreaLevel3',
'location.country','location.geoResolution','location.geometry.latitude','location.geometry.longitude',
'location.name','location.place','location.query','notes','pathogens',
'location.name','location.place','location.query','pathogens',
'preexistingConditions.hasPreexistingConditions','preexistingConditions.values',
'revisionMetadata.creationMetadata.curator','revisionMetadata.creationMetadata.date',
'revisionMetadata.creationMetadata.notes','revisionMetadata.editMetadata.curator',
Expand All @@ -163,7 +163,7 @@ describe('Case', () => {
'events.selfIsolation.value', 'genomeSequences','location.administrativeAreaLevel1',
'location.administrativeAreaLevel2','location.administrativeAreaLevel3',
'location.country','location.geoResolution','location.geometry.latitude','location.geometry.longitude',
'location.name','location.place','location.query','notes','pathogens',
'location.name','location.place','location.query','pathogens',
'preexistingConditions.hasPreexistingConditions','preexistingConditions.values',
'revisionMetadata.creationMetadata.curator','revisionMetadata.creationMetadata.date',
'revisionMetadata.creationMetadata.notes','revisionMetadata.editMetadata.curator',
Expand Down
Loading