tableau · t8y8 · Jul 5, 2017 · Jun 29, 2017 · Jun 30, 2017 · Jun 30, 2017
diff --git 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
@@ -25,15 +25,26 @@ 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)
+
+        # 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)
+
+        group_item._set_users(user_pager)
+
+    def _get_users_for_group(self, group_item, req_options=None):
         url =
8000
 "{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")
@@ -56,32 +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)
-        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
-        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
-            users.append(new_user)
-            group_item._set_users(users)
-        except UnpopulatedPropertyError:
-            # If we aren't populated, do nothing to the user list
-            pass
-        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)
@@ -90,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)
@@ -102,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))
diff --git a/tableauserverclient/server/pager.py b/tableauserverclient/server/pager.py
@@ -4,12 +4,25 @@
 
 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):
-        self._endpoint = endpoint.get
+
+        if hasattr(endpoint, 'get'):
+            # The simpliest case is to take an Endpoint and call its get
+            self._endpoint = endpoint.get
+        elif callable(endpoint):
+            # but if they pass a callable then use that instead (used internally)
+            self._endpoint = endpoint
+        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

diff --git a/test/assets/group_populate_users_empty.xml b/test/assets/group_populate_users_empty.xml
@@ -0,0 +1,6 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<tsResponse xmlns="http://tableau.com/api" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://tableau.com/api http://tableau.com/api/ts-api-2.3.xsd">
+    <pagination pageNumber="1" pageSize="100" totalAvailable="1" />
+    <users>
+    </users>
+</tsResponse>
diff --git a/test/assets/group_users_added.xml b/test/assets/group_users_added.xml
@@ -0,0 +1,7 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<tsResponse xmlns="http://tableau.com/api" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://tableau.com/api http://tableau.com/api/ts-api-2.3.xsd">
+    <pagination pageNumber="1" pageSize="100" totalAvailable="1" />
+    <users>
+        <user id="5de011f8-5aa9-4d5b-b991-f462c8dd6bb7" name="testuser" siteRole="ServerAdministrator" externalAuthUserId="" />
+    </users>
+</tsResponse>
diff --git 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')
 
@@ -52,17 +54,18 @@ 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:
@@ -71,35 +74,46 @@ def test_delete(self):
 
     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')
 
-        self.assertEqual(0, len(single_group.users))
+            m.get(self.baseurl + '/e7833b48-c6f7-47b5-a2a7-36e7dd232758/users', text=response_xml_empty)
+            self.assertEqual(0, len(list(single_group.users)))
 
     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: