-
Notifications
You must be signed in to change notification settings - Fork 2
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
Refs #83 - Corrected the jsonify expression for returning database ob… #96
Conversation
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.
Database query is OK but there are some problems regarding output, I suggested some improvements for them.
practice-app/app/block.py
Outdated
@@ -11,4 +11,4 @@ | |||
@block_api.route('/api/v1.0/<int:user_id>/blocked-users', methods=['GET']) | |||
def get_blocked_users(user_id): | |||
blocked_users = session.query(Blocking).filter(Blocking.blockingID == user_id).all() | |||
return jsonify({"blockedIDs": list(blocked_users)}), 200 | |||
return jsonify({col.name: str(getattr(blocked_users, col.name)) for col in blocked_users.__table__.columns}), 200 |
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.
Since this endpoint is for getting the users blocked by the user with given id, you don't need to return all Blocking object. Also, you tried to access object attributes from list not the object's itself. So, here is my suggestion:
return jsonify({col.name: str(getattr(blocked_users, col.name)) for col in blocked_users.__table__.columns}), 200 | |
return jsonify({"blockedIDs": [user.blockedID for user in blocked_users]}), 200 |
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.
Thank you for your review Kerem. I think you are correct, I am implementing your suggestion.
Since the suggested change is implemented and approved, I am merging this pull request. |
…ject.
The jsonify expression in the return clause has been corrected since only calling list() is not functional.