-
Notifications
You must be signed in to change notification settings - Fork 36
feat: templated external links #2194
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
base: master
Are you sure you want to change the base?
Changes from 7 commits
7cd9abe
a5ac1cc
ae07731
c403b88
c0fe37d
0adb721
6683b8a
a7fba61
47838fe
a289928
dfb0a78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| [ | ||
| { | ||
| "title": "Franzviewer II", | ||
| "url_template": "https://franz.site.com/franzviewer?id=${dataset.pid}", | ||
| "description_template": "View ${dataset.numberOfFiles} files in Franz' own personal viewer", | ||
| "filter": "(dataset.type == 'derived') && dataset.owner.includes('Franz')" | ||
| }, | ||
| { | ||
| "title": "High Beam-Energy View", | ||
| "url_template": "https://beamviewer.beamline.net/highenergy?id=${dataset.pid}", | ||
| "description_template": "The high-energy beamviewer (value ${dataset.scientificMetadata?.beamEnergy?.value}) at beamCo", | ||
| "filter": "(dataset.scientificMetadata?.beamEnergy?.value > 20)" | ||
| } | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,6 +77,7 @@ import { PartialUpdateDatablockDto } from "src/datablocks/dto/update-datablock.d | |
| import { Datablock } from "src/datablocks/schemas/datablock.schema"; | ||
| import { LogbooksService } from "src/logbooks/logbooks.service"; | ||
| import { Logbook } from "src/logbooks/schemas/logbook.schema"; | ||
| import { ExternalLinkClass } from "./schemas/externallink.class"; | ||
| import { CreateDatasetOrigDatablockDto } from "src/origdatablocks/dto/create-dataset-origdatablock"; | ||
| import { CreateOrigDatablockDto } from "src/origdatablocks/dto/create-origdatablock.dto"; | ||
| import { UpdateOrigDatablockDto } from "src/origdatablocks/dto/update-origdatablock.dto"; | ||
|
|
@@ -912,9 +913,9 @@ export class DatasetsController { | |
| outputDatasets = datasets.map((dataset) => | ||
| this.convertCurrentToObsoleteSchema(dataset), | ||
| ); | ||
| await Promise.all( | ||
| outputDatasets.map(async (dataset) => { | ||
| if (includeFilters) { | ||
| if (includeFilters) { | ||
| await Promise.all( | ||
| outputDatasets.map(async (dataset) => { | ||
| await Promise.all( | ||
| includeFilters.map(async ({ relation }) => { | ||
| switch (relation) { | ||
|
|
@@ -946,13 +947,9 @@ export class DatasetsController { | |
| } | ||
| }), | ||
| ); | ||
| } else { | ||
| /* eslint-disable @typescript-eslint/no-unused-expressions */ | ||
| // TODO: check the eslint error "Expected an assignment or function call and instead saw an expression" | ||
| dataset; | ||
| } | ||
| }), | ||
| ); | ||
| }), | ||
| ); | ||
| } | ||
| } | ||
| return outputDatasets as OutputDatasetObsoleteDto[]; | ||
| } | ||
|
|
@@ -1207,8 +1204,7 @@ export class DatasetsController { | |
| @Get("/findOne") | ||
| @ApiOperation({ | ||
| summary: "It returns the first dataset found.", | ||
| description: | ||
| "It returns the first dataset of the ones that matches the filter provided. The list returned can be modified by providing a filter.", | ||
| description: "Returns the first dataset that matches the provided filters.", | ||
| }) | ||
| @ApiQuery({ | ||
| name: "filter", | ||
|
|
@@ -1243,35 +1239,30 @@ export class DatasetsController { | |
|
|
||
| if (outputDataset) { | ||
| const includeFilters = mergedFilters.include ?? []; | ||
| await Promise.all( | ||
| includeFilters.map(async ({ relation }) => { | ||
| switch (relation) { | ||
| case "attachments": { | ||
| outputDataset.attachments = await this.attachmentsService.findAll( | ||
| { | ||
| where: { | ||
| datasetId: outputDataset.pid, | ||
| }, | ||
| }, | ||
| ); | ||
| break; | ||
| } | ||
| case "origdatablocks": { | ||
| outputDataset.origdatablocks = | ||
| await this.origDatablocksService.findAll({ | ||
| where: { datasetId: outputDataset.pid }, | ||
| }); | ||
| break; | ||
| if (includeFilters) { | ||
| await Promise.all( | ||
| includeFilters.map(async ({ relation }) => { | ||
| switch (relation) { | ||
| case "attachments": { | ||
| outputDataset.attachments = | ||
| await this.attachmentsService.findAll({ | ||
| where: { | ||
| datasetId: outputDataset.pid, | ||
| }, | ||
| }); | ||
| break; | ||
| } | ||
| case "origdatablocks": { | ||
| outputDataset.origdatablocks = | ||
| await this.origDatablocksService.findAll({ | ||
| where: { datasetId: outputDataset.pid }, | ||
| }); | ||
| break; | ||
| } | ||
| } | ||
| case "datablocks": { | ||
| outputDataset.datablocks = await this.datablocksService.findAll({ | ||
| where: { datasetId: outputDataset.pid }, | ||
| }); | ||
| break; | ||
| } | ||
| } | ||
| }), | ||
| ); | ||
| }), | ||
| ); | ||
| } | ||
| } | ||
| return outputDataset; | ||
| } | ||
|
|
@@ -1823,6 +1814,46 @@ export class DatasetsController { | |
| return await this.convertCurrentToObsoleteSchema(outputDatasetDto); | ||
| } | ||
|
|
||
| // GET /datasets/:id/externallinks | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add new endpoints to the old v3 controller?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what the right move is, honestly. |
||
| @UseGuards(PoliciesGuard) | ||
| @CheckPolicies( | ||
| "datasets", | ||
| (ability: AppAbility) => | ||
| ability.can(Action.DatasetRead, DatasetClass) || | ||
| ability.can(Action.DatasetReadOnePublic, DatasetClass), | ||
| ) | ||
| @Get("/:pid/externallinks") | ||
| @ApiOperation({ | ||
| summary: "Returns dataset external links.", | ||
| description: | ||
| "Returns the applicable external links for the dataset with the given pid.", | ||
| }) | ||
| @ApiParam({ | ||
| name: "pid", | ||
| description: "Id of the dataset to return external links", | ||
| type: String, | ||
| }) | ||
| @ApiResponse({ | ||
| status: HttpStatus.OK, | ||
| type: ExternalLinkClass, | ||
| isArray: true, | ||
| description: "A list of exernal link objects.", | ||
| }) | ||
| async findExternalLinksById( | ||
| @Req() request: Request, | ||
| @Param("pid") id: string, | ||
| ) { | ||
| const links = await this.datasetsService.findExternalLinksById(id); | ||
|
|
||
| await this.checkPermissionsForDatasetExtended( | ||
| request, | ||
| id, | ||
| Action.DatasetRead, | ||
| ); | ||
|
|
||
| return links; | ||
| } | ||
|
|
||
| // GET /datasets/:id/thumbnail | ||
| @UseGuards(PoliciesGuard) | ||
| @CheckPolicies( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,7 @@ import { | |
| PartialUpdateDatasetWithHistoryDto, | ||
| UpdateDatasetDto, | ||
| } from "./dto/update-dataset.dto"; | ||
| import { ExternalLinkClass } from "./schemas/externallink.class"; | ||
| import { IDatasetFields } from "./interfaces/dataset-filters.interface"; | ||
| import { DatasetClass, DatasetDocument } from "./schemas/dataset.schema"; | ||
| import { | ||
|
|
@@ -395,6 +396,53 @@ export class DatasetsService { | |
| throw new NotFoundException(error); | ||
| } | ||
| } | ||
|
|
||
| async findExternalLinksById(id: string): Promise<ExternalLinkClass[]> { | ||
| const thisDataSet = await this.findOneComplete({ | ||
| where: { pid: id }, | ||
| include: [DatasetLookupKeysEnum.all], | ||
| }); | ||
|
|
||
| if (!thisDataSet) { | ||
| // no luck. we need to create a new dataset | ||
| throw new NotFoundException(`Dataset #${id} not found`); | ||
| } | ||
|
|
||
| interface ExternalLinkTemplateConfig { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be nice to have interfaces/types in separate file, but that it just my subjective view.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general I agree. Right now this type is only used once, and in this spot. But I think we should be using types like this when we're parsing/validating all the JSON that this config data comes from. That may be a task best done en-masse though, as a separate change... |
||
| title: string; | ||
| url_template: string; | ||
| description_template: string; | ||
| filter: string; | ||
| } | ||
|
|
||
| const templates: ExternalLinkTemplateConfig[] | undefined = | ||
| this.configService.get("datasetExternalLinkTemplates"); | ||
| if (!templates) { | ||
| return []; | ||
| } | ||
|
|
||
| return templates | ||
| .filter((d) => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps a more descriptive variable name here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea. Addressed in 47838fe . |
||
| const filterFn = new Function("dataset", `return (${d.filter});`); | ||
| return filterFn(thisDataSet); | ||
| }) | ||
| .map((d) => { | ||
| const urlFn = new Function( | ||
| "dataset", | ||
| `return (\`${d.url_template}\`);`, | ||
| ); | ||
| const descriptionFn = new Function( | ||
| "dataset", | ||
| `return (\`${d.description_template}\`);`, | ||
| ); | ||
| return { | ||
| url: urlFn(thisDataSet), | ||
| title: d.title, | ||
| description: descriptionFn(thisDataSet), | ||
| }; | ||
| }); | ||
| } | ||
|
|
||
| // Get metadata keys | ||
| async metadataKeys( | ||
| filters: IFilters<DatasetDocument, IDatasetFields>, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you care to elaborate on this change a bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I came across this code when I was implementing an early version of this feature that included the external links in the dataset record itself, and found it confusing. The change I made is logically equivalent but easier to parse.