Skip to content

Commit b7bc3a3

Browse files
davidangbdavidangb
committed
factor out NIH link expire-time calculation into a standalone, testable method; add tests; refactor for correctness and Scala-ness
1 parent 2957049 commit b7bc3a3

File tree

3 files changed

+213
-16
lines changed

3 files changed

+213
-16
lines changed

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

Lines changed: 32 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,21 @@ object ProfileClient {
3434
case class SyncSelf(userInfo: UserInfo)
3535

3636
def props(requestContext: RequestContext): Props = Props(new ProfileClientActor(requestContext))
37+
38+
def calculateExpireTime(profile:Profile): Long = {
39+
(profile.lastLinkTime, profile.linkExpireTime) match {
40+
case (Some(lastLink), Some(expire)) if (lastLink < DateUtils.nowMinus24Hours && expire > DateUtils.nowPlus24Hours) =>
41+
// The user has not logged in to FireCloud within 24 hours, AND the user's expiration is
42+
// more than 24 hours in the future. Reset the user's expiration.
43+
DateUtils.nowPlus24Hours
44+
case (Some(lastLink), Some(expire)) =>
45+
// User in good standing; return the expire time unchanged
46+
expire
47+
case _ =>
48+
// Either last-link or expire is missing. Reset the user's expiration.
49+
DateUtils.nowPlus24Hours
50+
}
51+
}
3752
}
3853

3954
class ProfileClientActor(requestContext: RequestContext) extends Actor with FireCloudRequestBuilding {
@@ -119,24 +134,18 @@ class ProfileClientActor(requestContext: RequestContext) extends Actor with Fire
119134
case Some(nihUsername) =>
120135
// we have a linked profile.
121136

122-
// if we have no record of the user logging in or the user's expire time, default to 0
123-
val lastLinkSeconds = profile.lastLinkTime.getOrElse(0L)
124-
var linkExpireSeconds = profile.linkExpireTime.getOrElse(0L)
125-
126-
val howOld = DateUtils.hoursSince(lastLinkSeconds)
127-
val howSoonExpire = DateUtils.secondsSince(linkExpireSeconds)
137+
// get the current link expiration time
138+
val profileExpiration = profile.linkExpireTime.getOrElse(0L)
128139

129-
// if the user's link has expired, the user must re-link.
130-
// NB: we use a separate val here in case we need to change the logic. For instance, we could
131-
// change the logic later to be "link has expired OR user hasn't logged in within 24 hours"
132-
val loginRequired = (howSoonExpire >= 0)
140+
// calculate the possible new link expiration time
141+
val linkExpireSeconds = if (updateExpiration) {
142+
calculateExpireTime(profile)
143+
} else {
144+
profileExpiration
145+
}
133146

134-
// if the user has not logged in to FireCloud within 24 hours, AND the user's expiration is
135-
// further out than 24 hours, reset the user's expiration.
136-
if (updateExpiration && howOld >= 24 && Math.abs(howSoonExpire) >= (24*60*60)) {
137-
// generate time now+24 hours
138-
linkExpireSeconds = DateUtils.nowPlus24Hours
139-
// update linkExpireTime in Thurloe for this user
147+
// if the link expiration time needs updating (i.e. is different than what's in the profile), do the updating
148+
if (linkExpireSeconds != profileExpiration) {
140149
val expireKVP = FireCloudKeyValue(Some("linkExpireTime"), Some(linkExpireSeconds.toString))
141150
val expirePayload = ThurloeKeyValue(Some(userInfo.getUniqueId), Some(expireKVP))
142151

@@ -158,6 +167,13 @@ class ProfileClientActor(requestContext: RequestContext) extends Actor with Fire
158167
}
159168
}
160169

170+
val howSoonExpire = DateUtils.secondsSince(linkExpireSeconds)
171+
172+
// if the user's link has expired, the user must re-link.
173+
// NB: we use a separate val here in case we need to change the logic. For instance, we could
174+
// change the logic later to be "link has expired OR user hasn't logged in within 24 hours"
175+
val loginRequired = (howSoonExpire >= 0)
176+
161177
getNIHStatusResponse(pipeline, loginRequired, profile, linkExpireSeconds)
162178

163179
case _ =>

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,10 @@ object DateUtils {
1313
nowDateTime.plusDays(30).getMillis / EPOCH
1414
}
1515

16+
def nowMinus30Days: Long = {
17+
nowDateTime.minusDays(30).getMillis / EPOCH
18+
}
19+
1620
def nowPlus24Hours: Long = {
1721
nowDateTime.plusHours(24).getMillis / EPOCH
1822
}
@@ -21,6 +25,13 @@ object DateUtils {
2125
nowDateTime.minusHours(24).getMillis / EPOCH
2226
}
2327

28+
def nowPlus1Hour: Long = {
29+
nowDateTime.plusHours(1).getMillis / EPOCH
30+
}
31+
32+
def nowMinus1Hour: Long = {
33+
nowDateTime.minusHours(1).getMillis / EPOCH
34+
}
2435

2536
def hoursSince(seconds: Long): Int = {
2637
Hours.hoursBetween(dtFromSeconds(seconds), nowDateTime).getHours

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

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
package org.broadinstitute.dsde.firecloud.service
2+
3+
import org.broadinstitute.dsde.firecloud.model.Profile
4+
import org.broadinstitute.dsde.firecloud.core.ProfileClient
5+
import org.broadinstitute.dsde.firecloud.utils.DateUtils
6+
import org.scalatest.FreeSpec
7+
8+
class ProfileClientSpec extends FreeSpec {
9+
10+
11+
def makeProfile(lastLinkTime: Option[Long] = None, linkExpireTime: Option[Long] = None): Profile = {
12+
Profile (
13+
"firstName",
14+
"lastName",
15+
"title",
16+
"institute",
17+
"institutionalProgram",
18+
"programLocationCity",
19+
"programLocationState",
20+
"programLocationCountry",
21+
"pi",
22+
"nonProfitStatus",
23+
Some("billingAccountName"),
24+
Some("linkedNihUsername"),
25+
lastLinkTime, //lastLinkTime
26+
linkExpireTime, //linkExpireTime
27+
Some(false) //isDbGapAuthorized, unused
28+
)
29+
}
30+
31+
def assertExpireTimeWasUpdated(calculatedExpire: Long) = {
32+
// we expect the calculated time to be now + 24 hours. We can't test this exactly because milliseconds
33+
// may have elapsed in processing, so we rely on DateUtils rounding, and give a
34+
val diff = DateUtils.hoursUntil(calculatedExpire)
35+
36+
assert( diff == 23 || diff == 24,
37+
"Expected expiration 24 hours in the future, found expiration " + diff + " hours away" )
38+
}
39+
40+
41+
"ExpireTimeCalculationTests" - {
42+
43+
44+
// following tests: lastLink MORE than 24 hours in the future
45+
"lastLink > 24 hours in the past and expire > 24 hours in the future" - {
46+
"should return expire as now+24hours" in {
47+
val lastLink = DateUtils.nowMinus30Days
48+
val expire = DateUtils.nowPlus30Days
49+
50+
val profile = makeProfile(Some(lastLink), Some(expire))
51+
val calculatedExpire = ProfileClient.calculateExpireTime(profile)
52+
53+
assertExpireTimeWasUpdated(calculatedExpire)
54+
}
55+
}
56+
57+
"lastLink > 24 hours in the past and expire < 24 hours in the future" - {
58+
"should return expire time unchanged" in {
59+
val lastLink = DateUtils.nowMinus30Days
60+
val expire = DateUtils.nowPlus1Hour
61+
62+
val profile = makeProfile(Some(lastLink), Some(expire))
63+
val calculatedExpire = ProfileClient.calculateExpireTime(profile)
64+
assertResult(expire) { calculatedExpire }
65+
}
66+
}
67+
68+
"lastLink > 24 hours in the past and expire > 24 hours in the past" - {
69+
"should return expire time unchanged" in {
70+
val lastLink = DateUtils.nowMinus30Days
71+
val expire = DateUtils.nowMinus30Days
72+
73+
val profile = makeProfile(Some(lastLink), Some(expire))
74+
val calculatedExpire = ProfileClient.calculateExpireTime(profile)
75+
assertResult(expire) { calculatedExpire }
76+
}
77+
}
78+
79+
"lastLink > 24 hours in the past and expire < 24 hours in the past" - {
80+
"should return expire time unchanged" in {
81+
val lastLink = DateUtils.nowMinus30Days
82+
val expire = DateUtils.nowMinus1Hour
83+
84+
val profile = makeProfile(Some(lastLink), Some(expire))
85+
val calculatedExpire = ProfileClient.calculateExpireTime(profile)
86+
assertResult(expire) { calculatedExpire }
87+
}
88+
}
89+
90+
91+
// following tests: lastLink LESS than 24 hours in the future
92+
"lastLink < 24 hours in the past and expire > 24 hours in the future" - {
93+
"should return expire time unchanged" in {
94+
val lastLink = DateUtils.nowMinus1Hour
95+
val expire = DateUtils.nowPlus30Days
96+
97+
val profile = makeProfile(Some(lastLink), Some(expire))
98+
val calculatedExpire = ProfileClient.calculateExpireTime(profile)
99+
assertResult(expire) { calculatedExpire }
100+
}
101+
}
102+
103+
"lastLink < 24 hours in the past and expire < 24 hours in the future" - {
104+
"should return expire time unchanged" in {
105+
val lastLink = DateUtils.nowMinus1Hour
106+
val expire = DateUtils.nowPlus1Hour
107+
108+
val profile = makeProfile(Some(lastLink), Some(expire))
109+
val calculatedExpire = ProfileClient.calculateExpireTime(profile)
110+
assertResult(expire) { calculatedExpire }
111+
}
112+
}
113+
114+
"lastLink < 24 hours in the past and expire > 24 hours in the past" - {
115+
"should return expire time unchanged" in {
116+
val lastLink = DateUtils.nowMinus1Hour
117+
val expire = DateUtils.nowMinus30Days
118+
119+
val profile = makeProfile(Some(lastLink), Some(expire))
120+
val calculatedExpire = ProfileClient.calculateExpireTime(profile)
121+
assertResult(expire) { calculatedExpire }
122+
}
123+
}
124+
125+
"lastLink < 24 hours in the past and expire < 24 hours in the past" - {
126+
"should return expire time unchanged" in {
127+
val lastLink = DateUtils.nowMinus1Hour
128+
val expire = DateUtils.nowMinus1Hour
129+
130+
val profile = makeProfile(Some(lastLink), Some(expire))
131+
val calculatedExpire = ProfileClient.calculateExpireTime(profile)
132+
assertResult(expire) { calculatedExpire }
133+
}
134+
}
135+
136+
"No lastLink or expire time" - {
137+
"should return expire as now+24hours" in {
138+
val profile = makeProfile()
139+
val calculatedExpire = ProfileClient.calculateExpireTime(profile)
140+
141+
assertExpireTimeWasUpdated(calculatedExpire)
142+
}
143+
}
144+
145+
"No lastLink time" - {
146+
"should return expire as now+24hours" in {
147+
val expire = DateUtils.nowPlus1Hour
148+
val profile = makeProfile(None, Some(expire))
149+
val calculatedExpire = ProfileClient.calculateExpireTime(profile)
150+
151+
assertExpireTimeWasUpdated(calculatedExpire)
152+
}
153+
}
154+
155+
"No expire time" - {
156+
"should return expire as now+24hours" in {
157+
val lastLink = DateUtils.nowMinus1Hour
158+
val profile = makeProfile(Some(lastLink), None)
159+
val calculatedExpire = ProfileClient.calculateExpireTime(profile)
160+
161+
assertExpireTimeWasUpdated(calculatedExpire)
162+
}
163+
}
164+
165+
166+
167+
168+
}
169+
170+
}

0 commit comments

Comments
 (0)