[STCOR-869] Add small margin to fixed timeout warning to match call to /logout#1517
[STCOR-869] Add small margin to fixed timeout warning to match call to /logout#1517ryandberger wants to merge 4 commits intokeycloak-ramsonsfrom
Conversation
|
zburke
left a comment
zburke
left a comment
There was a problem hiding this comment.
@ryandberger, This works, but it splits the "when does my session end?" math across two different places, FFetch and FixedLengthSessionWarning. Could we consolidate all of into FFetch around lines 118-119? That would allow us to leave FixedLengthSessionWarning alone, and it would simplify the setTimeout() calls a little since they'd each just rely on their single rt...Interval values.
What do you think?
| * this is a small amount of time to wait so the proper order can be ensured if they happen simultaneously. | ||
| */ | ||
| export const RTR_TIME_MARGIN_IN_MS = 200; | ||
| export const RTR_TIME_MARGIN = '200m'; |
There was a problem hiding this comment.
TLDR, use '200' instead of '200m'.
ms() parses strings -> ints and parses ints -> strings. Normally, the string contains a unit (s, m, h, d... for seconds, minutes, hours, days...) but if you want to provide an input in milliseconds then you omit the unit 🤷 just don't tell your high school physics teacher.
Since all our other time-values here get pushed through ms(), I think it's good practice to follow suit here, even though it feels kinda dumb to convert from milliseconds to milliseconds. I should've included the same comment from line 71, "value must be a string parsable by ms()", just above on in the description for the RTR_*_EXPIRY_IF_UNKNOWN values at line 87 🤦, which would have helped make this more obvious.
| screen.getByText(/0:00/); | ||
| screen.getByText( | ||
| new RegExp(timestampFormatter(ms(stripes.config.rtr.fixedLengthSessionWarningTTL) - ms(RTR_TIME_MARGIN))) | ||
| ); |
There was a problem hiding this comment.
I don't understand why this test changed because from the user's point of view, nothing will be any different on screen: you will still see a countdown and it will still count down to 0:00 and then terminate your session. The only difference is that if your session was previously 100 seconds long, now it will be 99.8 seconds long. So, we still want to see 0:00 on the clock, we just expect to see it 200ms earlier than we did previously.
| await waitFor(() => screen.getByText(/00:09/), { timeout: 2000 }); | ||
| await waitFor(() => screen.getByText( | ||
| new RegExp(timestampFormatter(ms(stripes.config.rtr.fixedLengthSessionWarningTTL) - ms(RTR_TIME_MARGIN))) | ||
| ), { timeout: 2000 }); |
There was a problem hiding this comment.
As above, I don't think we this test should need to change. Can you explain why this was necessary?
2 participants
RTR_TIME_MARGINto be string to match other constants.timestampFormatter()to utils file so that it can be accessed in unit tests.