Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[STCOR-869] Add small margin to fixed timeout warning to match call to /logout #1517

Open
wants to merge 4 commits into
base: keycloak-ramsons
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/components/Root/FFetch.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ import {
RTR_AT_TTL_FRACTION,
RTR_ERROR_EVENT,
RTR_FLS_TIMEOUT_EVENT,
RTR_TIME_MARGIN_IN_MS,
RTR_TIME_MARGIN,
RTR_FLS_WARNING_EVENT,
RTR_RT_EXPIRY_IF_UNKNOWN,
} from './constants';
Expand Down Expand Up @@ -143,7 +143,7 @@ export class FFetch {
this.store.dispatch(setRtrFlsTimeout(setTimeout(() => {
this.logger.log('rtr-fls', 'emitting RTR_FLS_TIMEOUT_EVENT');
window.dispatchEvent(new Event(RTR_FLS_TIMEOUT_EVENT));
}, rtTimeoutInterval - RTR_TIME_MARGIN_IN_MS))); // Calling /logout a small margin before cookie is deleted to ensure it is included in the request
}, rtTimeoutInterval - ms(RTR_TIME_MARGIN)))); // Calling /logout a small margin before cookie is deleted to ensure it is included in the request
});
};

Expand Down
6 changes: 3 additions & 3 deletions src/components/Root/FFetch.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
RTR_AT_EXPIRY_IF_UNKNOWN,
RTR_AT_TTL_FRACTION,
RTR_FLS_WARNING_TTL,
RTR_TIME_MARGIN_IN_MS,
RTR_TIME_MARGIN,
} from './constants';

jest.mock('../../loginServices', () => ({
Expand Down Expand Up @@ -207,7 +207,7 @@ describe('FFetch class', () => {
expect(st).toHaveBeenCalledWith(expect.any(Function), (refreshTokenExpiration - whatTimeIsItMrFox) - ms(RTR_FLS_WARNING_TTL));

// FLS timeout
expect(st).toHaveBeenCalledWith(expect.any(Function), (refreshTokenExpiration - whatTimeIsItMrFox - RTR_TIME_MARGIN_IN_MS));
expect(st).toHaveBeenCalledWith(expect.any(Function), (refreshTokenExpiration - whatTimeIsItMrFox - ms(RTR_TIME_MARGIN)));
});

it('handles RTR data in the session', async () => {
Expand Down Expand Up @@ -380,7 +380,7 @@ describe('FFetch class', () => {
expect(st).toHaveBeenCalledWith(expect.any(Function), (refreshTokenExpiration - whatTimeIsItMrFox) - ms(RTR_FLS_WARNING_TTL));

// FLS timeout
expect(st).toHaveBeenCalledWith(expect.any(Function), (refreshTokenExpiration - whatTimeIsItMrFox - RTR_TIME_MARGIN_IN_MS));
expect(st).toHaveBeenCalledWith(expect.any(Function), (refreshTokenExpiration - whatTimeIsItMrFox - ms(RTR_TIME_MARGIN)));
});
});

Expand Down
2 changes: 1 addition & 1 deletion src/components/Root/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,4 +92,4 @@ export const RTR_RT_EXPIRY_IF_UNKNOWN = '10m';
* To account for minor delays between events (such as cookie expiration and API calls),
* 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';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

24 changes: 6 additions & 18 deletions src/components/SessionEventContainer/FixedLengthSessionWarning.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ import {
MessageBanner
} from '@folio/stripes-components';

import {
RTR_TIME_MARGIN
} from '../Root/constants';
import { timestampFormatter } from './utils';
import { useStripes } from '../../StripesContext';

/**
Expand All @@ -17,7 +21,7 @@ import { useStripes } from '../../StripesContext';
*/
const FixedLengthSessionWarning = () => {
const stripes = useStripes();
const [remainingMillis, setRemainingMillis] = useState(ms(stripes.config.rtr.fixedLengthSessionWarningTTL));
const [remainingMillis, setRemainingMillis] = useState(ms(stripes.config.rtr.fixedLengthSessionWarningTTL) - ms(RTR_TIME_MARGIN));

// configure an interval timer that sets state each second,
// counting down to 0.
Expand All @@ -32,23 +36,7 @@ const FixedLengthSessionWarning = () => {
};
}, []);

/**
* timestampFormatter
* convert time-remaining to mm:ss. Given the remaining time can easily be
* represented as elapsed-time since the JSDate epoch, convert to a
* Date object, format it, and extract the minutes and seconds.
* That is, given we have 99 seconds left, that converts to a Date
* like `1970-01-01T00:01:39.000Z`; extract the `01:39`.
*/
const timestampFormatter = () => {
if (remainingMillis >= 1000) {
return new Date(remainingMillis).toISOString().substring(14, 19);
}

return '00:00';
};

return <MessageBanner type="warning" show><FormattedMessage id="stripes-core.rtr.fixedLengthSession.timeRemaining" /> {timestampFormatter()}</MessageBanner>;
return <MessageBanner type="warning" show><FormattedMessage id="stripes-core.rtr.fixedLengthSession.timeRemaining" /> {timestampFormatter(remainingMillis)}</MessageBanner>;
};

export default FixedLengthSessionWarning;
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import { render, screen, waitFor } from '@folio/jest-config-stripes/testing-library/react';
import ms from 'ms';

import Harness from '../../../test/jest/helpers/harness';
import FixedLengthSessionWarning from './FixedLengthSessionWarning';
import { timestampFormatter } from './utils';
import { RTR_TIME_MARGIN } from '../Root/constants';

jest.mock('../Root/token-util');

Expand All @@ -17,7 +20,9 @@ describe('FixedLengthSessionWarning', () => {
it('renders a warning with seconds remaining', async () => {
render(<Harness stripes={stripes}><FixedLengthSessionWarning /></Harness>);
screen.getByText(/stripes-core.rtr.fixedLengthSession.timeRemaining/);
screen.getByText(/01:39/);
screen.getByText(
new RegExp(timestampFormatter(ms(stripes.config.rtr.fixedLengthSessionWarningTTL) - ms(RTR_TIME_MARGIN)))
);
});

it('renders 0:00 when time expires', async () => {
Expand All @@ -31,7 +36,9 @@ describe('FixedLengthSessionWarning', () => {

render(<Harness stripes={zeroSecondsStripes}><FixedLengthSessionWarning /></Harness>);
screen.getByText(/stripes-core.rtr.fixedLengthSession.timeRemaining/);
screen.getByText(/0:00/);
screen.getByText(
new RegExp(timestampFormatter(ms(stripes.config.rtr.fixedLengthSessionWarningTTL) - ms(RTR_TIME_MARGIN)))
);
Comment on lines -34 to +41
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});

// I've never had great luck with jest's fake timers, https://jestjs.io/docs/timer-mocks
Expand All @@ -53,7 +60,9 @@ describe('FixedLengthSessionWarning', () => {
expect(setInterval).toHaveBeenCalledTimes(1);
expect(setInterval).toHaveBeenLastCalledWith(expect.any(Function), 1000);

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 });
Comment on lines -56 to +65
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, I don't think we this test should need to change. Can you explain why this was necessary?

});
});
});
16 changes: 16 additions & 0 deletions src/components/SessionEventContainer/utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* timestampFormatter
* convert time-remaining to mm:ss. Given the remaining time can easily be
* represented as elapsed-time since the JSDate epoch, convert to a
* Date object, format it, and extract the minutes and seconds.
* That is, given we have 99 seconds left, that converts to a Date
* like `1970-01-01T00:01:39.000Z`; extract the `01:39`.
*/
// eslint-disable-next-line import/prefer-default-export
export const timestampFormatter = (remainingMillis) => {
if (remainingMillis >= 1000) {
return new Date(remainingMillis).toISOString().substring(14, 19);
}

return '00:00';
};
Loading