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

toc_generation service #3465

Merged
merged 33 commits into from
Mar 8, 2021
Merged
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
8ad80a4
toc_generation service, WIP
daneryl Jan 25, 2021
282a5a0
aggregations for generatedToc property
daneryl Feb 10, 2021
60263ad
review toc endpoint
daneryl Feb 11, 2021
77b0532
tocService as a feature and cronjob
daneryl Feb 11, 2021
2883177
customFilters for search endpoint
daneryl Feb 17, 2021
bcdb188
UI toc generation components, WIP
daneryl Feb 11, 2021
baa08e5
added basic styles on autogenerated toc
Feb 26, 2021
42e7dfd
toc_generation service, WIP
daneryl Jan 25, 2021
97405a3
aggregations for generatedToc property
daneryl Feb 10, 2021
ed1565a
review toc endpoint
daneryl Feb 11, 2021
2023a22
tocService as a feature and cronjob
daneryl Feb 11, 2021
5f88a16
customFilters for search endpoint
daneryl Feb 17, 2021
08bea1f
UI toc generation components, WIP
daneryl Feb 11, 2021
d1cb5d3
added basic styles on autogenerated toc
Feb 26, 2021
305f49a
Merge branch '3447_toc_service_integration' of https://github.com/hur…
Feb 26, 2021
5650300
Updated autogenerated toc
grafitto Mar 1, 2021
8868f57
fix lint errors
daneryl Mar 2, 2021
f632ead
fix blank state panel
daneryl Mar 2, 2021
eb09f2f
fix e2e
daneryl Mar 2, 2021
eba2caf
prevent crash on toc service unavailable
daneryl Mar 3, 2021
5928bb0
Removed unnecessary scss import
Mar 4, 2021
d8ef75b
fixed/refactor styles
daneryl Mar 3, 2021
adb875a
Rename spec describe
daneryl Mar 3, 2021
e0cff38
removed comment
daneryl Mar 3, 2021
cc42af9
fixed emit types
daneryl Mar 3, 2021
4967e1a
ignored eslint on spec
daneryl Mar 3, 2021
1cde69b
review fixes
daneryl Mar 4, 2021
d87b298
Removed for loop on toc scss
Mar 4, 2021
aa2f46b
uploadFile returns the full response
daneryl Mar 5, 2021
e1556b8
error handling on tocService
daneryl Mar 5, 2021
a32f998
icon for review toc
daneryl Mar 5, 2021
97efec7
filteredAggregations labels now using t function
daneryl Mar 8, 2021
0a6cd50
Merge branch 'development' into 3447_toc_service_integration
konzz Mar 8, 2021
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: 4 additions & 0 deletions app/api/entities/entities.js
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,10 @@ async function updateEntity(entity, _template) {
if (typeof entity.template !== 'undefined') {
d.template = entity.template;
}

if (typeof entity.generatedToc !== 'undefined') {
d.generatedToc = entity.generatedToc;
}
return model.save(d);
})
);
Expand Down
1 change: 1 addition & 0 deletions app/api/entities/entitiesModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const mongoSchema = new mongoose.Schema(
title: { type: String, required: true },
template: { type: mongoose.Schema.Types.ObjectId, ref: 'templates', index: true },
published: Boolean,
generatedToc: Boolean,
icon: new mongoose.Schema({
_id: String,
label: String,
Expand Down
20 changes: 19 additions & 1 deletion app/api/entities/specs/entities.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -340,14 +340,15 @@ describe('entities', () => {
});
});

describe('when published/template property changes', () => {
describe('when published/template/generatedToc property changes', () => {
it('should replicate the change for all the languages', done => {
const doc = {
_id: batmanFinishesId,
sharedId: 'shared',
metadata: {},
published: false,
template: templateId,
generatedToc: true,
};

entities
Expand All @@ -364,15 +365,32 @@ describe('entities', () => {
expect(docES.template).toBeDefined();

expect(docES.published).toBe(false);
expect(docES.generatedToc).toBe(true);
expect(docES.template.equals(templateId)).toBe(true);
expect(docEN.published).toBe(false);
expect(docEN.generatedToc).toBe(true);
expect(docEN.template.equals(templateId)).toBe(true);
done();
})
.catch(catchErrors(done));
});
});

describe('when generatedToc is undefined', () => {
it('should not replicate the value to all languages', async () => {
const doc = { _id: batmanFinishesId, sharedId: 'shared', generatedToc: true };
await entities.save(doc, { language: 'en' });
await entities.save({ _id: batmanFinishesId, sharedId: 'shared' }, { language: 'en' });
const [docES, docEN] = await Promise.all([
entities.getById('shared', 'es'),
entities.getById('shared', 'en'),
]);

expect(docES.generatedToc).toBe(true);
expect(docEN.generatedToc).toBe(true);
});
});

it('should sync select/multiselect/dates/multidate/multidaterange/numeric', done => {
const doc = {
_id: syncPropertiesEntityId,
Expand Down
24 changes: 24 additions & 0 deletions app/api/files/files.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { deleteUploadedFiles } from 'api/files/filesystem';
import connections from 'api/relationships';
import { search } from 'api/search';
import entities from 'api/entities';

import model from './filesModel';
import { validateFile } from '../../shared/types/fileSchema';
Expand Down Expand Up @@ -30,4 +31,27 @@ export const files = {

return toDeleteFiles;
},

async tocReviewed(_id: string, language: string) {
const savedFile = await files.save({ _id, generatedToc: false });
const sameEntityFiles = await files.get({ entity: savedFile.entity }, { generatedToc: 1 });
const [entity] = await entities.get({
sharedId: savedFile.entity,
});
Comment on lines +38 to +40
Copy link
Member

Choose a reason for hiding this comment

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

If you are already fetching the entities, should it be worth it to check if this process would actually change the value before producing the save on line 42?

I mean, do we risk it to just save even if there would be no changes or do we make the reduce on line 47 here and only save if there are actual changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i do not see any benefit other than a really small performance impact ?, adding a condition will add complexity. do you foresee any problems saving entities with the same values ?

Copy link
Member

Choose a reason for hiding this comment

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

My concern is that conditionals should happen at some point. So, if not here, then the entities save, when updating all the connected entities, should know: hey, nothing changed. But if this save actually triggers an full entity check and reindex on all connected entities, I would rather have the conditional here. If you are confident that is not going to happen, I agree a single save (or potentially n entities depending on languages) is not a huge deal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@RafaPolit im not confident its not going to happen, but if saving one entity can be this problematic i believe this should be handled at a lower level, not anytime we need to save one ? tech debt for this pls ?


await entities.save(
{
_id: entity._id,
sharedId: entity.sharedId,
template: entity.template,
generatedToc: sameEntityFiles.reduce<boolean>(
(generated, file) => generated || Boolean(file.generatedToc),
false
),
},
{ user: {}, language }
);

return savedFile;
},
};
22 changes: 22 additions & 0 deletions app/api/files/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,28 @@ export default (app: Application) => {
.catch(next);
});

app.post(
'/api/files/tocReviewed',
needsAuthorization(['admin', 'editor']),
validation.validateRequest({
properties: {
body: {
required: ['fileId'],
properties: {
fileId: { type: 'string' },
},
},
},
}),
async (req, res, next) => {
try {
res.json(await files.tocReviewed(req.body.fileId, req.language));
} catch (e) {
next(e);
}
}
);

app.get(
'/api/files/:filename',
validation.validateRequest({
Expand Down
38 changes: 32 additions & 6 deletions app/api/files/specs/fixtures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,29 @@ const entityEnId = db.id();
const uploadId = db.id();
const uploadId2 = db.id();
const templateId = db.id();
const importTemplate = db.id();
const fileName1 = 'f2082bf51b6ef839690485d7153e847a.pdf';

const fixtures: DBFixture = {
files: [
{
_id: uploadId,
entity: 'entity',
entity: 'sharedId1',
generatedToc: true,
originalname: 'upload1',
filename: fileName1,
type: 'custom',
},
{
_id: uploadId2,
entity: 'entity',
generatedToc: true,
entity: 'sharedId1',
filename: 'fileNotInDisk',
},
{
entity: 'sharedId1',
filename: 'fileWithoutTocFlag',
},
{ _id: db.id(), filename: 'fileNotOnDisk' },
{ _id: db.id(), originalname: 'upload2', type: 'custom' },
{ _id: db.id(), originalname: 'upload3', type: 'document' },
Expand All @@ -36,11 +43,21 @@ const fixtures: DBFixture = {
sharedId: 'sharedId1',
language: 'es',
title: 'Gadgets 01 ES',
toc: [{ _id: db.id(), label: 'existingToc' }],
generatedToc: true,
template: templateId,
},
{ _id: entityEnId, sharedId: 'sharedId1', language: 'en', title: 'Gadgets 01 EN' },
{
_id: entityEnId,
template: templateId,
sharedId: 'sharedId1',
language: 'en',
title: 'Gadgets 01 EN',
},
],
templates: [
{ _id: templateId, default: true, name: 'mydoc', properties: [] },
{ _id: importTemplate, default: true, name: 'import', properties: [] },
],
templates: [{ _id: templateId, default: true, name: 'mydoc', properties: [] }],
settings: [
{
_id: db.id(),
Expand All @@ -51,4 +68,13 @@ const fixtures: DBFixture = {
],
};

export { fixtures, entityId, entityEnId, fileName1, uploadId, uploadId2, templateId };
export {
fixtures,
entityId,
entityEnId,
fileName1,
uploadId,
uploadId2,
templateId,
importTemplate,
};
36 changes: 34 additions & 2 deletions app/api/files/specs/routes.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { FileType } from 'shared/types/fileType';
import { fixtures, uploadId, uploadId2 } from './fixtures';
import { files } from '../files';
import uploadRoutes from '../routes';
import entities from 'api/entities';

jest.mock(
'../../auth/authMiddleware.ts',
Expand Down Expand Up @@ -47,7 +48,7 @@ describe('files routes', () => {
});

it('should reindex all entities that are related to the saved file', async () => {
expect(search.indexEntities).toHaveBeenCalledWith({ sharedId: 'entity' }, '+fullText');
expect(search.indexEntities).toHaveBeenCalledWith({ sharedId: 'sharedId1' }, '+fullText');
});
});

Expand Down Expand Up @@ -85,7 +86,7 @@ describe('files routes', () => {
.query({ _id: uploadId2.toString() });

expect(search.indexEntities).toHaveBeenCalledWith(
{ sharedId: { $in: ['entity'] } },
{ sharedId: { $in: ['sharedId1'] } },
'+fullText'
);
});
Expand All @@ -107,5 +108,36 @@ describe('files routes', () => {

expect(response.body.errors[0].message).toBe('should be string');
});

describe('api/files/tocReviewed', () => {
it('should set tocGenerated to false on the file', async () => {
const response: SuperTestResponse = await request(app)
.post('/api/files/tocReviewed')
.set('content-language', 'es')
.send({ fileId: uploadId.toString() });

const [file] = await files.get({ _id: uploadId });
expect(file.generatedToc).toBe(false);
expect(response.body.entity).toBe('sharedId1');
});

it('should set tocGenerated to false on the entity when all associated files are false', async () => {
await request(app)
.post('/api/files/tocReviewed')
.send({ fileId: uploadId.toString() })
.expect(200);

let [entity] = await entities.get({ sharedId: 'sharedId1' });
expect(entity.generatedToc).toBe(true);

await request(app)
.post('/api/files/tocReviewed')
.send({ fileId: uploadId2.toString() })
.expect(200);

[entity] = await entities.get({ sharedId: 'sharedId1' });
expect(entity.generatedToc).toBe(false);
});
});
});
});
17 changes: 10 additions & 7 deletions app/api/files/specs/uploadRoutes.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { setUpApp, socketEmit, iosocket } from 'api/utils/testingRoutes';
import { FileType } from 'shared/types/fileType';
import entities from 'api/entities';

import { fixtures, templateId } from './fixtures';
import { fixtures, templateId, importTemplate } from './fixtures';
import { files } from '../files';
import uploadRoutes from '../routes';

Expand Down Expand Up @@ -72,7 +72,10 @@ describe('upload routes', () => {
expect(iosocket.emit).toHaveBeenCalledWith('conversionStart', 'sharedId1');
expect(iosocket.emit).toHaveBeenCalledWith('documentProcessed', 'sharedId1');

const [upload] = await files.get({ entity: 'sharedId1' }, '+fullText');
const [upload] = await files.get(
{ originalname: 'f2082bf51b6ef839690485d7153e847a.pdf' },
'+fullText'
);

expect(upload).toEqual(
expect.objectContaining({
Expand Down Expand Up @@ -105,14 +108,14 @@ describe('upload routes', () => {
it('should detect English documents and store the result', async () => {
await uploadDocument('uploads/eng.pdf');

const [upload] = await files.get({ entity: 'sharedId1' });
const [upload] = await files.get({ originalname: 'eng.pdf' });
expect(upload.language).toBe('eng');
}, 10000);

it('should detect Spanish documents and store the result', async () => {
await uploadDocument('uploads/spn.pdf');

const [upload] = await files.get({ entity: 'sharedId1' });
const [upload] = await files.get({ originalname: 'spn.pdf' });
expect(upload.language).toBe('spa');
});
});
Expand All @@ -126,7 +129,7 @@ describe('upload routes', () => {
.attach('file', path.join(__dirname, 'uploads/invalid_document.txt'))
);

const [upload] = await files.get({ entity: 'sharedId1' }, '+fullText');
const [upload] = await files.get({ originalname: 'invalid_document.txt' }, '+fullText');
expect(upload.status).toBe('failed');
});

Expand Down Expand Up @@ -176,15 +179,15 @@ describe('upload routes', () => {
await socketEmit('IMPORT_CSV_END', async () =>
request(app)
.post('/api/import')
.field('template', templateId.toString())
.field('template', importTemplate.toString())
.attach('file', `${__dirname}/uploads/importcsv.csv`)
);

expect(iosocket.emit).toHaveBeenCalledWith('IMPORT_CSV_START');
expect(iosocket.emit).toHaveBeenCalledWith('IMPORT_CSV_PROGRESS', 1);
expect(iosocket.emit).toHaveBeenCalledWith('IMPORT_CSV_PROGRESS', 2);

const imported = await entities.get({ template: templateId });
const imported = await entities.get({ template: importTemplate });
expect(imported).toEqual([
expect.objectContaining({ title: 'imported entity one' }),
expect.objectContaining({ title: 'imported entity two' }),
Expand Down
4 changes: 2 additions & 2 deletions app/api/search/deprecatedRoutes.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import Joi from 'joi';
import entities from 'api/entities';
import { searchSchema } from 'api/search/searchSchema';
import { searchParamsSchema } from 'shared/types/searchParams';
import { search } from './search';
import { validation, parseQuery } from '../utils';

Expand All @@ -25,7 +25,7 @@ export default app => {
app.get(
'/api/search',
parseQuery,
validation.validateRequest(searchSchema),
validation.validateRequest(searchParamsSchema),

(req, res, next) => {
const action = req.query.geolocation ? 'searchGeolocations' : 'search';
Expand Down
17 changes: 16 additions & 1 deletion app/api/search/documentQueryBuilder.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import { preloadOptionsSearch } from 'shared/config';
import filterToMatch, { multiselectFilter } from './metadataMatchers';
import { propertyToAggregation } from './metadataAggregations';
import { propertyToAggregation, generatedTocAggregations } from './metadataAggregations';

export default function() {
const baseQuery = {
Expand Down Expand Up @@ -231,6 +231,17 @@ export default function() {
}
},

customFilters(filters = {}) {
Object.keys(filters).forEach(key => {
if (filters[key].values.length) {
addFilter({
terms: { [key]: filters[key].values },
});
}
});
return this;
},

filterMetadata(filters = []) {
filters.forEach(filter => {
const match = filterToMatch(filter, filter.suggested ? 'suggestedMetadata' : 'metadata');
Expand All @@ -241,6 +252,10 @@ export default function() {
return this;
},

generatedTOCAggregations() {
baseQuery.aggregations.all.aggregations.generatedToc = generatedTocAggregations(baseQuery);
},

aggregations(properties, dictionaries) {
properties.forEach(property => {
baseQuery.aggregations.all.aggregations[property.name] = propertyToAggregation(
Expand Down
Loading