-
Notifications
You must be signed in to change notification settings - Fork 37
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
rest: add share_workflow endpoint #640
Conversation
Adds a new endpoint to share a workflow with a user. Closes reanahub/reana-client#680
8551616
to
1f608f6
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #640 +/- ##
==========================================
- Coverage 59.41% 59.14% -0.27%
==========================================
Files 32 32
Lines 3294 3314 +20
==========================================
+ Hits 1957 1960 +3
- Misses 1337 1354 +17
|
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 PR needs to be rebased! The commit messages might also need to be reworded to conform to the new commit convention.
"user_email_to_share_with": fields.String(), | ||
"message": fields.String(missing=None), | ||
"valid_until": fields.String(missing=None), |
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.
As for r-workflow-controller, I think we should put these parameters in the body of the request instead of using query parameters
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 agree.
The change has been applied to this superseding PR: #658
application/json: | ||
{ | ||
"message": "Malformed request.", | ||
"errors": ["Missing data for required field."] |
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.
As for r-workflow-controller, let's remove the errors
array if it's not needed, here and in the specification of the other error codes.
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.
Done.
The change has been applied to this superseding PR: #658
{ | ||
"errors": ["User is not allowed to share the workflow."] | ||
} |
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.
Examples and schema do not match
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.
Well spotted. Fixed it.
The change has been applied to this superseding PR: #658
from webargs import fields, validate | ||
from webargs.flaskparser import use_kwargs |
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.
As for r-wokflow-controller, please check the order of imports https://github.com/reanahub/reana/wiki/Tips-for-Python#imports
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'm pretty sure it's correct but I'm a bit short on time. Please double check.
The change has been applied to this superseding PR: #658
except ValueError as e: | ||
# In case of invalid workflow name / UUID | ||
logging.exception(str(e)) | ||
return jsonify({"message": str(e)}), 403 |
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 think this is not needed?
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.
You are right, thanks.
The change has been applied to this superseding PR: #658
"message": "Malformed request.", | ||
"errors": ["Missing data for required field."] | ||
} | ||
403: |
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 think 403 is never returned?
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.
Actually this can be returned by the signin_required
decorator, so let's keep it. We should also add 401, which is also returned by signin_required
.
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.
Added the 401.
The change has been applied to this superseding PR: #658
Included in #658 |
Adds a new endpoint to share a workflow with a user.
Closes reanahub/reana-client#680