Skip to content

Conversation

hkethi002
Copy link
Contributor

@hkethi002 hkethi002 commented Jul 31, 2017

Fixes #853
For api/groups?join=projects, each group result is returned with a a list of that group's project that the user has access to, permissions are filtered for the projects as well.
Specifically returned for each project are its

  • id
  • label
  • description
  • permissions

Review Checklist

  • Tests were added to cover all code changes
  • Documentation was added / updated
  • Code and tests follow standards in CONTRIBUTING.md

@hkethi002 hkethi002 requested a review from nagem July 31, 2017 20:34
api/dao/base.py Outdated
except KeyError:
raise APINotFoundException('Children cannot be listed from the {0} level'.format(self.cont_name))
query = {self.cont_name[:-1]: bson.ObjectId(_id)}
if self.cont_name == 'groups':
Copy link
Contributor

@nagem nagem Aug 3, 2017

Choose a reason for hiding this comment

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

We should use self.use_object_id here instead of keying directly on groups, right now there isn't a difference because the other container that don't use an object id don't ever call get_children but we'd like to respect what the storage instance chooses.

api/dao/base.py Outdated
raise APINotFoundException('Children cannot be listed from the {0} level'.format(self.cont_name))
query = {self.cont_name[:-1]: bson.ObjectId(_id)}
if self.cont_name == 'groups':
query = {self.cont_name[:-1]: _id}
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a utility method in contianerutil.py that singularizes container names, mind changing this existing code to use that instead? Analysis -> Analyses ruined the "remove the s" hack (also wouldn't affect us here since analyses don't have children but good to clean up that old hack when we can).

@nagem
Copy link
Contributor

nagem commented Aug 3, 2017

Looks good 👍

Just a bit of cleanup. Also FYI @dpuccetti

api/dao/base.py Outdated
if uid:
query['permissions'] = {'$elemMatch': {'_id': uid}}
else:
query = {self.cont_name[:-1]: bson.ObjectId(_id)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just one more right here.

Copy link
Contributor

Choose a reason for hiding this comment

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

And wouldn't you want to respect the uid match no matter the container? I must have missed that in the first review.

@nagem
Copy link
Contributor

nagem commented Aug 4, 2017

Changes look good, thanks!
Feel free to merge.

@hkethi002 hkethi002 merged commit e4fbd1c into master Aug 4, 2017
@hkethi002 hkethi002 deleted the group-display branch August 4, 2017 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants