responding to trevyn's comments. Taking specific version out of getUs…

…erTos.
broadinstitute · Oct 10, 2023 · 12852be · 12852be
1 parent b916096
commit 12852be
Show file tree

Hide file tree

Showing 6 changed files with 26 additions and 26 deletions.
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
@@ -75,7 +75,7 @@ trait DirectoryDAO {
 
   def acceptTermsOfService(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Boolean]
   def rejectTermsOfService(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Boolean]
-  def getUserTos(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Option[SamUserTos]]
+  def getUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]]
 
   def createPetManagedIdentity(petManagedIdentity: PetManagedIdentity, samRequestContext: SamRequestContext): IO[PetManagedIdentity]
   def loadPetManagedIdentity(petManagedIdentityId: PetManagedIdentityId, samRequestContext: SamRequestContext): IO[Option[PetManagedIdentity]]

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
@@ -647,15 +647,15 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val
     }
   }
 
-  override def getUserTos(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] =
+  override def getUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] =
     readOnlyTransaction("getUserTos", samRequestContext) { implicit session =>
       val tosTable = TosTable.syntax
       val column = TosTable.column
 
       val loadUserTosQuery =
         samsql"""select ${tosTable.resultAll}
               from ${TosTable as tosTable}
-              where ${column.samUserId} = ${userId} and ${column.version} = ${tosVersion}
+              where ${column.samUserId} = ${userId}
               order by ${column.createdAt} desc
               limit 1"""
 

diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/TosService.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/TosService.scala
@@ -33,12 +33,12 @@ class TosService(val directoryDao: DirectoryDAO, val tosConfig: TermsOfServiceCo
 
   @Deprecated
   def getTosDetails(samUser: SamUser, samRequestContext: SamRequestContext): IO[TermsOfServiceDetails] =
-    directoryDao.getUserTos(samUser.id, tosConfig.version, samRequestContext).map { tos =>
+    directoryDao.getUserTos(samUser.id, samRequestContext).map { tos =>
       TermsOfServiceDetails(isEnabled = true, tosConfig.isGracePeriodEnabled, tosConfig.version, tos.map(_.version))
     }
 
   def getTosComplianceStatus(samUser: SamUser, samRequestContext: SamRequestContext): IO[TermsOfServiceComplianceStatus] =
-    directoryDao.getUserTos(samUser.id, tosConfig.version, samRequestContext).map { tos =>
+    directoryDao.getUserTos(samUser.id, samRequestContext).map { tos =>
       val userHasAcceptedLatestVersion = userHasAcceptedLatestTosVersion(tos)
       val permitsSystemUsage = tosAcceptancePermitsSystemUsage(samUser, tos)
       TermsOfServiceComplianceStatus(samUser.id, userHasAcceptedLatestVersion, permitsSystemUsage)

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
@@ -340,10 +340,10 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench
         true
     }
 
-  override def getUserTos(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] =
+  override def getUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] =
     loadUser(userId, samRequestContext).map {
       case None => None
-      case Some(user) =>
+      case Some(_) =>
         userTos.get(userId)
     }
 

diff --git a/...est/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala b/...est/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala
@@ -1486,7 +1486,7 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B
         dao.acceptTermsOfService(defaultUser.id, tosConfig.version, samRequestContext).unsafeRunSync() shouldBe true
 
         // Assert
-        val userTos = dao.getUserTos(defaultUser.id, tosConfig.version, samRequestContext).unsafeRunSync()
+        val userTos = dao.getUserTos(defaultUser.id, samRequestContext).unsafeRunSync()
         userTos should not be empty
         userTos.get.createdAt should beAround(Instant.now())
         userTos.get.action shouldBe TosTable.ACCEPT
@@ -1501,7 +1501,7 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B
         dao.acceptTermsOfService(defaultUser.id, "2", samRequestContext).unsafeRunSync() shouldBe true
 
         // Assert
-        val userTos = dao.getUserTos(defaultUser.id, "2", samRequestContext).unsafeRunSync()
+        val userTos = dao.getUserTos(defaultUser.id, samRequestContext).unsafeRunSync()
         userTos should not be empty
         userTos.get.createdAt should beAround(Instant.now())
         userTos.get.action shouldBe TosTable.ACCEPT
@@ -1516,7 +1516,7 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B
         dao.rejectTermsOfService(user.id, tosConfig.version, samRequestContext).unsafeRunSync() shouldBe true
 
         // Assert
-        val userTos = dao.getUserTos(user.id, tosConfig.version, samRequestContext).unsafeRunSync()
+        val userTos = dao.getUserTos(user.id, samRequestContext).unsafeRunSync()
         userTos should not be empty
         userTos.get.createdAt should beAround(Instant.now())
         userTos.get.action shouldBe TosTable.REJECT
@@ -1530,7 +1530,7 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B
         dao.rejectTermsOfService(user.id, tosConfig.version, samRequestContext).unsafeRunSync() shouldBe true
 
         // Assert
-        val userTos = dao.getUserTos(user.id, tosConfig.version, samRequestContext).unsafeRunSync()
+        val userTos = dao.getUserTos(user.id, samRequestContext).unsafeRunSync()
         userTos should not be empty
         userTos.get.createdAt should beAround(Instant.now())
         userTos.get.action shouldBe TosTable.REJECT
@@ -1544,7 +1544,7 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B
         dao.createUser(user, samRequestContext).unsafeRunSync()
 
         // Assert
-        val userTos = dao.getUserTos(user.id, tosConfig.version, samRequestContext).unsafeRunSync()
+        val userTos = dao.getUserTos(user.id, samRequestContext).unsafeRunSync()
         userTos should be(None)
       }
     }

diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/TosServiceSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/TosServiceSpec.scala
@@ -72,7 +72,7 @@ class TosServiceSpec extends AnyFreeSpec with TestSupport with BeforeAndAfterAll
       val tosVersion = "2"
       val tosService =
         new TosService(dirDAO, TestSupport.tosConfig.copy(version = tosVersion))
-      when(dirDAO.getUserTos(serviceAccountUser.id, tosVersion, samRequestContext))
+      when(dirDAO.getUserTos(serviceAccountUser.id, samRequestContext))
         .thenReturn(IO.pure(Some(SamUserTos(serviceAccountUser.id, tosVersion, TosTable.ACCEPT, Instant.now()))))
       val complianceStatus = tosService.getTosComplianceStatus(serviceAccountUser, samRequestContext).unsafeRunSync()
       complianceStatus.permitsSystemUsage shouldBe true
@@ -99,14 +99,14 @@ class TosServiceSpec extends AnyFreeSpec with TestSupport with BeforeAndAfterAll
 
     "when the user has not accepted any ToS version" - {
       "says the user has not accepted the latest version" in {
-        when(dirDAO.getUserTos(defaultUser.id, tosVersion, samRequestContext))
+        when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
           .thenReturn(IO.pure(None))
         val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
         complianceStatus.userHasAcceptedLatestTos shouldBe false
       }
       withoutGracePeriod - {
         cannotUseTheSystem in {
-          when(dirDAO.getUserTos(defaultUser.id, tosVersion, samRequestContext))
+          when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
             .thenReturn(IO.pure(None))
           // CASE 1
           val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
@@ -115,7 +115,7 @@ class TosServiceSpec extends AnyFreeSpec with TestSupport with BeforeAndAfterAll
       }
       withGracePeriod - {
         cannotUseTheSystem in {
-          when(dirDAO.getUserTos(defaultUser.id, tosVersion, samRequestContext))
+          when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
             .thenReturn(IO.pure(None))
           // CASE 4
           val complianceStatus = tosServiceV2GracePeriodEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
@@ -125,14 +125,14 @@ class TosServiceSpec extends AnyFreeSpec with TestSupport with BeforeAndAfterAll
     }
     "when the user has accepted a non-current ToS version" - {
       "says the user has not accepted the latest version" in {
-        when(dirDAO.getUserTos(defaultUser.id, tosVersion, samRequestContext))
+        when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
           .thenReturn(IO.pure(None))
         val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
         complianceStatus.userHasAcceptedLatestTos shouldBe false
       }
       withoutGracePeriod - {
         cannotUseTheSystem in {
-          when(dirDAO.getUserTos(defaultUser.id, tosVersion, samRequestContext))
+          when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
             .thenReturn(IO.pure(None))
           // CASE 2
           val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
@@ -141,7 +141,7 @@ class TosServiceSpec extends AnyFreeSpec with TestSupport with BeforeAndAfterAll
       }
       withGracePeriod - {
         canUseTheSystem in {
-          when(dirDAO.getUserTos(defaultUser.id, tosVersion, samRequestContext))
+          when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
             .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.ACCEPT, Instant.now()))))
           // CASE 5
           val complianceStatus = tosServiceV2GracePeriodEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
@@ -151,14 +151,14 @@ class TosServiceSpec extends AnyFreeSpec with TestSupport with BeforeAndAfterAll
     }
     "when the user has accepted the current ToS version" - {
       "says the user has accepted the latest version" in {
-        when(dirDAO.getUserTos(defaultUser.id, tosVersion, samRequestContext))
+        when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
           .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.ACCEPT, Instant.now()))))
         val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
         complianceStatus.userHasAcceptedLatestTos shouldBe true
       }
       withoutGracePeriod - {
         canUseTheSystem in {
-          when(dirDAO.getUserTos(defaultUser.id, tosVersion, samRequestContext))
+          when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
             .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.ACCEPT, Instant.now()))))
           // CASE 3
           val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
@@ -167,7 +167,7 @@ class TosServiceSpec extends AnyFreeSpec with TestSupport with BeforeAndAfterAll
       }
       withGracePeriod - {
         canUseTheSystem in {
-          when(dirDAO.getUserTos(defaultUser.id, tosVersion, samRequestContext))
+          when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
             .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.ACCEPT, Instant.now()))))
           // CASE 6
           val complianceStatus = tosServiceV2GracePeriodEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
@@ -178,14 +178,14 @@ class TosServiceSpec extends AnyFreeSpec with TestSupport with BeforeAndAfterAll
 
     "when the user has rejected the latest ToS version" - {
       "says the user has rejected the latest version" in {
-        when(dirDAO.getUserTos(defaultUser.id, tosVersion, samRequestContext))
+        when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
           .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.REJECT, Instant.now()))))
         val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
         complianceStatus.userHasAcceptedLatestTos shouldBe false
       }
       withoutGracePeriod - {
         cannotUseTheSystem in {
-          when(dirDAO.getUserTos(defaultUser.id, tosVersion, samRequestContext))
+          when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
             .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.REJECT, Instant.now()))))
           // CASE 1
           val complianceStatus = tosServiceV2.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
@@ -194,7 +194,7 @@ class TosServiceSpec extends AnyFreeSpec with TestSupport with BeforeAndAfterAll
       }
       withGracePeriod - {
         cannotUseTheSystem in {
-          when(dirDAO.getUserTos(defaultUser.id, tosVersion, samRequestContext))
+          when(dirDAO.getUserTos(defaultUser.id, samRequestContext))
             .thenReturn(IO.pure(Option(SamUserTos(defaultUser.id, tosVersion, TosTable.REJECT, Instant.now()))))
           // CASE 4
           val complianceStatus = tosServiceV2GracePeriodEnabled.getTosComplianceStatus(defaultUser, samRequestContext).unsafeRunSync()
@@ -204,7 +204,7 @@ class TosServiceSpec extends AnyFreeSpec with TestSupport with BeforeAndAfterAll
     }
     "when a service account is using the api" - {
       "let it use the api regardless of tos status" in {
-        when(dirDAO.getUserTos(serviceAccountUser.id, tosVersion, samRequestContext))
+        when(dirDAO.getUserTos(serviceAccountUser.id, samRequestContext))
           .thenReturn(IO.pure(None))
         val complianceStatus = tosServiceV2.getTosComplianceStatus(serviceAccountUser, samRequestContext).unsafeRunSync()
         complianceStatus.permitsSystemUsage shouldBe true