Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Type param #177

Merged
merged 4 commits into from
Feb 5, 2024
Merged

Type param #177

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a4d62de
added the type param and fixed tests
femalves Feb 2, 2024
f8a11cd
fixing count error
femalves Feb 2, 2024
00178ca
improving efficiency a little bit
femalves Feb 2, 2024
ea317cc
fixing bugs
femalves Feb 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion biblib/tests/functional_tests/test_deletion_abuse_epic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ def test_deletion_abuse_epic(self):
url,
headers=stub_owner.headers
)
self.assertTrue(response.json['libraries_count'] == 0)
self.assertTrue(response.json['count'] == 0)
self.assertTrue(len(response.json['libraries']) == 0)
if __name__ == '__main__':
unittest.main(verbosity=2)
4 changes: 2 additions & 2 deletions biblib/tests/functional_tests/test_deletion_epic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ def test_deletion_epic(self):
url,
headers=stub_user.headers
)
self.assertTrue(response.json['libraries_count'] == 1)
self.assertTrue(response.json['count'] == 1)

# Deletes the first library
url = url_for('documentview', library=library_id_1)
Expand All @@ -110,7 +110,7 @@ def test_deletion_epic(self):
url,
headers=stub_user.headers
)
self.assertTrue(response.json['libraries_count'] == 0)
self.assertTrue(response.json['count'] == 0)

if __name__ == '__main__':
unittest.main(verbosity=2)
4 changes: 2 additions & 2 deletions biblib/tests/functional_tests/test_returned_data_epic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ def test_returned_data_user_view_epic(self):
headers=user_dave.headers
)
self.assertEqual(response.status_code, 200)
self.assertTrue(response.json['libraries_count'] == 1)
self.assertTrue(response.json['count'] == 1)
self.assertTrue(
response.json['libraries'][0]['num_documents']
== (number_of_documents+number_of_documents_second)
Expand Down Expand Up @@ -177,7 +177,7 @@ def test_returned_data_user_view_epic(self):
self.assertEqual(response.status_code, 200)
libraries = response.json

self.assertTrue(libraries['libraries_count'] == 1)
self.assertTrue(libraries['count'] == 1)
self.assertTrue(
libraries['libraries'][0]['num_documents'] == number_of_documents+1
)
Expand Down
13 changes: 6 additions & 7 deletions biblib/tests/unit_tests/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ def test_user_can_retrieve_library(self):
absolute_uid=user.absolute_uid
)

self.assertEqual(libraries['libraries_count'], number_of_libs)
self.assertEqual(libraries['count'], number_of_libs)

def test_user_can_retrieve_rows_number_of_libraries(self):
"""
Expand Down Expand Up @@ -474,8 +474,7 @@ def test_user_can_retrieve_all_libraries_by_paging(self):
rows=10
)
libraries += curr_libraries['libraries']
total_libraries = curr_libraries['libraries_count']

total_libraries = curr_libraries['count']
self.assertEqual(total_libraries, 100)
self.assertEqual(libraries_full, libraries)

Expand Down Expand Up @@ -562,7 +561,7 @@ def test_user_retrieves_correct_library_content_from_userview(self):
absolute_uid=user.absolute_uid
)

self.assertTrue(libraries['libraries_count'] == number_of_libs)
self.assertTrue(libraries['count'] == number_of_libs)

for library in libraries['libraries']:
for key in self.stub_library.user_view_get_response():
Expand Down Expand Up @@ -592,7 +591,7 @@ def test_user_retrieves_correct_library_content_from_userview(self):
sort_order='asc'
)

self.assertTrue(libraries['libraries_count'] == number_of_libs)
self.assertTrue(libraries['count'] == number_of_libs)
for library in libraries['libraries']:
for key in self.stub_library.user_view_get_response():
self.assertIn(key, library.keys(), 'Missing key: {0}'
Expand Down Expand Up @@ -621,7 +620,7 @@ def test_user_retrieves_correct_library_content_from_userview(self):
sort_order='asc'
)

self.assertTrue(libraries['libraries_count'] == number_of_libs)
self.assertTrue(libraries['count'] == number_of_libs)
for library in libraries['libraries']:
for key in self.stub_library.user_view_get_response():
self.assertIn(key, library.keys(), 'Missing key: {0}'
Expand Down Expand Up @@ -650,7 +649,7 @@ def test_user_retrieves_correct_library_content_from_userview(self):
absolute_uid=user_other.absolute_uid
)

self.assertTrue(libraries['libraries_count'] == 2)
self.assertTrue(libraries['count'] == 2)

def test_dates_of_updates_change_correctly(self):
"""
Expand Down
4 changes: 2 additions & 2 deletions biblib/tests/unit_tests/test_webservices.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2032,7 +2032,7 @@ def test_can_remove_a_library(self):
url,
headers=stub_user.headers
)
self.assertTrue(response.json['libraries_count'] == 0,
self.assertTrue(response.json['count'] == 0,
response.json)

# Check there is no document content
Expand Down Expand Up @@ -3173,7 +3173,7 @@ def test_when_user_has_classic_credentials_and_library_returns(self):
url,
headers=stub_user.headers
)
self.assertTrue(response.json['libraries_count'] > 0,
self.assertTrue(response.json['count'] > 0,
msg='No libraries returned: {}'.format(response.json))
self.assertEqual(response.json['libraries'][0]['name'], stub_library.name)
self.assertEqual(response.json['libraries'][0]['description'],
Expand Down
37 changes: 16 additions & 21 deletions biblib/views/user_view.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ def retrieve_user_email(owner_absolute_uid):
return response

@classmethod
def get_libraries(cls, service_uid, absolute_uid, start=0, rows=None, sort_col="date_created", sort_order="desc", ownership=False):
def get_libraries(cls, service_uid, absolute_uid, start=0, rows=None, sort_col="date_created", sort_order="desc", type="all"):
"""
Get all the libraries a user has
:param service_uid: microservice UID of the user
Expand All @@ -85,13 +85,8 @@ def get_libraries(cls, service_uid, absolute_uid, start=0, rows=None, sort_col="
.order_by(getattr(getattr(Library, sort_col), sort_order)())\
.all()

libraries_response = {'libraries_count': len(user_libraries)}

if rows: rows=start+rows

my_libraries = []
shared_with_me = []
for permission, library in user_libraries[start:rows]:
libraries = []
for permission, library in user_libraries:
shinyichen marked this conversation as resolved.
Show resolved Hide resolved

# For this library get all the people who have permissions
users = session.query(Permissions).filter_by(
Expand Down Expand Up @@ -153,16 +148,13 @@ def get_libraries(cls, service_uid, absolute_uid, start=0, rows=None, sort_col="
owner=owner
)

if (ownership and main_permission in ['owner']) or not ownership:
my_libraries.append(payload)
elif ownership and main_permission in ['admin', 'read', 'write']:
shared_with_me.append(payload)

if ownership:
libraries_response['my_libraries'] = my_libraries
libraries_response['shared_with_me'] = shared_with_me
else:
libraries_response['libraries'] = my_libraries
if type == 'all' or (type == 'owner' and main_permission in ['owner']) or (type == 'collaborator' and main_permission in ['admin', 'read', 'write']):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I am missing something, but do we still need this if with the filtering incorporated in the new method you wrote above?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, seems like this part and the slicing of list (below) are not needed anymore

libraries.append(payload)

count = len(libraries)
if rows: libraries = libraries[start: start+rows]
elif start > 0: libraries = libraries[start:]
libraries_response = {'count': count, 'libraries': libraries}

return libraries_response

Expand All @@ -176,6 +168,8 @@ def get(self):
:param rows: The number of rows to return from the start point (int). default: None (returns all libraries)
:param sort: Library column to sort on. default: date_created (date_created, date_last_modified, name)
:param order: Direction sort libraries. default: desc (asc, desc)
:param type: Level of library ownership. default: all (all, owner, collaborator)

:return: list of the users libraries with the relevant information

Header:
Expand Down Expand Up @@ -228,8 +222,9 @@ def get(self):
if sort_order not in ['asc', 'desc']:
raise ValueError

ownership = get_params.get('ownership', default=False, type=check_boolean)
current_app.logger.debug("GET params: {}, start: {}, end: {}".format(get_params, start, rows))
type = get_params.get('type', default='all', type=str)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern on this is that type is a python builtin, so it might be worth renaming to something safer in case type() becomes relevant in the future.

if type not in ['all', 'owner', 'collaborator']:
raise ValueError

except ValueError:
msg = "Failed to parse input parameters: {}. Please confirm the request is properly formatted.".format(request)
Expand All @@ -245,7 +240,7 @@ def get(self):
rows=rows,
sort_col=sort_col,
sort_order=sort_order,
ownership=ownership)
type=type)
return response, 200

def post(self):
Expand Down
Loading