-
Notifications
You must be signed in to change notification settings - Fork 8
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
Bugfixes #175
Conversation
biblib/views/user_view.py
Outdated
@@ -84,6 +84,7 @@ def get_libraries(cls, service_uid, absolute_uid, start=0, rows=None, sort_col=" | |||
.filter(Permissions.user_id == service_uid)\ | |||
.order_by(getattr(getattr(Library, sort_col), sort_order)())\ | |||
.all() | |||
libraries_response = {'libraries_count': len(result)} |
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.
Moving this because there was another result
variable further down. When returning len(result) the output wasn't the expected one.
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.
That makes sense. It was a good idea to rename the result below to owner_permissions
, owner
. I don't think you need to break the tuple here, but it might be worth giving this a more descriptive name as well.
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.
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.
Looks good! Seeing as you tweaked the names of the other variables, I think it could be worth giving result
a more descriptive name as well.
biblib/views/user_view.py
Outdated
@@ -84,6 +84,7 @@ def get_libraries(cls, service_uid, absolute_uid, start=0, rows=None, sort_col=" | |||
.filter(Permissions.user_id == service_uid)\ | |||
.order_by(getattr(getattr(Library, sort_col), sort_order)())\ | |||
.all() | |||
libraries_response = {'libraries_count': len(result)} |
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.
That makes sense. It was a good idea to rename the result below to owner_permissions
, owner
. I don't think you need to break the tuple here, but it might be worth giving this a more descriptive name as well.
if ownership: | ||
response['my_libraries'] = my_libraries | ||
response['shared_with_me'] = shared_with_me | ||
libraries_response['my_libraries'] = my_libraries |
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.
Giving this a more descriptive name was a good idea for readability.
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.
Looks good to me! Feel free to merge and version whenever you are ready.
No description provided.