ID-1324 Adjust response body of group sync endpoint

broadinstitute · Jul 8, 2024 · d090006 · d090006
1 parent e630c18
commit d090006
Show file tree

Hide file tree

Showing 3 changed files with 18 additions and 6 deletions.
diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutes.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutes.scala
@@ -249,7 +249,7 @@ trait GoogleExtensionRoutes extends ExtensionRoutes with SamUserDirectives with
                     .synchronizeGroupMembers(policyId, samRequestContext = samRequestContext)
                     .recover {
                       // If the group sync was already done previously, then no need to return any sync report items, just return 200
-                      case _: GroupAlreadySynchronized => Map.empty[WorkbenchEmail, Seq[SyncReportItem]]
+                      case exception: GroupAlreadySynchronized => Map(exception.group.email -> Seq.empty[SyncReportItem])
                     }
                     .map { syncReport =>
                       StatusCodes.OK -> syncReport

diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleGroupSynchronizer.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleGroupSynchronizer.scala
@@ -15,8 +15,10 @@ import org.broadinstitute.dsde.workbench.sam.util.OpenTelemetryIOUtils._
 import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext
 import org.broadinstitute.dsde.workbench.util.FutureSupport
 
-class GroupAlreadySynchronized(errorReport: ErrorReport = ErrorReport(StatusCodes.Conflict, "Group has already been synchronized"))
-    extends WorkbenchExceptionWithErrorReport(errorReport)
+case class GroupAlreadySynchronized(
+    group: WorkbenchGroup,
+    override val errorReport: ErrorReport = ErrorReport(StatusCodes.Conflict, "Group has already been synchronized")
+) extends WorkbenchExceptionWithErrorReport(errorReport)
 
 /** This class makes sure that our google groups have the right members.
   *
@@ -61,7 +63,7 @@ class GoogleGroupSynchronizer(
           if (group.version > group.lastSynchronizedVersion.getOrElse(0)) {
             IO.unit
           } else {
-            IO.raiseError(new GroupAlreadySynchronized)
+            IO.raiseError(new GroupAlreadySynchronized(group))
           }
         members <- calculateAuthDomainIntersectionIfRequired(group, samRequestContext)
         subGroupSyncs <- syncSubGroupsIfRequired(group, visitedGroups, samRequestContext)

diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala
@@ -286,7 +286,7 @@ class GoogleExtensionRoutesSpec extends GoogleExtensionRoutesSpecHelper with Sca
     }
   }
 
-  "GET /api/google/policy/{resourceTypeName}/{resourceId}/{accessPolicyName}/sync" should "200 with empty response when deduping sync requests" in {
+  "GET /api/google/policy/{resourceTypeName}/{resourceId}/{accessPolicyName}/sync" should "200 with group email when deduping sync requests" in {
     val resourceTypes = Map(resourceType.name -> resourceType)
     val (user, samDep, routes) = createTestUser(resourceTypes)
 
@@ -297,6 +297,14 @@ class GoogleExtensionRoutesSpec extends GoogleExtensionRoutesSpecHelper with Sca
       }
     }
 
+    import spray.json.DefaultJsonProtocol._
+    val createdPolicy = Get(s"/api/resource/${resourceType.name}/foo/policies") ~> routes.route ~> check {
+      status shouldEqual StatusCodes.OK
+      responseAs[Seq[AccessPolicyResponseEntry]]
+        .find(_.policyName == AccessPolicyName(resourceType.ownerRoleName.value))
+        .getOrElse(fail("created policy not returned by get request"))
+    }
+
     import spray.json.DefaultJsonProtocol._
     import SamGoogleModelJsonSupport._
     Post(s"/api/google/resource/${resourceType.name}/foo/${resourceType.ownerRoleName.value}/sync") ~> routes.route ~> check {
@@ -307,7 +315,9 @@ class GoogleExtensionRoutesSpec extends GoogleExtensionRoutesSpecHelper with Sca
     // A duplicate call should return OK and an empty response
     Post(s"/api/google/resource/${resourceType.name}/foo/${resourceType.ownerRoleName.value}/sync") ~> routes.route ~> check {
       status shouldEqual StatusCodes.OK
-      responseAs[Map[WorkbenchEmail, Seq[SyncReportItem]]] shouldBe empty
+      responseAs[Map[WorkbenchEmail, Seq[SyncReportItem]]] shouldEqual Map(
+        createdPolicy.email -> Seq.empty
+      )
     }
   }