Add annotation access controls at the folder level #905
Conversation
| Description('Get the annotations from the items in a folder') | ||
| .param('id', 'The ID of the folder', required=True, paramType='path') | ||
| .param('recurse', 'Whether or not to retrieve all ' | ||
| 'annotations from subfolders', required=False, default=False, dataType='boolean') |
There was a problem hiding this comment.
Why did you choose to have recurse default to false for this endpoint (and true on the present endpoint)?
There was a problem hiding this comment.
My thought was that when you think of an endpoint that gets annotations from a folder someone is more likely to assume that it's just returning the annotations from that folder and not all other folders it contains as well. Whereas with the present endpoint I thought they'd be more likely to use it when trying to determine if a user owns any annotations in the folder or its subfolders. But I realize this logic isn't very intuitive, so I could change it so they're both the same.
| ) | ||
| @access.public | ||
| def returnFolderAnnotations(self, id, recurse, limit, offset, sort): | ||
| return self.getFolderAnnotations(id, recurse, self.getCurrentUser(), limit, offset, |
There was a problem hiding this comment.
If the returned result has a count method, girder will automatically add a header with the count of results for a list-like entry. This could be done like here: https://github.com/girder/girder/blob/master/girder/utility/acl_mixin.py#L345-L351.
A simple way to do this might be to add a count parameter to getFolderAnnotations where, if True, instead of adding the sort, skip, and limit fields to the pipeline, you'd do pipeline += [{'$count': 'count'}], then this endpoint would become
result = self.getFolderAnnotations(
id, recurse, self.getCurrentUser(), limit, offset, sort[0][0], sort[0][1])
def count():
try:
return next(iter(self.getFolderAnnotations(
id, recurse, self.getCurrentUser(), count=True)))['count']
except StopIteration:
# If there are no values, this won't return the count, in
# which case it is zero.
return 0
result.count = count
return result
There was a problem hiding this comment.
Oh that's good to know, thanks!
See 0e1adec
| annotations = self.getFolderAnnotations(id, recurse, self.getCurrentUser(), 1) | ||
| try: | ||
| next(annotations) | ||
| yield True |
There was a problem hiding this comment.
If you use return rather than yield, the endpoint will return true or false rather than [true] or [false], which was more what I would have expected,
There was a problem hiding this comment.
That's a good point.
See 06302c7
| @@ -0,0 +1,150 @@ | |||
| import $ from 'jquery'; | |||
There was a problem hiding this comment.
You need to add the HierarchyWidget to the views/index.js file to get it imported.
|
@eagw Can you merge master? A recent change to one of the upstream libraries broke CI and there is a fix on that was merged to master. |
| key: annot[key] for key in ('access', 'public', 'publicFlags') | ||
| if key in annot | ||
| }}) | ||
| yield annot |
There was a problem hiding this comment.
We might be change a lot of annotations, so returning them all is probably not desired. We may just want to return a summary (maybe {'updated': <count>})?
Also, since this can take a while to run, we should add setResponseTimeLimit() inside the loop.
There was a problem hiding this comment.
That makes sense.
See 666ee38
| annot = Annotation().load(annotation['_id'], user=user, getElements=False) | ||
| annot = Annotation().setPublic(annot, public) | ||
| annot = Annotation().setAccessList( | ||
| annot, access, save=False, user=user) |
There was a problem hiding this comment.
Isn't save=False is the default for both setPublic and setAccessList? If so, do we need to be explicit here?
There was a problem hiding this comment.
Yes it is.
See 7b3786f
|
|
||
| import AnnotationModel from '../models/AnnotationModel'; | ||
|
|
||
| wrap(HierarchyWidget, 'initialize', function (initialize, settings) { |
There was a problem hiding this comment.
Since we are doing the same thing in the render method, can we get rid of doing it in the initialize method?
There was a problem hiding this comment.
Yes, we can.
See 62a6b50
|
This is working well in my test case. After the last few comments are addressed, we can merge this. I'd like to have a follow-on PR to add some tests of the end points, so at least we know we don't break them in the future. |
Transferred from DigitalSlideArchive/HistomicsUI and PR #217
Adds the ability to change the access list for all annotations in a folder (and optionally all annotations in the subfolders of the folder). The control can be found as an entry in the 'Folder actions' dropdown menu; it appears as long as the user owns at least one annotation in the folder (or it's subfolders) and only alters the permissions on the annotations that the user owns.
Closes DigitalSlideArchive/HistomicsUI issue #27