factor out NIH link expire-time calculation into a standalone, testab…

…le method; add tests; refactor for correctness and Scala-ness

src/main/scala/org/broadinstitute/dsde/firecloud/core/ProfileClient.scala

@@ -34,6 +34,21 @@ object ProfileClient {
   case class SyncSelf(userInfo: UserInfo)
 
   def props(requestContext: RequestContext): Props = Props(new ProfileClientActor(requestContext))
+
+  def calculateExpireTime(profile:Profile): Long = {
+    (profile.lastLinkTime, profile.linkExpireTime) match {
+      case (Some(lastLink), Some(expire)) if (lastLink < DateUtils.nowMinus24Hours && expire > DateUtils.nowPlus24Hours) =>
+        // The user has not logged in to FireCloud within 24 hours, AND the user's expiration is
+        // more than 24 hours in the future. Reset the user's expiration.
+        DateUtils.nowPlus24Hours
+      case (Some(lastLink), Some(expire)) =>
+        // User in good standing; return the expire time unchanged
+        expire
+      case _ =>
+        // Either last-link or expire is missing. Reset the user's expiration.
+        DateUtils.nowPlus24Hours
+    }
+  }
 }
 
 class ProfileClientActor(requestContext: RequestContext) extends Actor with FireCloudRequestBuilding {
@@ -119,24 +134,18 @@ class ProfileClientActor(requestContext: RequestContext) extends Actor with Fire
                 case Some(nihUsername) =>
                   // we have a linked profile.
 
-                  // if we have no record of the user logging in or the user's expire time, default to 0
-                  val lastLinkSeconds = profile.lastLinkTime.getOrElse(0L)
-                  var linkExpireSeconds = profile.linkExpireTime.getOrElse(0L)
-
-                  val howOld = DateUtils.hoursSince(lastLinkSeconds)
-                  val howSoonExpire = DateUtils.secondsSince(linkExpireSeconds)
+                  // get the current link expiration time
+                  val profileExpiration = profile.linkExpireTime.getOrElse(0L)
 
-                  // if the user's link has expired, the user must re-link.
-                  // NB: we use a separate val here in case we need to change the logic. For instance, we could
-                  // change the logic later to be "link has expired OR user hasn't logged in within 24 hours"
-                  val loginRequired = (howSoonExpire >= 0)
+                  // calculate the possible new link expiration time
+                  val linkExpireSeconds = if (updateExpiration) {
+                    calculateExpireTime(profile)
+                  } else {
+                    profileExpiration
+                  }
 
-                  // if the user has not logged in to FireCloud within 24 hours, AND the user's expiration is
-                  // further out than 24 hours, reset the user's expiration.
-                  if (updateExpiration && howOld >= 24 && Math.abs(howSoonExpire) >= (24*60*60)) {
-                    // generate time now+24 hours
-                    linkExpireSeconds = DateUtils.nowPlus24Hours
-                    // update linkExpireTime in Thurloe for this user
+                  // if the link expiration time needs updating (i.e. is different than what's in the profile), do the updating
+                  if (linkExpireSeconds != profileExpiration) {
                     val expireKVP = FireCloudKeyValue(Some("linkExpireTime"), Some(linkExpireSeconds.toString))
                     val expirePayload = ThurloeKeyValue(Some(userInfo.getUniqueId), Some(expireKVP))
 
@@ -158,6 +167,13 @@ class ProfileClientActor(requestContext: RequestContext) extends Actor with Fire
                     }
                   }
 
+                  val howSoonExpire = DateUtils.secondsSince(linkExpireSeconds)
+
+                  // if the user's link has expired, the user must re-link.
+                  // NB: we use a separate val here in case we need to change the logic. For instance, we could
+                  // change the logic later to be "link has expired OR user hasn't logged in within 24 hours"
+                  val loginRequired = (howSoonExpire >= 0)
+
                   getNIHStatusResponse(pipeline, loginRequired, profile, linkExpireSeconds)
 
                 case _ =>

src/main/scala/org/broadinstitute/dsde/firecloud/utils/DateUtils.scala

@@ -13,6 +13,10 @@ object DateUtils {
     nowDateTime.plusDays(30).getMillis / EPOCH
   }
 
+  def nowMinus30Days: Long = {
+    nowDateTime.minusDays(30).getMillis / EPOCH
+  }
+
   def nowPlus24Hours: Long = {
     nowDateTime.plusHours(24).getMillis / EPOCH
   }
@@ -21,6 +25,13 @@ object DateUtils {
     nowDateTime.minusHours(24).getMillis / EPOCH
   }
 
+  def nowPlus1Hour: Long = {
+    nowDateTime.plusHours(1).getMillis / EPOCH
+  }
+
+  def nowMinus1Hour: Long = {
+    nowDateTime.minusHours(1).getMillis / EPOCH
+  }
 
   def hoursSince(seconds: Long): Int = {
     Hours.hoursBetween(dtFromSeconds(seconds), nowDateTime).getHours

src/test/scala/org/broadinstitute/dsde/firecloud/service/ProfileClientSpec.scala

@@ -0,0 +1,170 @@
+package org.broadinstitute.dsde.firecloud.service
+
+import org.broadinstitute.dsde.firecloud.model.Profile
+import org.broadinstitute.dsde.firecloud.core.ProfileClient
+import org.broadinstitute.dsde.firecloud.utils.DateUtils
+import org.scalatest.FreeSpec
+
+class ProfileClientSpec extends FreeSpec {
+
+
+  def makeProfile(lastLinkTime: Option[Long] = None, linkExpireTime: Option[Long] = None): Profile = {
+    Profile (
+      "firstName",
+      "lastName",
+      "title",
+      "institute",
+      "institutionalProgram",
+      "programLocationCity",
+      "programLocationState",
+      "programLocationCountry",
+      "pi",
+      "nonProfitStatus",
+      Some("billingAccountName"),
+      Some("linkedNihUsername"),
+      lastLinkTime, //lastLinkTime
+      linkExpireTime, //linkExpireTime
+      Some(false) //isDbGapAuthorized, unused
+    )
+  }
+
+  def assertExpireTimeWasUpdated(calculatedExpire: Long) = {
+    // we expect the calculated time to be now + 24 hours. We can't test this exactly because milliseconds
+    // may have elapsed in processing, so we rely on DateUtils rounding, and give a
+    val diff = DateUtils.hoursUntil(calculatedExpire)
+
+    assert( diff == 23 || diff == 24,
+      "Expected expiration 24 hours in the future, found expiration " + diff + " hours away" )
+  }
+
+
+  "ExpireTimeCalculationTests" - {
+
+
+    // following tests: lastLink MORE than 24 hours in the future
+    "lastLink > 24 hours in the past and expire > 24 hours in the future" - {
+      "should return expire as now+24hours" in {
+        val lastLink = DateUtils.nowMinus30Days
+        val expire = DateUtils.nowPlus30Days
+
+        val profile = makeProfile(Some(lastLink), Some(expire))
+        val calculatedExpire = ProfileClient.calculateExpireTime(profile)
+
+        assertExpireTimeWasUpdated(calculatedExpire)
+      }
+    }
+
+    "lastLink > 24 hours in the past and expire < 24 hours in the future" - {
+      "should return expire time unchanged" in {
+        val lastLink = DateUtils.nowMinus30Days
+        val expire = DateUtils.nowPlus1Hour
+
+        val profile = makeProfile(Some(lastLink), Some(expire))
+        val calculatedExpire = ProfileClient.calculateExpireTime(profile)
+        assertResult(expire) { calculatedExpire }
+      }
+    }
+
+    "lastLink > 24 hours in the past and expire > 24 hours in the past" - {
+      "should return expire time unchanged" in {
+        val lastLink = DateUtils.nowMinus30Days
+        val expire = DateUtils.nowMinus30Days
+
+        val profile = makeProfile(Some(lastLink), Some(expire))
+        val calculatedExpire = ProfileClient.calculateExpireTime(profile)
+        assertResult(expire) { calculatedExpire }
+      }
+    }
+
+    "lastLink > 24 hours in the past and expire < 24 hours in the past" - {
+      "should return expire time unchanged" in {
+        val lastLink = DateUtils.nowMinus30Days
+        val expire = DateUtils.nowMinus1Hour
+
+        val profile = makeProfile(Some(lastLink), Some(expire))
+        val calculatedExpire = ProfileClient.calculateExpireTime(profile)
+        assertResult(expire) { calculatedExpire }
+      }
+    }
+
+
+    // following tests: lastLink LESS than 24 hours in the future
+    "lastLink < 24 hours in the past and expire > 24 hours in the future" - {
+      "should return expire time unchanged" in {
+        val lastLink = DateUtils.nowMinus1Hour
+        val expire = DateUtils.nowPlus30Days
+
+        val profile = makeProfile(Some(lastLink), Some(expire))
+        val calculatedExpire = ProfileClient.calculateExpireTime(profile)
+        assertResult(expire) { calculatedExpire }
+      }
+    }
+
+    "lastLink < 24 hours in the past and expire < 24 hours in the future" - {
+      "should return expire time unchanged" in {
+        val lastLink = DateUtils.nowMinus1Hour
+        val expire = DateUtils.nowPlus1Hour
+
+        val profile = makeProfile(Some(lastLink), Some(expire))
+        val calculatedExpire = ProfileClient.calculateExpireTime(profile)
+        assertResult(expire) { calculatedExpire }
+      }
+    }
+
+    "lastLink < 24 hours in the past and expire > 24 hours in the past" - {
+      "should return expire time unchanged" in {
+        val lastLink = DateUtils.nowMinus1Hour
+        val expire = DateUtils.nowMinus30Days
+
+        val profile = makeProfile(Some(lastLink), Some(expire))
+        val calculatedExpire = ProfileClient.calculateExpireTime(profile)
+        assertResult(expire) { calculatedExpire }
+      }
+    }
+
+    "lastLink < 24 hours in the past and expire < 24 hours in the past" - {
+      "should return expire time unchanged" in {
+        val lastLink = DateUtils.nowMinus1Hour
+        val expire = DateUtils.nowMinus1Hour
+
+        val profile = makeProfile(Some(lastLink), Some(expire))
+        val calculatedExpire = ProfileClient.calculateExpireTime(profile)
+        assertResult(expire) { calculatedExpire }
+      }
+    }
+
+    "No lastLink or expire time" - {
+      "should return expire as now+24hours" in {
+        val profile = makeProfile()
+        val calculatedExpire = ProfileClient.calculateExpireTime(profile)
+
+        assertExpireTimeWasUpdated(calculatedExpire)
+      }
+    }
+
+    "No lastLink time" - {
+      "should return expire as now+24hours" in {
+        val expire = DateUtils.nowPlus1Hour
+        val profile = makeProfile(None, Some(expire))
+        val calculatedExpire = ProfileClient.calculateExpireTime(profile)
+
+        assertExpireTimeWasUpdated(calculatedExpire)
+      }
+    }
+
+    "No expire time" - {
+      "should return expire as now+24hours" in {
+        val lastLink = DateUtils.nowMinus1Hour
+        val profile = makeProfile(Some(lastLink), None)
+        val calculatedExpire = ProfileClient.calculateExpireTime(profile)
+
+        assertExpireTimeWasUpdated(calculatedExpire)
+      }
+    }
+
+
+
+
+  }
+
+}