CORE-282 sort top groups (#1643)

dvoet · web-flow · commit ff4c95290be8 · 2025-02-14T14:14:24.000-05:00
diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/DirectoryDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/DirectoryDAO.scala
@@ -11,7 +11,7 @@ import org.broadinstitute.dsde.workbench.sam.azure.{
   PetManagedIdentity,
   PetManagedIdentityId
 }
-import org.broadinstitute.dsde.workbench.sam.model.api.{AdminUpdateUserRequest, SamUser, SamUserAttributes}
+import org.broadinstitute.dsde.workbench.sam.model.api.{AdminUpdateUserRequest, GroupMembershipCount, SamUser, SamUserAttributes}
 import org.broadinstitute.dsde.workbench.sam.model.{BasicWorkbenchGroup, FullyQualifiedResourceId, ResourceAction, ResourceTypeName, SamUserTos}
 import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext
 
@@ -217,5 +217,5 @@ trait DirectoryDAO {
     * @return
     *   a map of group to membership count
     */
-  def listGroupsContributingToMostMemberships(samUser: SamUser, limit: Int, samRequestContext: SamRequestContext): IO[Map[WorkbenchGroupIdentity, Int]]
+  def listGroupsContributingToMostMemberships(samUser: SamUser, limit: Int, samRequestContext: SamRequestContext): IO[List[GroupMembershipCount]]
 }
diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala
@@ -24,7 +24,7 @@ import org.broadinstitute.dsde.workbench.sam.db.SamTypeBinders._
 import org.broadinstitute.dsde.workbench.sam.db._
 import org.broadinstitute.dsde.workbench.sam.db.tables._
 import org.broadinstitute.dsde.workbench.sam.model._
-import org.broadinstitute.dsde.workbench.sam.model.api.{AdminUpdateUserRequest, SamUser, SamUserAttributes}
+import org.broadinstitute.dsde.workbench.sam.model.api.{AdminUpdateUserRequest, GroupMembershipCount, SamUser, SamUserAttributes}
 import org.broadinstitute.dsde.workbench.sam.util.{DatabaseSupport, SamRequestContext}
 import org.postgresql.util.PSQLException
 import scalikejdbc._
@@ -736,8 +736,8 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val
       samUser: SamUser,
       limit: Int,
       samRequestContext: SamRequestContext
-  ): IO[Map[WorkbenchGroupIdentity, Int]] = if (limit <= 0) {
-    IO.pure(Map.empty)
+  ): IO[List[GroupMembershipCount]] = if (limit <= 0) {
+    IO.pure(List.empty)
   } else {
     readOnlyTransaction("listGroupsContributingToMostMemberships", samRequestContext) { implicit session =>
       val f = GroupMemberFlatTable.syntax("f")
@@ -762,15 +762,15 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val
                             left join ${PolicyTable as p} on ${p.groupId} = ${g.id}
                             left join ${ResourceTable as r} on ${p.resourceId} = ${r.id}
                             left join ${ResourceTypeTable as rt} on ${r.resourceTypeId} = ${rt.id}
+                            order by gc.membership_count desc
                             """
 
       query
         .map { rs =>
-          resultSetToGroupIdentity(rs, g, p, r, rt) -> rs.int("membership_count")
+          GroupMembershipCount(resultSetToGroupIdentity(rs, g, p, r, rt), rs.int("membership_count"))
         }
         .list()
         .apply()
-        .toMap
     }
   }
 
diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/UserService.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/UserService.scala
@@ -520,7 +520,7 @@ class UserService(
   def countIndirectPublicGroupMemberships(samUser: SamUser, samRequestContext: SamRequestContext): IO[Int] =
     directoryDAO.countIndirectPublicGroupMemberships(samUser, samRequestContext)
 
-  def listGroupsContributingToMostMemberships(samUser: SamUser, limit: Int, samRequestContext: SamRequestContext): IO[Map[WorkbenchGroupIdentity, Int]] =
+  def listGroupsContributingToMostMemberships(samUser: SamUser, limit: Int, samRequestContext: SamRequestContext): IO[List[GroupMembershipCount]] =
     directoryDAO.listGroupsContributingToMostMemberships(samUser, limit, samRequestContext)
 
   def getSamUserCombinedState(
@@ -573,7 +573,8 @@ class UserService(
       ),
       Map("enterpriseFeatures" -> enterpriseFeatures.toJson),
       favoriteResources,
-      topGroups.map { case (group, count) => GroupMembershipCount(group, count) }.toList match {
+      topGroups match {
+        // we want to omit the topGroups field if it's empty so that it does not seem like there are no groups
         case Nil => None
         case list => Option(list)
       }
diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDAO.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDAO.scala
@@ -15,7 +15,7 @@ import org.broadinstitute.dsde.workbench.sam.azure.{
   PetManagedIdentityId
 }
 import org.broadinstitute.dsde.workbench.sam.db.tables.TosTable
-import org.broadinstitute.dsde.workbench.sam.model.api.{AdminUpdateUserRequest, SamUser, SamUserAttributes}
+import org.broadinstitute.dsde.workbench.sam.model.api.{AdminUpdateUserRequest, GroupMembershipCount, SamUser, SamUserAttributes}
 import org.broadinstitute.dsde.workbench.sam.model.{AccessPolicy, BasicWorkbenchGroup, FullyQualifiedResourceId, ResourceAction, ResourceTypeName, SamUserTos}
 import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext
 
@@ -525,5 +525,5 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench
       samUser: SamUser,
       limit: Int,
       samRequestContext: SamRequestContext
-  ): IO[Map[WorkbenchGroupIdentity, Int]] = ???
+  ): IO[List[GroupMembershipCount]] = ???
 }
diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala
@@ -12,7 +12,7 @@ import org.broadinstitute.dsde.workbench.sam.db.TestDbReference
 import org.broadinstitute.dsde.workbench.sam.db.tables.TosTable
 import org.broadinstitute.dsde.workbench.sam.matchers.TimeMatchers
 import org.broadinstitute.dsde.workbench.sam.model._
-import org.broadinstitute.dsde.workbench.sam.model.api.{AdminUpdateUserRequest, SamUser, SamUserAttributes}
+import org.broadinstitute.dsde.workbench.sam.model.api.{AdminUpdateUserRequest, GroupMembershipCount, SamUser, SamUserAttributes}
 import org.broadinstitute.dsde.workbench.sam.{Generator, TestSupport}
 import org.scalatest.Inside.inside
 import org.scalatest.freespec.AnyFreeSpec
@@ -2259,11 +2259,13 @@ class PostgresDirectoryDAOSpec extends AnyFreeSpec with Matchers with BeforeAndA
         dao.addGroupMember(testGroups.last._1.id, otherUser.id, samRequestContext).unsafeRunSync()
 
         val result = dao.listGroupsContributingToMostMemberships(defaultUser, limit, samRequestContext).unsafeRunSync()
-        result should be(testGroups.drop(drop).map { case (group, index) => group.id -> (1 + index * multiplier) }.toMap)
+        result.reverse should contain theSameElementsInOrderAs testGroups.drop(drop).map { case (group, index) =>
+          GroupMembershipCount(group.id, 1 + index * multiplier)
+        }
 
         // check that another user gives different results
         val otherResult = dao.listGroupsContributingToMostMemberships(otherUser, limit, samRequestContext).unsafeRunSync()
-        otherResult should be(Map(testGroups.last._1.id -> (1 + (limit + drop) * multiplier)))
+        otherResult should be(List(GroupMembershipCount(testGroups.last._1.id, 1 + (limit + drop) * multiplier)))
       }
 
       "list policies and groups" in {
@@ -2282,7 +2284,7 @@ class PostgresDirectoryDAOSpec extends AnyFreeSpec with Matchers with BeforeAndA
         dao.updateSynchronizedDateAndVersion(group, samRequestContext).unsafeRunSync()
 
         val result = dao.listGroupsContributingToMostMemberships(defaultUser, 10, samRequestContext).unsafeRunSync()
-        result should be(Map(policy.id -> 1, group.id -> 1))
+        result should contain theSameElementsAs List(GroupMembershipCount(policy.id, 1), GroupMembershipCount(group.id, 1))
       }
 
       "ignores unsynchronized groups" in {
@@ -2297,10 +2299,10 @@ class PostgresDirectoryDAOSpec extends AnyFreeSpec with Matchers with BeforeAndA
         val parentUuid = UUID.randomUUID().toString
         val parentGroup = BasicWorkbenchGroup(WorkbenchGroupName(parentUuid), Set(leafGroup.id), WorkbenchEmail(parentUuid))
         dao.createGroup(parentGroup, samRequestContext = samRequestContext).unsafeRunSync()
-        // not synchronized
+        // updateSynchronizedDateAndVersion not called
 
         val result = dao.listGroupsContributingToMostMemberships(defaultUser, 10, samRequestContext).unsafeRunSync()
-        result should be(Map(leafGroup.id -> 1))
+        result should be(List(GroupMembershipCount(leafGroup.id, 1)))
       }
     }
   }
diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/UserServiceSpecs/SupportSummarySpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/UserServiceSpecs/SupportSummarySpec.scala
@@ -97,11 +97,11 @@ class SupportSummarySpec extends UserServiceTestTraits with TimeMatchers {
       .when(directoryDAO)
       .countIndirectPublicGroupMemberships(any[SamUser], any[SamRequestContext])
     lenient()
-      .doReturn(IO.pure(Map(testGroupName -> 1, testPolicyId -> 2)))
+      .doReturn(IO.pure(List(GroupMembershipCount(testGroupName, 1), GroupMembershipCount(testPolicyId, 2))))
       .when(directoryDAO)
       .listGroupsContributingToMostMemberships(any[SamUser], any[Int], any[SamRequestContext])
     lenient()
-      .doReturn(IO.pure(Map.empty))
+      .doReturn(IO.pure(List.empty))
       .when(directoryDAO)
       .listGroupsContributingToMostMemberships(any[SamUser], ArgumentMatchers.eq(0), any[SamRequestContext])
 

Original file line number	Diff line number	Diff line change
`@@ -11,7 +11,7 @@ import org.broadinstitute.dsde.workbench.sam.azure.{`
`11`	`11`	`PetManagedIdentity,`
`12`	`12`	`PetManagedIdentityId`
`13`	`13`	`}`
`14`		`-import org.broadinstitute.dsde.workbench.sam.model.api.{AdminUpdateUserRequest, SamUser, SamUserAttributes}`
	`14`	`+import org.broadinstitute.dsde.workbench.sam.model.api.{AdminUpdateUserRequest, GroupMembershipCount, SamUser, SamUserAttributes}`
`15`	`15`	`import org.broadinstitute.dsde.workbench.sam.model.{BasicWorkbenchGroup, FullyQualifiedResourceId, ResourceAction, ResourceTypeName, SamUserTos}`
`16`	`16`	`import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext`
`17`	`17`
`@@ -217,5 +217,5 @@ trait DirectoryDAO {`
`217`	`217`	`* @return`
`218`	`218`	`* a map of group to membership count`
`219`	`219`	`*/`
`220`		`- def listGroupsContributingToMostMemberships(samUser: SamUser, limit: Int, samRequestContext: SamRequestContext): IO[Map[WorkbenchGroupIdentity, Int]]`
	`220`	`+ def listGroupsContributingToMostMemberships(samUser: SamUser, limit: Int, samRequestContext: SamRequestContext): IO[List[GroupMembershipCount]]`
`221`	`221`	`}`
Original file line number	Diff line number	Diff line change
`@@ -15,7 +15,7 @@ import org.broadinstitute.dsde.workbench.sam.azure.{`
`15`	`15`	`PetManagedIdentityId`
`16`	`16`	`}`
`17`	`17`	`import org.broadinstitute.dsde.workbench.sam.db.tables.TosTable`
`18`		`-import org.broadinstitute.dsde.workbench.sam.model.api.{AdminUpdateUserRequest, SamUser, SamUserAttributes}`
	`18`	`+import org.broadinstitute.dsde.workbench.sam.model.api.{AdminUpdateUserRequest, GroupMembershipCount, SamUser, SamUserAttributes}`
`19`	`19`	`import org.broadinstitute.dsde.workbench.sam.model.{AccessPolicy, BasicWorkbenchGroup, FullyQualifiedResourceId, ResourceAction, ResourceTypeName, SamUserTos}`
`20`	`20`	`import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext`
`21`	`21`
`@@ -525,5 +525,5 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench`
`525`	`525`	`samUser: SamUser,`
`526`	`526`	`limit: Int,`
`527`	`527`	`samRequestContext: SamRequestContext`
`528`		`- ): IO[Map[WorkbenchGroupIdentity, Int]] = ???`
	`528`	`+ ): IO[List[GroupMembershipCount]] = ???`
`529`	`529`	`}`