Fix Issue 52904 (#80)

Author: labkey-alan

CHANGE.txt

@@ -9,6 +9,9 @@ What's New in the LabKey 3.4.0 package
 - Add "valueExpression" to PropertyDescriptor class
     - needed for creating/adding domain calculated fields
     - update Domain class to append calculatedFields to the domain's fields
+- Fix Issue 52904
+    - UnexpectedRedirectError is not wrapped with ServerContextError
+    - Allow redirects when deactivating users
 
 What's New in the LabKey 3.3.0 package
 ==============================

labkey/security.py

@@ -121,6 +121,7 @@ def deactivate_users(
         target_ids=target_ids,
         api="deactivateUsers.view",
         container_path=container_path,
+        allow_redirects=True,
     )
     if response is not None and response["status_code"] == 200:
         return dict(success=True)
@@ -360,7 +361,11 @@ def __make_security_role_api_request(
 
 
 def __make_user_api_request(
-    server_context: ServerContext, target_ids: List[int], api: str, container_path: str = None
+    server_context: ServerContext,
+    target_ids: List[int],
+    api: str,
+    container_path: str = None,
+    allow_redirects: bool = False,
 ):
     """
     Make a request to the LabKey User Controller
@@ -372,7 +377,7 @@ def __make_user_api_request(
     """
     url = server_context.build_url(USER_CONTROLLER, api, container_path)
 
-    return server_context.make_request(url, {"userId": target_ids})
+    return server_context.make_request(url, {"userId": target_ids}, allow_redirects=allow_redirects)
 
 
 class SecurityWrapper:

labkey/server_context.py

@@ -164,7 +164,12 @@ def webdav_client(self, webdav_options: dict = None):
         return client
 
     def handle_request_exception(self, exception):
-        if type(exception) in [RequestAuthorizationError, QueryNotFoundError, ServerNotFoundError]:
+        if type(exception) in [
+            RequestAuthorizationError,
+            QueryNotFoundError,
+            ServerNotFoundError,
+            UnexpectedRedirectError,
+        ]:
             raise exception
 
         raise ServerContextError(self, exception)

test/integration/test_domain.py

@@ -265,8 +265,20 @@ def test_domain_save_options(api: APIWrapper, list_fixture):
 
 def test_domain_add_calculated_field(api: APIWrapper, list_fixture):
     domain, options = api.domain.get_domain_details(LISTS_SCHEMA, LIST_NAME)
-    domain.add_field({"name": "calcDouble", "conceptURI": "http://www.labkey.org/exp/xml#calculated", "valueExpression": "rowId * 2"})
-    domain.add_field({"name": "calcCube", "conceptURI": "http://www.labkey.org/exp/xml#calculated", "valueExpression": "power(rowId, 3)"})
+    domain.add_field(
+        {
+            "name": "calcDouble",
+            "conceptURI": "http://www.labkey.org/exp/xml#calculated",
+            "valueExpression": "rowId * 2",
+        }
+    )
+    domain.add_field(
+        {
+            "name": "calcCube",
+            "conceptURI": "http://www.labkey.org/exp/xml#calculated",
+            "valueExpression": "power(rowId, 3)",
+        }
+    )
 
     api.domain.save(LISTS_SCHEMA, LIST_NAME, domain, options=options)
     saved_domain = api.domain.get(LISTS_SCHEMA, LIST_NAME)

test/integration/test_query.py

@@ -158,6 +158,7 @@ def test_cannot_delete_qc_state_in_use(api: APIWrapper, qc_states, study, datase
     dataset_row_to_remove = [{"lsid": inserted_lsid}]
     api.query.delete_rows(SCHEMA_NAME, QUERY_NAME, dataset_row_to_remove)
 
+
 LISTS_SCHEMA = "lists"
 PARENT_LIST_NAME = "parent_list"
 PARENT_LIST_DEFINITION = {
@@ -214,6 +215,7 @@ def test_cannot_delete_qc_state_in_use(api: APIWrapper, qc_states, study, datase
 child_three,parent_three
 """
 
+
 @pytest.fixture
 def parent_list_fixture(api: APIWrapper):
     api.domain.create(PARENT_LIST_DEFINITION)
@@ -251,11 +253,16 @@ def test_import_rows(api: APIWrapper, parent_list_fixture, child_list_fixture, t
     child_file.close()
     assert resp["success"] == False
     assert resp["errorCount"] == 1
-    assert resp["errors"][0]["exception"] == "Could not convert value 'parent_one' (String) for Integer field 'parent'"
+    assert (
+        resp["errors"][0]["exception"]
+        == "Could not convert value 'parent_one' (String) for Integer field 'parent'"
+    )
 
     # Should pass, because import_lookup_by_alternate_key is True
     child_file = child_data_path.open()
-    resp = api.query.import_rows("lists", CHILD_LIST_NAME, data_file=child_file, import_lookup_by_alternate_key=True)
+    resp = api.query.import_rows(
+        "lists", CHILD_LIST_NAME, data_file=child_file, import_lookup_by_alternate_key=True
+    )
     child_file.close()
     assert resp["success"] == True
     assert resp["rowCount"] == 3

test/integration/test_security.py

@@ -6,8 +6,10 @@
 # JavaClientApiTest.testImpersonationConnection()
 
 pytestmark = pytest.mark.integration  # Mark all tests in this module as integration tests
-TEST_EMAIL = "test_user@test.test"
+TEST_EMAIL = "test_user@example.com"
 TEST_DISPLAY_NAME = "test user"
+DEACTIVATED_EMAIL = "deactivated_user@example.com"
+DEACTIVATED_DISPLAY_NAME = "deactivated user"
 
 
 @pytest.fixture(scope="session")
@@ -17,7 +19,7 @@ def test_user(api: APIWrapper, project):
     user_id = resp["userId"]
     yield {"id": user_id, "email": TEST_EMAIL, "display_name": TEST_DISPLAY_NAME}
     url = api.server_context.build_url("security", "deleteUser.api", container_path="/")
-    resp = api.server_context.make_request(url, {"id": user_id})
+    api.server_context.make_request(url, {"id": user_id})
 
 
 def test_impersonation(api: APIWrapper, test_user):
@@ -44,3 +46,22 @@ def test_impersonation(api: APIWrapper, test_user):
 
     # We need to stop impersonating a user before leaving so we don't mess up other tests.
     api.security.stop_impersonating()
+
+
+@pytest.fixture(scope="module")
+def deactivated_user(api: APIWrapper, project):
+    url = api.server_context.build_url("security", "createNewUser.api")
+    resp = api.server_context.make_request(url, {"email": DEACTIVATED_EMAIL, "sendEmail": False})
+    user_id = resp["userId"]
+    yield {"id": user_id, "email": DEACTIVATED_EMAIL, "display_name": DEACTIVATED_DISPLAY_NAME}
+    url = api.server_context.build_url("security", "deleteUser.api", container_path="/")
+    api.server_context.make_request(url, {"id": user_id})
+
+
+def test_issue_52904(api: APIWrapper, deactivated_user):
+    resp = api.security.deactivate_users(target_ids=[deactivated_user["id"]])
+    assert resp["success"] is True
+
+    # Deactivating again shouldn't issue a redirect
+    resp = api.security.deactivate_users(target_ids=[deactivated_user["id"]])
+    assert resp["success"] is True

test/unit/test_security.py

@@ -287,7 +287,7 @@ def setUp(self):
             "data": {"userId": [123]},
             "headers": None,
             "timeout": 300,
-            "allow_redirects": False,
+            "allow_redirects": True,
         }
 
         self.args = [mock_server_context(self.service), self.__user_id]