From 12852be04d55f0ed0c3fc44b67571872562999d7 Mon Sep 17 00:00:00 2001 From: Tristan Garwood Date: Tue, 10 Oct 2023 13:30:33 -0400 Subject: [PATCH] responding to trevyn's comments. Taking specific version out of getUserTos. --- .../sam/dataAccess/DirectoryDAO.scala | 2 +- .../sam/dataAccess/PostgresDirectoryDAO.scala | 4 +-- .../workbench/sam/service/TosService.scala | 4 +-- .../sam/dataAccess/MockDirectoryDAO.scala | 4 +-- .../dataAccess/PostgresDirectoryDAOSpec.scala | 10 +++---- .../sam/service/TosServiceSpec.scala | 28 +++++++++---------- 6 files changed, 26 insertions(+), 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 index 6f8e1937a..35a8f5295 100644 --- 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 index da225004c..876b6dd74 100644 --- 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,7 +647,7 @@ 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 @@ -655,7 +655,7 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val 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 index 18aab1c42..946ebbd1f 100644 --- 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 index 36f7a6c0c..689df11cb 100644 --- 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/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala index 588e40c34..123b12003 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala +++ b/src/test/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 index d2485f9d3..313b8c065 100644 --- 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