Separating revisions and retract methods into 2 separate branches. Th… #229
Conversation
…is branch now only includes /datasets/<id>/revisions and not datasets/<id>/retract
| # Response with the integer | ||
| return jsonify(revision_number) | ||
|
|
||
| @app.route('/datasets/<id>/revisions', methods=['GET']) |
There was a problem hiding this comment.
To be consistent, please add a comment block that describes this call, the parameters, and return type above this line. Follow examples from other endpoints in this file.
src/app.py
Outdated
| outputjson = [] | ||
| total_revisions = 1 | ||
| for i in complete_previous_list: | ||
| revision = collections.OrderedDict() |
There was a problem hiding this comment.
I don't see the need to use this collections.OrderedDict() because we don't care about the ordering of these three properties: dataset, dataset_uuid, and revision_number. And they should be consistent without enforcing ordering from the result anyway.
src/app.py
Outdated
| revision['revision_number'] = total_revisions | ||
| if j['status'].lower() != DATASET_STATUS_PUBLISHED: | ||
| user_token = auth_helper_instance.getAuthorizationTokens(request.headers) | ||
| if not user_in_hubmap_read_group(request) or isinstance(user_token, Response): |
There was a problem hiding this comment.
Need to rework this logic for handling the very last revision based on its status and the user token. Your code currently skips this next revision dataset if it's non-published and the user doesn't has access (no token or not in the HuBMAP-Read group). This would work correctly only when this dataset is the latest revision.
src/app.py
Outdated
| outputjson.append(revision) | ||
| total_revisions = total_revisions + 1 | ||
| outputjson.reverse() | ||
| return Response(json.dumps(outputjson, sort_keys=True), 200, mimetype='application/json') |
There was a problem hiding this comment.
Better to use Flask's jsonify() which can serialize python list/dict to JSON and wrap it in a Response with the application/json mimetype.
src/app.py
Outdated
| if can_display: | ||
| outputjson.append(revision) | ||
| total_revisions = total_revisions + 1 | ||
| outputjson.reverse() |
There was a problem hiding this comment.
We can't rely on this reverse() because the outputjson list is not guaranteed an ordered list.
Separated datasets//revisions and datasets//retract into 2 separate feature branches. Retract is still a work in progress (PR changed to draft). This one passed all local testing and is ready to be tested on dev-integrate assuming no merge conflicts or requested changes.