-
Notifications
You must be signed in to change notification settings - Fork 32
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?
feat: templated external links #2194
Conversation
…e dataset with the given id.
| where: { datasetId: outputDataset.pid }, | ||
| }); | ||
| break; | ||
| if (includeFilters) { |
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.
| return await this.convertCurrentToObsoleteSchema(outputDatasetDto); | ||
| } | ||
|
|
||
| // GET /datasets/:id/externallinks |
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.
Should we add new endpoints to the old v3 controller?
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'm not sure what the right move is, honestly.
I'd be happy to add them but it would of course be a change to the v3 API (though only an additive one). Are we still changing that API or is it in a static state now that we have v4?
| throw new NotFoundException(`Dataset #${id} not found`); | ||
| } | ||
|
|
||
| interface ExternalLinkTemplateConfig { |
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 think it would be nice to have interfaces/types in separate file, but that it just my subjective view.
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.
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...
src/datasets/datasets.service.ts
Outdated
| } | ||
|
|
||
| return templates | ||
| .filter((d) => { |
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.
Perhaps a more descriptive variable name here?
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.
Good idea. Addressed in 47838fe .
| // so there is no equivalent schema representation for it. | ||
|
|
||
| export class ExternalLinkClass { | ||
| @ApiProperty({ |
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.
You could consider having these api properties automatically generated as we for example did in the samples module (if I remember correctly)
let me know if you want more information
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.
Do you mean by using SchemaFactory? I don't think we can do that here because these properties have no representations in MongoDb ... But I could be wrong. I'd love more information!
|
I think tests are missing? |
|
@GBirkel I have converted this to draft, as it did not have commits for the past month. Feel free to move it back to "ready for review" once you feel so or if I shouldn't have moved to draft |
Merge branch 'refs/heads/master' into templated-external-links-v3
Thanks. Yeah, I got sidetracked by other activity at the ALS. I think you're right that it needs tests before I can call it ready. |
This is a cleaned-up version of an earlier draft PR ( #2097 ).
Description
Add a field for generating dataset links based on admin-configured templates.
See #689 for details.
This has a companion PR for the front-end, which is still in draft form: SciCatProject/frontend#1910
Motivation
We get a lot of requests from users and researchers to provide a variety of different types of web-based tools that related directly to datasets. These can be pages that display visualization, pages that provide analysis tools (including AI/ML) and jupyter notebooks on particular JupyterHub instance. In all cases, we would want to take the user to the page with the dataset in context.
Changes:
This returns an array of objects,
{ url: string, title: string, description: string }. SeeExternalLinkClassin the code for more detail.datasetExternalLinkTemplates.jsonwith an example template file.Example of the content of this file:
[ { "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)" } ]The
filterfield is JavaScript, parsed into a function at run time, which is called with adatasetargument containing a full dataset record. If the function returns true, the given link applies to the given dataset.The
url_templateanddescription_templatefields are JavaScript-style format strings, which are passed the same dataset argument. The strings they emit go into theurlanddescriptionobjects returned by the API.