Skip to content
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

Allow fetching duke-ds-project permissions #170

Merged
merged 16 commits into from
Aug 9, 2018

Conversation

johnbradley
Copy link
Contributor

Changes add query parameter is_deliverable to the /api/v2/duke-ds-projects/ endpoint.
Also returns is_deliverable in the payload for duke-ds-project.

Fixes #169

@johnbradley
Copy link
Contributor Author

I am having second thoughts about these changes in relation to performance.
It takes 11 seconds to query the list for my user with 63 projects. If I skip fetching project permissions it drops to 2.7 seconds.

@dleehr
Copy link
Member

dleehr commented Aug 3, 2018

Do you think we might want to add caching in some form?

There might be a simple Django or DRF plugin that solves this handily for these DukeDS API responses that don’t change every second. And there’s always redis or memcached. Probably a good idea for user listing too.

@johnbradley
Copy link
Contributor Author

Do you think we might want to add caching in some form?

I worry about timely cache invalidation if we cache project permissions.
First time users will wait ~11 seconds while we populate the cache.

One thought was to turn this around and check permissions at the point the try to proceed with a delivery. A user sees all projects and can select them and then we show them a warning like You must have admin priviledges to deliver this project. This feels like we are just moving user frustration further down the road.

I looked to see what the DukeDS portal does: It fetches permissions for each project individually.

Perhaps I could look at paging the data?

@dleehr
Copy link
Member

dleehr commented Aug 6, 2018

One thought was to turn this around and check permissions at the point the try to proceed with a delivery.

One thing that occurs to me on reflection is that neither method is a great experience for the user.

If we filter out the non-deliverable ones, users might be confused about why they're seeing more projects listed in the DDS web portal vs the datadelivery UI.

But if we include all of their projects in the listing, we have to communicate which ones cannot be delivered and why. If we don't deliver this feedback until late in the process, that can be frustrating.

So perhaps the best tradeoff for now is to allow them to pick any of their projects, then check the permission and deliver feedback before proceeding to the next step?

This was too slow to fetch a reasonable list of projects.
As an alternative added here is a new duke-ds-project-permissions
endpoint. This endpoint requires at least project_id query
parameter due to the underlying DukeDS API.
@johnbradley
Copy link
Contributor Author

I removed theis_delivery property/filtering from the delivery endpoint.
I added a new /api/v2/duke-ds-project-permissions/ endpoint to allow checking project permissions.

The underlying DukeDS API requires a project_id: https://api.dataservice.duke.edu/apiexplorer#!/projects/getApiV1ProjectsProjectIdPermissions
To work around this the endpoint requires project_id query parameter to return a list of permissions.
There is also an optional user_id filter.

The underlying DukeDS API doesn't return a unique ID.
To work around this the code generates one by combining the project and user ids returned.

This is to have access to data_service.get_project_permissions.
The DukeDS api only returns non-deleted when listing projects.
However if a user fetches a project directly it can return a
deleted project. This will allow users to tell if a project
that is used in a transfer has been deleted.
@johnbradley johnbradley changed the title Allow filtering duke-ds-project by is_deliverable Allow fetching duke-ds-project permissions Aug 8, 2018
@johnbradley johnbradley requested a review from dleehr August 8, 2018 20:53
dds_util = DDSUtil(self.request.user)
project_id = self.request.query_params.get('project')
user_id = self.request.query_params.get('user')
# a project id is required due to the underlying implementation of DukeDS API
Copy link
Member

Choose a reason for hiding this comment

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

If this endpoint, when un-filtered, returned all of the user's projects+permissions, then I think a project_id query parameter makes sense (as a filter on that list). But since this requires that parameter - and it's a specific ID and not a search term, it doesn't seem like query parameters are the best fit.

Perhaps it would make more sense to list project permissions via a detail route under the single project endpoint (/duke-ds-projects/:id/permissions/), as DukeDS does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to make it fit in with the other ember models. I think I can use and adapter to make it work with this approach in ember.

@johnbradley johnbradley requested a review from dleehr August 9, 2018 14:57
Copy link
Member

@dleehr dleehr left a comment

Choose a reason for hiding this comment

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

LGTM. Looks like the destructure_id method is no longer used, as there's no way to fetch a single permission record by ID through the API. Do we need that functionality, or just the listing?

@johnbradley johnbradley merged commit 7b55f13 into master Aug 9, 2018
@johnbradley johnbradley deleted the 169-project-is-deliverable branch August 9, 2018 16:53
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