From a4d62de7d01ed1aeec86f59601a17222f9748829 Mon Sep 17 00:00:00 2001 From: femalves Date: Fri, 2 Feb 2024 18:38:15 +0000 Subject: [PATCH 1/4] added the type param and fixed tests --- .../test_deletion_abuse_epic.py | 2 +- .../functional_tests/test_deletion_epic.py | 4 +-- .../test_returned_data_epic.py | 4 +-- biblib/tests/unit_tests/test_views.py | 13 ++++--- biblib/tests/unit_tests/test_webservices.py | 4 +-- biblib/views/user_view.py | 36 ++++++++----------- 6 files changed, 28 insertions(+), 35 deletions(-) diff --git a/biblib/tests/functional_tests/test_deletion_abuse_epic.py b/biblib/tests/functional_tests/test_deletion_abuse_epic.py index 0ea4fde..d1784d0 100644 --- a/biblib/tests/functional_tests/test_deletion_abuse_epic.py +++ b/biblib/tests/functional_tests/test_deletion_abuse_epic.py @@ -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) \ No newline at end of file diff --git a/biblib/tests/functional_tests/test_deletion_epic.py b/biblib/tests/functional_tests/test_deletion_epic.py index 34d0e0a..6b87183 100644 --- a/biblib/tests/functional_tests/test_deletion_epic.py +++ b/biblib/tests/functional_tests/test_deletion_epic.py @@ -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) @@ -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) \ No newline at end of file diff --git a/biblib/tests/functional_tests/test_returned_data_epic.py b/biblib/tests/functional_tests/test_returned_data_epic.py index 554a288..8f5b1fc 100644 --- a/biblib/tests/functional_tests/test_returned_data_epic.py +++ b/biblib/tests/functional_tests/test_returned_data_epic.py @@ -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) @@ -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 ) diff --git a/biblib/tests/unit_tests/test_views.py b/biblib/tests/unit_tests/test_views.py index fb228ad..6b88843 100644 --- a/biblib/tests/unit_tests/test_views.py +++ b/biblib/tests/unit_tests/test_views.py @@ -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): """ @@ -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) @@ -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(): @@ -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}' @@ -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}' @@ -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): """ diff --git a/biblib/tests/unit_tests/test_webservices.py b/biblib/tests/unit_tests/test_webservices.py index 4aacb3a..2a2fff3 100644 --- a/biblib/tests/unit_tests/test_webservices.py +++ b/biblib/tests/unit_tests/test_webservices.py @@ -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 @@ -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'], diff --git a/biblib/views/user_view.py b/biblib/views/user_view.py index eaf3589..bb8393a 100644 --- a/biblib/views/user_view.py +++ b/biblib/views/user_view.py @@ -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 @@ -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: # For this library get all the people who have permissions users = session.query(Permissions).filter_by( @@ -152,17 +147,13 @@ def get_libraries(cls, service_uid, absolute_uid, start=0, rows=None, sort_col=" num_users=num_users, owner=owner ) + current_app.logger.debug('Type: {0}.'.format(type)) + if type == 'all' or (type == 'owner' and main_permission in ['owner']) or (type == 'collaborator' and main_permission in ['admin', 'read', 'write']): + libraries.append(payload) - 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 rows: libraries = libraries[start: start+rows] + elif start > 0: libraries = libraries[start:] + libraries_response = {'count': len(libraries), 'libraries': libraries} return libraries_response @@ -176,6 +167,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: @@ -228,8 +221,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) + 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) @@ -245,7 +239,7 @@ def get(self): rows=rows, sort_col=sort_col, sort_order=sort_order, - ownership=ownership) + type=type) return response, 200 def post(self): From f8a11cd7e6b310552806c0226d2f3a98b66bdc6f Mon Sep 17 00:00:00 2001 From: femalves Date: Fri, 2 Feb 2024 18:51:30 +0000 Subject: [PATCH 2/4] fixing count error --- biblib/tests/unit_tests/test_views.py | 2 +- biblib/views/user_view.py | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/biblib/tests/unit_tests/test_views.py b/biblib/tests/unit_tests/test_views.py index 6b88843..188ffd0 100644 --- a/biblib/tests/unit_tests/test_views.py +++ b/biblib/tests/unit_tests/test_views.py @@ -474,7 +474,7 @@ def test_user_can_retrieve_all_libraries_by_paging(self): rows=10 ) libraries += curr_libraries['libraries'] - total_libraries += curr_libraries['count'] + total_libraries = curr_libraries['count'] self.assertEqual(total_libraries, 100) self.assertEqual(libraries_full, libraries) diff --git a/biblib/views/user_view.py b/biblib/views/user_view.py index bb8393a..0180df0 100644 --- a/biblib/views/user_view.py +++ b/biblib/views/user_view.py @@ -147,13 +147,14 @@ def get_libraries(cls, service_uid, absolute_uid, start=0, rows=None, sort_col=" num_users=num_users, owner=owner ) - current_app.logger.debug('Type: {0}.'.format(type)) + if type == 'all' or (type == 'owner' and main_permission in ['owner']) or (type == 'collaborator' and main_permission in ['admin', 'read', 'write']): libraries.append(payload) - + + count = len(libraries) if rows: libraries = libraries[start: start+rows] elif start > 0: libraries = libraries[start:] - libraries_response = {'count': len(libraries), 'libraries': libraries} + libraries_response = {'count': count, 'libraries': libraries} return libraries_response From 00178caa63dbefbe5c79e90d6415a25f02593e4b Mon Sep 17 00:00:00 2001 From: femalves Date: Fri, 2 Feb 2024 21:46:13 +0000 Subject: [PATCH 3/4] improving efficiency a little bit --- biblib/views/user_view.py | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/biblib/views/user_view.py b/biblib/views/user_view.py index 0180df0..1d86dc6 100644 --- a/biblib/views/user_view.py +++ b/biblib/views/user_view.py @@ -58,6 +58,21 @@ def retrieve_user_email(owner_absolute_uid): service ) return response + + @classmethod + def get_user_libraries(cls, session, service_uid, sort_col, sort_order, type): + + query = session.query(Permissions, Library)\ + .join(Permissions.library)\ + .filter(Permissions.user_id == service_uid) + + if type == 'owner': + query = query.filter(Permissions.permissions['owner'].astext.cast(Boolean).is_(True)) + elif type != 'all': + query = query.filter(Permissions.permissions['owner'].astext.cast(Boolean).is_(False)) + + return query.order_by(getattr(getattr(Library, sort_col), sort_order)()).all() + @classmethod def get_libraries(cls, service_uid, absolute_uid, start=0, rows=None, sort_col="date_created", sort_order="desc", type="all"): @@ -79,12 +94,9 @@ def get_libraries(cls, service_uid, absolute_uid, start=0, rows=None, sort_col=" # The nested getattr calls allow us to request a column from the library model, # and then request the proper sort order from that column. with current_app.session_scope() as session: - user_libraries = session.query(Permissions, Library)\ - .join(Permissions.library)\ - .filter(Permissions.user_id == service_uid)\ - .order_by(getattr(getattr(Library, sort_col), sort_order)())\ - .all() - + + user_libraries = cls.get_user_libraries(session, service_uid, sort_col, sort_order, type) + libraries = [] for permission, library in user_libraries: @@ -150,7 +162,7 @@ def get_libraries(cls, service_uid, absolute_uid, start=0, rows=None, sort_col=" if type == 'all' or (type == 'owner' and main_permission in ['owner']) or (type == 'collaborator' and main_permission in ['admin', 'read', 'write']): libraries.append(payload) - + count = len(libraries) if rows: libraries = libraries[start: start+rows] elif start > 0: libraries = libraries[start:] From ea317cc7d230858385d30c1ff0e00dca693f44bb Mon Sep 17 00:00:00 2001 From: femalves Date: Mon, 5 Feb 2024 15:37:56 +0000 Subject: [PATCH 4/4] fixing bugs --- biblib/views/user_view.py | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/biblib/views/user_view.py b/biblib/views/user_view.py index 1d86dc6..2e9622d 100644 --- a/biblib/views/user_view.py +++ b/biblib/views/user_view.py @@ -60,22 +60,33 @@ def retrieve_user_email(owner_absolute_uid): return response @classmethod - def get_user_libraries(cls, session, service_uid, sort_col, sort_order, type): + def get_user_libraries(cls, session, service_uid, sort_col, sort_order, access_type, start=0, rows=None): query = session.query(Permissions, Library)\ .join(Permissions.library)\ .filter(Permissions.user_id == service_uid) - if type == 'owner': + if access_type == 'owner': query = query.filter(Permissions.permissions['owner'].astext.cast(Boolean).is_(True)) - elif type != 'all': + elif access_type == 'collaborator': query = query.filter(Permissions.permissions['owner'].astext.cast(Boolean).is_(False)) + + # Sorting + query = query.order_by(getattr(getattr(Library, sort_col), sort_order)()) + + count = query.count() + + # Pagination + if start > 0: + query = query.offset(start) + if rows: + query = query.limit(rows) - return query.order_by(getattr(getattr(Library, sort_col), sort_order)()).all() + return query.all(), count @classmethod - def get_libraries(cls, service_uid, absolute_uid, start=0, rows=None, sort_col="date_created", sort_order="desc", type="all"): + def get_libraries(cls, service_uid, absolute_uid, start=0, rows=None, sort_col="date_created", sort_order="desc", access_type="all"): """ Get all the libraries a user has :param service_uid: microservice UID of the user @@ -95,7 +106,7 @@ def get_libraries(cls, service_uid, absolute_uid, start=0, rows=None, sort_col=" # and then request the proper sort order from that column. with current_app.session_scope() as session: - user_libraries = cls.get_user_libraries(session, service_uid, sort_col, sort_order, type) + user_libraries, count = cls.get_user_libraries(session, service_uid, sort_col, sort_order, access_type, start, rows) libraries = [] for permission, library in user_libraries: @@ -160,12 +171,8 @@ def get_libraries(cls, service_uid, absolute_uid, start=0, rows=None, sort_col=" owner=owner ) - if type == 'all' or (type == 'owner' and main_permission in ['owner']) or (type == 'collaborator' and main_permission in ['admin', 'read', 'write']): - libraries.append(payload) + 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 @@ -180,7 +187,7 @@ 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) + :param access_type: Level of library ownership. default: all (all, owner, collaborator) :return: list of the users libraries with the relevant information @@ -234,8 +241,8 @@ def get(self): if sort_order not in ['asc', 'desc']: raise ValueError - type = get_params.get('type', default='all', type=str) - if type not in ['all', 'owner', 'collaborator']: + access_type = get_params.get('access_type', default='all', type=str) + if access_type not in ['all', 'owner', 'collaborator']: raise ValueError except ValueError: @@ -252,7 +259,7 @@ def get(self): rows=rows, sort_col=sort_col, sort_order=sort_order, - type=type) + access_type=access_type) return response, 200 def post(self):