rest: add sharing parameters to get_workflows
#545
Conversation
Adds a new endpoint to share a workflow with a user. Closes reanahub/reana-client#680
Adds a new endpoint to unshare a workflow. Closes reanahub/reana-client#681
Adds a new endpoint to retrieve whom a workflow is shared with. Closes reanahub/reana-client#686
Adds new `shared`, `shared_with` and `shared_by` parameters to the `get_workflows` endpoint to retrieve workflows along with their sharing information. Closes reanahub/reana-client#687
DaanRosendal
force-pushed
the
feature/list-shared
branch
from
6a70239 to
b1ae7b1
Compare
| @@ -64,6 +65,9 @@ | |||
| "user": fields.String(required=True), | |||
| "verbose": fields.Bool(missing=False), | |||
| "workflow_id_or_name": fields.String(), | |||
| "shared": fields.Bool(missing=False), | |||
| "shared_by": fields.String(), | |||
| "shared_with": fields.DelimitedList(fields.String()), | |||
There was a problem hiding this comment.
Should multiple values of shared_with be interpreted as the workflows shared with all the given users or with any of them? This also changes a bit the semantics of anybody and nobody
|
|
||
| from flask import Blueprint, jsonify, request | ||
| from reana_commons.config import WORKFLOW_TIME_FORMAT | ||
| from reana_db.database import Session | ||
| from reana_db.models import RunStatus, User, UserWorkflow, Workflow, WorkflowResource | ||
| from reana_db.utils import ( | ||
| _get_workflow_by_uuid, |
| @@ -13,13 +13,14 @@ | |||
| import logging | |||
| import re | |||
| from typing import Optional | |||
| from uuid import uuid4 | |||
| from uuid import UUID, uuid4 | |||
| if not shared and not shared_with and not shared_by: | ||
| # default case: retrieve owned workflows | ||
| query = user.workflows | ||
| else: | ||
| if shared or shared_by: | ||
| # retrieve owned workflows and unowned workflows shared by others | ||
| workflows_shared_with_user = Session.query( | ||
| user.shared_workflows.subquery().c.id_ | ||
| ) | ||
| query = Session.query(Workflow).filter( | ||
| or_( | ||
| Workflow.owner_id == user.id_, | ||
| Workflow.id_.in_(workflows_shared_with_user), | ||
| ) | ||
| ) | ||
| if shared_by and not shared: | ||
| if shared_by == "anybody": | ||
| # retrieve unowned workflows shared by anyone | ||
| query = query.filter(Workflow.id_.in_(workflows_shared_with_user)) | ||
| else: | ||
| # retrieve unowned workflows shared by specific user | ||
| shared_by_user = ( | ||
| Session.query(User).filter(User.email == shared_by).first() | ||
| ) | ||
| if not shared_by_user: | ||
| return ( | ||
| jsonify( | ||
| { | ||
| "message": f"User with email '{shared_by}' does not exist." | ||
| } | ||
| ), | ||
| 404, | ||
| ) | ||
| query = query.filter(Workflow.owner_id == shared_by_user.id_) | ||
| if shared_with: | ||
| # starting point: retrieve owned workflows | ||
| query = user.workflows | ||
|
|
||
| first_shared_with = shared_with[0] | ||
| workflows_shared_by_user = Session.query(Workflow.id_).join( | ||
| UserWorkflow, | ||
| and_( | ||
| Workflow.id_ == UserWorkflow.workflow_id, | ||
| Workflow.owner_id == user.id_, | ||
| ), | ||
| ) | ||
|
|
||
| if first_shared_with == "nobody": | ||
| # retrieve owned unshared workflows | ||
| query = query.filter(Workflow.id_.notin_(workflows_shared_by_user)) | ||
| elif first_shared_with == "anybody": | ||
| # retrieve exclusively owned shared workflows | ||
| query = query.filter(Workflow.id_.in_(workflows_shared_by_user)) | ||
| else: | ||
| # retrieve owned workflows shared with specific users | ||
| shared_with_users = Session.query(User.id_).filter( | ||
| User.email.in_(shared_with) | ||
| ) | ||
| shared_with_workflows_ids = Session.query( | ||
| UserWorkflow.workflow_id | ||
| ).filter(UserWorkflow.user_id.in_(shared_with_users)) | ||
| query = query.filter(Workflow.id_.in_(shared_with_workflows_ids)) |
There was a problem hiding this comment.
As far as I can tell, we can consider shared, shared_with and shared_by as mutually exclusive, right?
If that's the case, then I would suggest changing all of this code to make it more clear, as now there are lots of different cases due to the many nested ifs.
In particular, we could first check if the user specified more than one of shared, shared_with and shared_by and if so return an error. Otherwise, we just have three cases, one for each flag. Something like this (not tested):
if shared is not None and shared_by is not None:
return error
if shared is not None and shared_with is not None:
return error
if shared_by is not None and shared_with is not None:
return error
if shared;
# ...
elif shared_by:
# ...
elif shared_with:
# ...
else:
# normal caseThis should avoid most (if not all) nested ifs, making the logic a bit easier to understand.
| ), | ||
| ) | ||
|
|
||
| if first_shared_with == "nobody": |
There was a problem hiding this comment.
Do nobody and anybody only make sense if they are provided on their own, without other emails? If so, we need to check that
| "shared_with": ",".join(shared_with_emails) | ||
| if shared_with_emails != "-" | ||
| else "-", |
There was a problem hiding this comment.
No need to make this a string, we can return an array of strings here (and it will be deserialized correctly). If it's not shared with anybody, just return an empty list.
| shared_with: | ||
| type: string |
There was a problem hiding this comment.
This should be a list, not a string
| @@ -258,14 +281,90 @@ def get_workflows(args, paginate=None): # noqa | |||
| include_progress: bool = args.get("include_progress", verbose) | |||
| include_workspace_size: bool = args.get("include_workspace_size", verbose) | |||
| workflow_id_or_name: Optional[str] = args.get("workflow_id_or_name") | |||
| shared: bool = args.get("shared") | |||
| shared_by: Optional[str] = args.get("shared_by") | |||
| shared_with: Optional[str] = args.get("shared_with") | |||
There was a problem hiding this comment.
I think this should be List[str] and not Optional[str]
| @@ -306,6 +436,10 @@ def get_workflows(args, paginate=None): # noqa | |||
| "progress": get_workflow_progress( | |||
| workflow, include_progress=include_progress | |||
| ), | |||
| "owner_email": owner_email, | |||
There was a problem hiding this comment.
Given that we already have user in the response, maybe we can call this user_email?
| assert response_data[0]["owner_email"] == user1.email | ||
|
|
||
|
|
||
| def test_get_workflows_shared_with( |
There was a problem hiding this comment.
Let's also add a couple of tests to check corner cases like shared_with = nobody and shared_with = anybody?
|
Included in #552 |
Adds new
shared,shared_withandshared_byparameters to theget_workflowsendpoint to retrieve workflows along with their sharinginformation.
Closes reanahub/reana-client#687