From a78be765e4ba5745a1f3bc385a6c36f35b916506 Mon Sep 17 00:00:00 2001 From: Tyler Doyle Date: Thu, 29 Jun 2017 10:18:05 -0700 Subject: [PATCH 1/7] Cleaned up git history and made a new PR --- tableauserverclient/models/group_item.py | 3 ++- .../server/endpoint/groups_endpoint.py | 24 ++++++++++++------- tableauserverclient/server/pager.py | 8 ++++++- test/test_group.py | 3 +++ 4 files changed, 28 insertions(+), 10 deletions(-) diff --git a/tableauserverclient/models/group_item.py b/tableauserverclient/models/group_item.py index c0014eac0..ab627b569 100644 --- a/tableauserverclient/models/group_item.py +++ b/tableauserverclient/models/group_item.py @@ -33,7 +33,8 @@ def users(self): if self._users is None: error = "Group must be populated with users first." raise UnpopulatedPropertyError(error) - return self._users + # Each call to `.users` should create a new pager, this just runs the callable + return self._users() def _set_users(self, users): self._users = users diff --git a/tableauserverclient/server/endpoint/groups_endpoint.py b/tableauserverclient/server/endpoint/groups_endpoint.py index 243aa54c9..0839ff881 100644 --- a/tableauserverclient/server/endpoint/groups_endpoint.py +++ b/tableauserverclient/server/endpoint/groups_endpoint.py @@ -25,15 +25,29 @@ def get(self, req_options=None): # Gets all users in a given group @api(version="2.0") def populate_users(self, group_item, req_options=None): + from .. import Pager + if not group_item.id: error = "Group item missing ID. Group must be retrieved from server first." raise MissingRequiredFieldError(error) + + # populate_users (better named `iter_users`?) creates a new pager and wraps it in a function + # so we can call it again as needed. This is simplier than an object that manages it for us. + # If they need to adjust request options they can call populate_users again, otherwise they can just + # call `group_item.users` to get a new Pager, or list(group_item.users) if they need a list + + def user_pager(): + return Pager(lambda options: self._get_users_for_group(group_item, options), req_options) + + group_item._set_users(user_pager) + + def _get_users_for_group(self, group_item, req_options=None): url = "{0}/{1}/users".format(self.baseurl, group_item.id) server_response = self.get_request(url, req_options) - group_item._set_users(UserItem.from_response(server_response.content)) + user_item = UserItem.from_response(server_response.content) pagination_item = PaginationItem.from_response(server_response.content) logger.info('Populated users for group (ID: {0})'.format(group_item.id)) - return pagination_item + return user_item, pagination_item # Deletes 1 group by id @api(version="2.0") @@ -59,10 +73,6 @@ def remove_user(self, group_item, user_id): self._remove_user(group_item, user_id) try: users = group_item.users - for user in users: - if user.id == user_id: - users.remove(user) - break except UnpopulatedPropertyError: # If we aren't populated, do nothing to the user list pass @@ -74,8 +84,6 @@ def add_user(self, group_item, user_id): new_user = self._add_user(group_item, user_id) try: users = group_item.users - users.append(new_user) - group_item._set_users(users) except UnpopulatedPropertyError: # If we aren't populated, do nothing to the user list pass diff --git a/tableauserverclient/server/pager.py b/tableauserverclient/server/pager.py index 1a6bfe17c..61ad06d5f 100644 --- a/tableauserverclient/server/pager.py +++ b/tableauserverclient/server/pager.py @@ -9,7 +9,13 @@ class Pager(object): """ def __init__(self, endpoint, request_opts=None): - self._endpoint = endpoint.get + if hasattr(endpoint, 'get'): + # The simpliest case is to take an Endpoint and call its get + self._endpoint = endpoint.get + else: + # but if they pass a callable then use that instead (used internally) + self._endpoint = endpoint + self._options = request_opts # If we have options we could be starting on any page, backfill the count diff --git a/test/test_group.py b/test/test_group.py index 20c45455d..a4ef134a3 100644 --- a/test/test_group.py +++ b/test/test_group.py @@ -48,6 +48,7 @@ def test_get_before_signin(self): self.server._auth_token = None self.assertRaises(TSC.NotSignedInError, self.server.groups.get) + @unittest.skip("TODO: I need to mock Pager") def test_populate_users(self): with open(POPULATE_USERS, 'rb') as f: response_xml = f.read().decode('utf-8') @@ -69,6 +70,7 @@ def test_delete(self): m.delete(self.baseurl + '/e7833b48-c6f7-47b5-a2a7-36e7dd232758', status_code=204) self.server.groups.delete('e7833b48-c6f7-47b5-a2a7-36e7dd232758') + @unittest.skip("TODO: I need to mock Pager") def test_remove_user(self): with open(POPULATE_USERS, 'rb') as f: response_xml = f.read().decode('utf-8') @@ -85,6 +87,7 @@ def test_remove_user(self): self.assertEqual(0, len(single_group.users)) + @unittest.skip("TODO: I need to mock Pager") def test_add_user(self): with open(ADD_USER, 'rb') as f: response_xml = f.read().decode('utf-8') From 49705571cb6562982f3e45d26ce958a6533236b6 Mon Sep 17 00:00:00 2001 From: Tyler Doyle Date: Fri, 30 Jun 2017 10:01:54 -0700 Subject: [PATCH 2/7] Update docstring and ad a safety check --- tableauserverclient/server/pager.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tableauserverclient/server/pager.py b/tableauserverclient/server/pager.py index 61ad06d5f..d93bb8a0d 100644 --- a/tableauserverclient/server/pager.py +++ b/tableauserverclient/server/pager.py @@ -4,11 +4,18 @@ class Pager(object): """ - Generator that takes an endpoint with `.get` and lazily loads items from Server. - Supports all `RequestOptions` including starting on any page. + Generator that takes an endpoint (top level endpoints with `.get)` and lazily loads items from Server. + Supports all `RequestOptions` including starting on any page. Also used by models to load sub-models + (users in a group, views in a workbook, etc) by passing a different endpoint. + + Will loop over anything that returns (List[ModelItem], PaginationItem). """ def __init__(self, endpoint, request_opts=None): + + if not callable(endpoint): + raise ValueError("Pager needs a server endpoint to page through.") + if hasattr(endpoint, 'get'): # The simpliest case is to take an Endpoint and call its get self._endpoint = endpoint.get From f91e7251598e4f58269ad5118ebefc3d4bb09f5c Mon Sep 17 00:00:00 2001 From: Tyler Doyle Date: Fri, 30 Jun 2017 10:39:28 -0700 Subject: [PATCH 3/7] Fix my lameness --- tableauserverclient/server/pager.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tableauserverclient/server/pager.py b/tableauserverclient/server/pager.py index d93bb8a0d..81cdee2b9 100644 --- a/tableauserverclient/server/pager.py +++ b/tableauserverclient/server/pager.py @@ -13,16 +13,14 @@ class Pager(object): def __init__(self, endpoint, request_opts=None): - if not callable(endpoint): - raise ValueError("Pager needs a server endpoint to page through.") - if hasattr(endpoint, 'get'): # The simpliest case is to take an Endpoint and call its get self._endpoint = endpoint.get else: # but if they pass a callable then use that instead (used internally) self._endpoint = endpoint - + if not callable(endpoint): + raise ValueError("Pager needs a server endpoint to page through.") self._options = request_opts # If we have options we could be starting on any page, backfill the count From 74fcd9c23cca87d2a5984d9414519d96960fc248 Mon Sep 17 00:00:00 2001 From: Tyler Doyle Date: Fri, 30 Jun 2017 13:43:42 -0700 Subject: [PATCH 4/7] Checkpoint: Tests work now, I don't need to mock anything other than responses. Still need to add a len menthod to pager --- .../server/endpoint/groups_endpoint.py | 12 +--- test/assets/group_populate_users_empty.xml | 6 ++ test/assets/group_users_added.xml | 7 +++ test/test_group.py | 61 +++++++++++-------- 4 files changed, 50 insertions(+), 36 deletions(-) create mode 100644 test/assets/group_populate_users_empty.xml create mode 100644 test/assets/group_users_added.xml diff --git a/tableauserverclient/server/endpoint/groups_endpoint.py b/tableauserverclient/server/endpoint/groups_endpoint.py index 0839ff881..4979b3e5a 100644 --- a/tableauserverclient/server/endpoint/groups_endpoint.py +++ b/tableauserverclient/server/endpoint/groups_endpoint.py @@ -71,22 +71,12 @@ def create(self, group_item): @api(version="2.0") def remove_user(self, group_item, user_id): self._remove_user(group_item, user_id) - try: - users = group_item.users - except UnpopulatedPropertyError: - # If we aren't populated, do nothing to the user list - pass logger.info('Removed user (id: {0}) from group (ID: {1})'.format(user_id, group_item.id)) # Adds 1 user to 1 group @api(version="2.0") def add_user(self, group_item, user_id): - new_user = self._add_user(group_item, user_id) - try: - users = group_item.users - except UnpopulatedPropertyError: - # If we aren't populated, do nothing to the user list - pass + self._add_user(group_item, user_id) logger.info('Added user (id: {0}) to group (ID: {1})'.format(user_id, group_item.id)) def _remove_user(self, group_item, user_id): diff --git a/test/assets/group_populate_users_empty.xml b/test/assets/group_populate_users_empty.xml new file mode 100644 index 000000000..5d921df03 --- /dev/null +++ b/test/assets/group_populate_users_empty.xml @@ -0,0 +1,6 @@ + + + + + + \ No newline at end of file diff --git a/test/assets/group_users_added.xml b/test/assets/group_users_added.xml new file mode 100644 index 000000000..41768d1c9 --- /dev/null +++ b/test/assets/group_users_added.xml @@ -0,0 +1,7 @@ + + + + + + + \ No newline at end of file diff --git a/test/test_group.py b/test/test_group.py index a4ef134a3..7df6e1210 100644 --- a/test/test_group.py +++ b/test/test_group.py @@ -9,7 +9,9 @@ GET_XML = os.path.join(TEST_ASSET_DIR, 'group_get.xml') POPULATE_USERS = os.path.join(TEST_ASSET_DIR, 'group_populate_users.xml') +POPULATE_USERS_EMPTY = os.path.join(TEST_ASSET_DIR, 'group_populate_users_empty.xml') ADD_USER = os.path.join(TEST_ASSET_DIR, 'group_add_user.xml') +ADD_USER_POPULATE = os.path.join(TEST_ASSET_DIR, 'group_users_added.xml') CREATE_GROUP = os.path.join(TEST_ASSET_DIR, 'group_create.xml') CREATE_GROUP_ASYNC = os.path.join(TEST_ASSET_DIR, 'group_create_async.xml') @@ -48,61 +50,70 @@ def test_get_before_signin(self): self.server._auth_token = None self.assertRaises(TSC.NotSignedInError, self.server.groups.get) - @unittest.skip("TODO: I need to mock Pager") + def test_populate_users(self): with open(POPULATE_USERS, 'rb') as f: response_xml = f.read().decode('utf-8') with requests_mock.mock() as m: - m.get(self.baseurl + '/e7833b48-c6f7-47b5-a2a7-36e7dd232758/users', text=response_xml) + m.get(self.baseurl + '/e7833b48-c6f7-47b5-a2a7-36e7dd232758/users?pageNumber=1&pageSize=100&sort=name:asc', text=response_xml, complete_qs=True) single_group = TSC.GroupItem(name='Test Group') single_group._id = 'e7833b48-c6f7-47b5-a2a7-36e7dd232758' - pagination_item = self.server.groups.populate_users(single_group) + self.server.groups.populate_users(single_group) - self.assertEqual(1, pagination_item.total_available) - user = single_group.users.pop() - self.assertEqual('dd2239f6-ddf1-4107-981a-4cf94e415794', user.id) - self.assertEqual('alice', user.name) - self.assertEqual('Publisher', user.site_role) - self.assertEqual('2016-08-16T23:17:06Z', format_datetime(user.last_login)) + self.assertEqual(1, len(list(single_group.users))) + user = list(single_group.users).pop() + self.assertEqual('dd2239f6-ddf1-4107-981a-4cf94e415794', user.id) + self.assertEqual('alice', user.name) + self.assertEqual('Publisher', user.site_role) + self.assertEqual('2016-08-16T23:17:06Z', format_datetime(user.last_login)) def test_delete(self): with requests_mock.mock() as m: m.delete(self.baseurl + '/e7833b48-c6f7-47b5-a2a7-36e7dd232758', status_code=204) self.server.groups.delete('e7833b48-c6f7-47b5-a2a7-36e7dd232758') - @unittest.skip("TODO: I need to mock Pager") def test_remove_user(self): with open(POPULATE_USERS, 'rb') as f: - response_xml = f.read().decode('utf-8') + response_xml_populate = f.read().decode('utf-8') + + with open(POPULATE_USERS_EMPTY, 'rb') as f: + response_xml_empty = f.read().decode('utf-8') + with requests_mock.mock() as m: url = self.baseurl + '/e7833b48-c6f7-47b5-a2a7-36e7dd232758/users' \ '/dd2239f6-ddf1-4107-981a-4cf94e415794' + m.delete(url, status_code=204) - m.get(self.baseurl + '/e7833b48-c6f7-47b5-a2a7-36e7dd232758/users', text=response_xml) + # We register the get endpoint twice. The first time we have 1 user, the second we have 'removed' them. + m.get(self.baseurl + '/e7833b48-c6f7-47b5-a2a7-36e7dd232758/users', text=response_xml_populate) + single_group = TSC.GroupItem('test') single_group._id = 'e7833b48-c6f7-47b5-a2a7-36e7dd232758' self.server.groups.populate_users(single_group) - self.assertEqual(1, len(single_group.users)) + self.assertEqual(1, len(list(single_group.users))) self.server.groups.remove_user(single_group, 'dd2239f6-ddf1-4107-981a-4cf94e415794') + + m.get(self.baseurl + '/e7833b48-c6f7-47b5-a2a7-36e7dd232758/users', text=response_xml_empty) + self.assertEqual(0, len(list(single_group.users))) - self.assertEqual(0, len(single_group.users)) - - @unittest.skip("TODO: I need to mock Pager") def test_add_user(self): with open(ADD_USER, 'rb') as f: - response_xml = f.read().decode('utf-8') + response_xml_add = f.read().decode('utf-8') + with open(ADD_USER_POPULATE, 'rb') as f: + response_xml_populate = f.read().decode('utf-8') with requests_mock.mock() as m: - m.post(self.baseurl + '/e7833b48-c6f7-47b5-a2a7-36e7dd232758/users', text=response_xml) + m.post(self.baseurl + '/e7833b48-c6f7-47b5-a2a7-36e7dd232758/users', text=response_xml_add) + m.get(self.baseurl + '/e7833b48-c6f7-47b5-a2a7-36e7dd232758/users', text=response_xml_populate) single_group = TSC.GroupItem('test') single_group._id = 'e7833b48-c6f7-47b5-a2a7-36e7dd232758' - single_group._users = [] - self.server.groups.add_user(single_group, '5de011f8-5aa9-4d5b-b991-f462c8dd6bb7') - self.assertEqual(1, len(single_group.users)) - user = single_group.users.pop() - self.assertEqual('5de011f8-5aa9-4d5b-b991-f462c8dd6bb7', user.id) - self.assertEqual('testuser', user.name) - self.assertEqual('ServerAdministrator', user.site_role) + self.server.groups.add_user(single_group, '5de011f8-5aa9-4d5b-b991-f462c8dd6bb7') + self.server.groups.populate_users(single_group) + self.assertEqual(1, len(list(single_group.users))) + user = list(single_group.users).pop() + self.assertEqual('5de011f8-5aa9-4d5b-b991-f462c8dd6bb7', user.id) + self.assertEqual('testuser', user.name) + self.assertEqual('ServerAdministrator', user.site_role) def test_add_user_before_populating(self): with open(GET_XML, 'rb') as f: From a199e74ec57b0caeffe7834b14ba85c32b611957 Mon Sep 17 00:00:00 2001 From: Tyler Doyle Date: Fri, 30 Jun 2017 14:18:04 -0700 Subject: [PATCH 5/7] fix style --- test/test_group.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/test_group.py b/test/test_group.py index 7df6e1210..4886634fc 100644 --- a/test/test_group.py +++ b/test/test_group.py @@ -50,12 +50,12 @@ def test_get_before_signin(self): self.server._auth_token = None self.assertRaises(TSC.NotSignedInError, self.server.groups.get) - def test_populate_users(self): with open(POPULATE_USERS, 'rb') as f: response_xml = f.read().decode('utf-8') with requests_mock.mock() as m: - m.get(self.baseurl + '/e7833b48-c6f7-47b5-a2a7-36e7dd232758/users?pageNumber=1&pageSize=100&sort=name:asc', text=response_xml, complete_qs=True) + m.get(self.baseurl + '/e7833b48-c6f7-47b5-a2a7-36e7dd232758/users?pageNumber=1&pageSize=100&sort=name:asc', + text=response_xml, complete_qs=True) single_group = TSC.GroupItem(name='Test Group') single_group._id = 'e7833b48-c6f7-47b5-a2a7-36e7dd232758' self.server.groups.populate_users(single_group) @@ -75,14 +75,14 @@ def test_delete(self): def test_remove_user(self): with open(POPULATE_USERS, 'rb') as f: response_xml_populate = f.read().decode('utf-8') - + with open(POPULATE_USERS_EMPTY, 'rb') as f: response_xml_empty = f.read().decode('utf-8') - + with requests_mock.mock() as m: url = self.baseurl + '/e7833b48-c6f7-47b5-a2a7-36e7dd232758/users' \ '/dd2239f6-ddf1-4107-981a-4cf94e415794' - + m.delete(url, status_code=204) # We register the get endpoint twice. The first time we have 1 user, the second we have 'removed' them. m.get(self.baseurl + '/e7833b48-c6f7-47b5-a2a7-36e7dd232758/users', text=response_xml_populate) @@ -92,7 +92,7 @@ def test_remove_user(self): self.server.groups.populate_users(single_group) self.assertEqual(1, len(list(single_group.users))) self.server.groups.remove_user(single_group, 'dd2239f6-ddf1-4107-981a-4cf94e415794') - + m.get(self.baseurl + '/e7833b48-c6f7-47b5-a2a7-36e7dd232758/users', text=response_xml_empty) self.assertEqual(0, len(list(single_group.users))) From 715aa2584912a87bd617b4b776ee6c651dbaa10b Mon Sep 17 00:00:00 2001 From: Tyler Doyle Date: Wed, 5 Jul 2017 11:35:06 -0700 Subject: [PATCH 6/7] Final cleanup --- .../server/endpoint/groups_endpoint.py | 21 ++++++------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/tableauserverclient/server/endpoint/groups_endpoint.py b/tableauserverclient/server/endpoint/groups_endpoint.py index 4979b3e5a..7c1a25c64 100644 --- a/tableauserverclient/server/endpoint/groups_endpoint.py +++ b/tableauserverclient/server/endpoint/groups_endpoint.py @@ -31,10 +31,7 @@ def populate_users(self, group_item, req_options=None): error = "Group item missing ID. Group must be retrieved from server first." raise MissingRequiredFieldError(error) - # populate_users (better named `iter_users`?) creates a new pager and wraps it in a function - # so we can call it again as needed. This is simplier than an object that manages it for us. - # If they need to adjust request options they can call populate_users again, otherwise they can just - # call `group_item.users` to get a new Pager, or list(group_item.users) if they need a list + # Define an inner function that we bind to the model_item's `.user` property. def user_pager(): return Pager(lambda options: self._get_users_for_group(group_item, options), req_options) @@ -70,16 +67,6 @@ def create(self, group_item): # Removes 1 user from 1 group @api(version="2.0") def remove_user(self, group_item, user_id): - self._remove_user(group_item, user_id) - logger.info('Removed user (id: {0}) from group (ID: {1})'.format(user_id, group_item.id)) - - # Adds 1 user to 1 group - @api(version="2.0") - def add_user(self, group_item, user_id): - self._add_user(group_item, user_id) - logger.info('Added user (id: {0}) to group (ID: {1})'.format(user_id, group_item.id)) - - def _remove_user(self, group_item, user_id): if not group_item.id: error = "Group item missing ID." raise MissingRequiredFieldError(error) @@ -88,8 +75,11 @@ def _remove_user(self, group_item, user_id): raise ValueError(error) url = "{0}/{1}/users/{2}".format(self.baseurl, group_item.id, user_id) self.delete_request(url) + logger.info('Removed user (id: {0}) from group (ID: {1})'.format(user_id, group_item.id)) - def _add_user(self, group_item, user_id): + # Adds 1 user to 1 group + @api(version="2.0") + def add_user(self, group_item, user_id): if not group_item.id: error = "Group item missing ID." raise MissingRequiredFieldError(error) @@ -100,3 +90,4 @@ def _add_user(self, group_item, user_id): add_req = RequestFactory.Group.add_user_req(user_id) server_response = self.post_request(url, add_req) return UserItem.from_response(server_response.content).pop() + logger.info('Added user (id: {0}) to group (ID: {1})'.format(user_id, group_item.id)) From 2003bbbb0635e4b915f529a36e30ce39db4e3a1b Mon Sep 17 00:00:00 2001 From: Tyler Doyle Date: Wed, 5 Jul 2017 12:11:49 -0700 Subject: [PATCH 7/7] PR feedback --- tableauserverclient/server/pager.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tableauserverclient/server/pager.py b/tableauserverclient/server/pager.py index 81cdee2b9..8d988f483 100644 --- a/tableauserverclient/server/pager.py +++ b/tableauserverclient/server/pager.py @@ -16,11 +16,13 @@ def __init__(self, endpoint, request_opts=None): if hasattr(endpoint, 'get'): # The simpliest case is to take an Endpoint and call its get self._endpoint = endpoint.get - else: + elif callable(endpoint): # but if they pass a callable then use that instead (used internally) self._endpoint = endpoint - if not callable(endpoint): - raise ValueError("Pager needs a server endpoint to page through.") + else: + # Didn't get something we can page over + raise ValueError("Pager needs a server endpoint to page through.") + self._options = request_opts # If we have options we could be starting on any page, backfill the count