last touches

broadinstitute · Nov 27, 2023 · 3dc54dd · 3dc54dd
1 parent cdbdfa9
commit 3dc54dd
Show file tree

Hide file tree

Showing 4 changed files with 18 additions and 26 deletions.
diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala
@@ -99,17 +99,17 @@ trait TermsOfServiceRoutes extends SamUserDirectives {
                 }
               }
             } ~
-              pathPrefix("history") { // api/termsOfService/v1/user/{userId}/history
-                pathEndOrSingleSlash {
-                  get {
-                    parameters("limit".as[Integer].withDefault(100)) { (limit: Int) =>
-                      complete {
-                        tosService.getTermsOfServiceHistoryForUser(samUser.id, samRequestContext, limit)
-                      }
+            pathPrefix("history") { // api/termsOfService/v1/user/{userId}/history
+              pathEndOrSingleSlash {
+                get {
+                  parameters("limit".as[Integer].withDefault(100)) { (limit: Int) =>
+                    complete {
+                      tosService.getTermsOfServiceHistoryForUser(samUser.id, samRequestContext, limit)
                     }
                   }
                 }
-              } ~
+              }
+            } ~
             // The {user_id} route must be last otherwise it will try to parse the other routes incorrectly as user id's
             pathPrefix(Segment) { userId => // api/termsOfService/v1/user/{userId}
               validate(samUserIdPattern.matches(userId), "User ID must be alpha numeric") {
@@ -126,7 +126,8 @@ trait TermsOfServiceRoutes extends SamUserDirectives {
                     get {
                       parameters("limit".as[Integer].withDefault(100)) { (limit: Int) =>
                         complete(tosService.getTermsOfServiceHistoryForUser(requestUserId, samRequestContext, limit))
-                      }                    }
+                      }
+                    }
                   }
                 }
               }

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
@@ -649,10 +649,10 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val
   }
 
   // When no tosVersion is specified, return the latest TosRecord for the user
-  override def getUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] =
-    getUserTosVersion(userId, None, samRequestContext)
+  override def getUserTermsOfService(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] =
+    getUserTermsOfServiceVersion(userId, None, samRequestContext)
 
-  override def getUserTosVersion(userId: WorkbenchUserId, tosVersion: Option[String], samRequestContext: SamRequestContext): IO[Option[SamUserTos]] =
+  override def getUserTermsOfServiceVersion(userId: WorkbenchUserId, tosVersion: Option[String], samRequestContext: SamRequestContext): IO[Option[SamUserTos]] =
     readOnlyTransaction("getUserTos", samRequestContext) { implicit session =>
       val tosTable = TosTable.syntax
       val column = TosTable.column
@@ -669,7 +669,6 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val
       val userTosRecordOpt: Option[TosRecord] = loadUserTosQuery.map(TosTable(tosTable)).first().apply()
       userTosRecordOpt.map(TosTable.unmarshalUserRecord)
     }
-  }
 
   override def getUserTermsOfServiceHistory(userId: WorkbenchUserId, samRequestContext: SamRequestContext, limit: Integer): IO[List[SamUserTos]] =
     readOnlyTransaction("getUserTosHistory", samRequestContext) { implicit session =>

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
@@ -107,7 +107,7 @@ class TosService(
     }
 
   private def ensureLatestTermsOfService(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[SamUserTos] = for {
-    maybeTermsOfServiceRecord <- directoryDao.getUserTos(userId, samRequestContext)
+    maybeTermsOfServiceRecord <- directoryDao.getUserTermsOfService(userId, samRequestContext)
     latestUserTermsOfService <- maybeTermsOfServiceRecord
       .map(IO.pure)
       .getOrElse(
@@ -123,13 +123,6 @@ class TosService(
       case None => throw new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, s"Could not find user:${userId}"))
     }
 
-  // Note: if version is None, then the query will return the last accepted ToS info for the user
-  private def loadTosRecordForUser(userId: WorkbenchUserId, version: Option[String], samRequestContext: SamRequestContext): IO[SamUserTos] =
-    directoryDao.getUserTermsOfServiceVersion(userId, version, samRequestContext).map {
-      case Some(samUserTos) => samUserTos
-      case None => throw new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, s"Could not find Terms of Service entry for user:${userId}"))
-    }
-
   def getTermsOfServiceHistoryForUser(userId: WorkbenchUserId, samRequestContext: SamRequestContext, limit: Integer): IO[TermsOfServiceHistory] =
     ensureAdminIfNeeded[TermsOfServiceHistory](userId, samRequestContext) {
       directoryDao.getUserTermsOfServiceHistory(userId, samRequestContext, limit).map {
@@ -144,9 +137,8 @@ class TosService(
 
   def getTermsOfServiceComplianceStatus(samUser: SamUser, samRequestContext: SamRequestContext): IO[TermsOfServiceComplianceStatus] = for {
     latestUserTos <- directoryDao.getUserTermsOfService(samUser.id, samRequestContext)
-    previousUserTos <- directoryDao.getUserTermsOfServiceVersion(samUser.id, tosConfig.previousVersion, samRequestContext)
-    userHasAcceptedLatestVersion = userHasAcceptedLatestTermsOfServiceVersion(latestUserTos)
-    permitsSystemUsage = tosAcceptancePermitsSystemUsage(samUser, latestUserTos, previousUserTos)
+    userHasAcceptedLatestVersion = userHasAcceptedCurrentTermsOfService(latestUserTos)
+    permitsSystemUsage = tosAcceptancePermitsSystemUsage(samUser, latestUserTos)
   } yield TermsOfServiceComplianceStatus(samUser.id, userHasAcceptedLatestVersion, permitsSystemUsage)
 
   /** If grace period enabled, don't check ToS, return true If ToS disabled, return true Otherwise return true if user has accepted ToS, or is a service account

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
@@ -155,7 +155,7 @@ class TosServiceSpec(_system: ActorSystem)
 
         when(dirDAO.getUserTosVersion(serviceAccountUser.id, previousTosVersion, samRequestContext)).thenReturn(IO.pure(None))
 
-        val complianceStatus = tosService.getTosComplianceStatus(serviceAccountUser, samRequestContext).unsafeRunSync()
+        val complianceStatus = tosService.getTermsOfServiceComplianceStatus(serviceAccountUser, samRequestContext).unsafeRunSync()
         complianceStatus.permitsSystemUsage shouldBe true
       }
 
@@ -168,7 +168,7 @@ class TosServiceSpec(_system: ActorSystem)
 
         when(dirDAO.getUserTosVersion(uamiUser.id, previousTosVersion, samRequestContext)).thenReturn(IO.pure(None))
 
-        val complianceStatus = tosService.getTosComplianceStatus(uamiUser, samRequestContext).unsafeRunSync()
+        val complianceStatus = tosService.getTermsOfServiceComplianceStatus(uamiUser, samRequestContext).unsafeRunSync()
         complianceStatus.permitsSystemUsage shouldBe true
       }
     }