-
Notifications
You must be signed in to change notification settings - Fork 30
feat(APIv4): add origdatablock v4 controllers #1953
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
Conversation
One additional proposed change: |
This comment was marked as resolved.
This comment was marked as resolved.
The API tag for origdatablock public controller needs to be changed |
Okay, will be fixed. Not sure how I managed to miss that... |
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.
The PR in general looks great. I have left few minor comments but otherwise I think you did a great job.
@@ -1364,7 +1372,7 @@ export class CaslAbilityFactory { | |||
accessGroups: { $in: user.currentGroups }, | |||
}); | |||
can(Action.OrigdatablockReadOneAccess, OrigDatablock, { | |||
ownerGroup: { $in: user.currentGroups }, | |||
isPublished: true, |
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.
So as I understand with this change the CREATE_DATASET_PRIVILEGED_GROUPS
can only read published Origdatablock
and not all that are in their access group
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.
The removed check for ownerGroup here was a duplicate, the exact same check is still in the code a few lines above. This one was a copy-paste error I assume, where there should always have been the isPublished check..
}) | ||
@IsOptional() | ||
@IsBoolean() | ||
readonly isPublished?: boolean; |
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 guess the idea behind adding the field in the update dto is to be able to control it from outside by allowing a user with proper access to update the isPublished
flag
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.
This is specifically in the update DTO because the base class OwnableDto
does not include isPublished
, so each subclass needs to add it manually. From #1757 I got the impression that long-term isPublished may not stay in the Ownable
schema, so i opted to put it here
@@ -1,40 +1,12 @@ | |||
import { PipeTransform, Injectable } from "@nestjs/common"; |
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 like the idea that you made this reusable and moved it to a common module
6f237a6
to
aa02269
Compare
Description
Add new controllers
OrigDatablocksV4Controller
andOrigDatablocksPublicV4Controller
, mirroring the structure forDatasets
. Add required functionality in theOrigDatablocksService
and refactor support functions from DatasetsV4 to the common module where possibleMotivation
The improvements from API v4 currently only cover the
Dataset
subsystem, other collections are not yet available under/api/v4/
endpoints. Instead of copying large parts of the v3 API, v4 should use the new querying standards developed for Datasets.This PR is a trial run to transfer the same functionality to the
OrigDatablocks
system.Changes:
add
OrigDatablocksV4Controller
andOrigDatablocksPublicV4Controller
with full v3 functionalitymerge GET /api/v3/origdatablocks and /api/v3/fullquery endpoints to /api/v4/origdatablocks
move GET /api/v3/origdatablocks/fullquery/files endpoint to /api/v4/origdatablocks/files
copy querying machinery from Datasets v4 implementation
update
OrigDatablocksService
with new functions to accept queries in the v4 formatadd
isPublished
as optional field inUpdateOrigDatablockDto
add unit and api tests for new systems
add
pid
as required field inOutputDatasetDto
(was optional as inherited fromCreateDatasetDto
)Refactor of support functions:
FilterValidationPipe and IncludeValidationPipe moved to common module
Needed constants moved into collection/types/collection-lookup.ts files for import
All API v4 controllers refactored to use common module pipes, removed individual versions
Tests included
Documentation
official documentation info