feat(stark-ui): implementation of a session timeout warning #876

Mallikki · 2018-11-21T13:10:52Z

ISSUES CLOSED: #719

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/NationalBankBelgium/stark/blob/master/CONTRIBUTING.md#-commit-message-guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

Issue Number: #119 #719

What is the new behavior?

A new SessionTimeoutWarning dialog will be displayed when the user is inactive for too long, along with a timer.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

coveralls · 2018-11-21T13:28:33Z

Coverage increased (+0.04%) to 93.606% when pulling 7e82c3b on Mallikki:feature/session-timeout-warning into c414f9b on NationalBankBelgium:master.

coveralls · 2018-11-21T13:28:33Z

Coverage increased (+0.08%) to 93.646% when pulling 578b8a2 on Mallikki:feature/session-timeout-warning into 962e0fb on NationalBankBelgium:master.

Mallikki · 2018-11-21T14:37:53Z

Hey guys, I still need to look after the translation problem, but I've made the pull request anyway so you can already have a look at the change ;)

christophercr

Some changes... one of them might solve the issue with the translations :)

christophercr · 2018-11-21T16:22:15Z

docs/TIMEOUT_WARNING.md

@@ -0,0 +1,43 @@
+# Session Timeout Warning configuration
+
+If you wish your project to warn the user that its session is about to end, you can use our `SessionTimeoutWarningDialogComponent`.


Maybe we can re-phrase this like:

"Stark provides a nice feature when it comes to session expiration handling: in case the user's session is about to end due to inactivity (the user is idle for some time) and you want to warn him about this. This is what the SessionTimeoutWarningDialogComponent does and also asks the user whether to keep his session alive."

What do you think?

christophercr · 2018-11-21T16:25:22Z

docs/TIMEOUT_WARNING.md

+
+If you wish your project to warn the user that its session is about to end, you can use our `SessionTimeoutWarningDialogComponent`.
+
+To do so, you'll have to modifiy the `app.module.ts` file of your application.


"To use this feature, you'll have to modify the app.module.ts file of your application."

christophercr · 2018-11-21T16:25:27Z

docs/TIMEOUT_WARNING.md

+```
+import {
+	StarkSessionUiModule
+} from "@nationalbankbelgium/stark-ui";


Can you put this import in one single line to make it more readable?

christophercr · 2018-11-21T16:25:46Z

docs/TIMEOUT_WARNING.md

+	StarkSessionUiModule
+} from "@nationalbankbelgium/stark-ui";
+
+//the timeoutWarningDialogEffects won't be used


christophercr · 2018-11-21T16:26:53Z

docs/TIMEOUT_WARNING.md

+@NgModule({
+	imports: [
+		StarkSessionUiModule.forRoot({
+			enableTimeoutWarningDialogEffects: false


As we discussed, can you rename this option to "timeoutWarningDialogDisabled" so we keep consistency with the way the options in stark-app-config are named?

christophercr · 2018-11-21T17:08:13Z

packages/stark-ui/src/modules/session-ui/entities/stark-session-ui-config.ts

+);
+
+/**
+ * Definition of the configuration object for the Stark Session service


"Stark Session UI module"

christophercr · 2018-11-21T17:08:27Z

packages/stark-ui/src/modules/session-ui/entities/stark-session-ui-config.ts

+ */
+export class StarkSessionUiConfig {
+	/**
+	 * Router state for the Login page where the user can choose a profile and use it to impersonate himself as someone else.


christophercr · 2018-11-21T17:08:48Z

packages/stark-ui/src/modules/session-ui/entities/stark-session-ui-config.ts

+	 * Router state for the Login page where the user can choose a profile and use it to impersonate himself as someone else.
+	 * This state is only used in DEVELOPMENT.
+	 */
+	public enableTimeoutWarningDialogEffects?: boolean;


As we discussed, can you rename this option to "timeoutWarningDialogDisabled" so we keep consistency with the way the options in stark-app-config are named?

christophercr · 2018-11-21T17:09:30Z

packages/stark-ui/src/modules/session-ui/session-ui.module.ts

-		const english: StarkLocale = { languageCode: "en", translations: translationsEn };
-		const french: StarkLocale = { languageCode: "fr", translations: translationsFr };
-		const dutch: StarkLocale = { languageCode: "nl", translations: translationsNl };
-		mergeUiTranslations(translateService, english, french, dutch);


I think this is why the translations are not working anymore in the Session UI module :p Maybe a rebase problem?

christophercr · 2018-11-21T17:13:45Z

showcase/src/app/app.module.ts

@@ -235,7 +235,9 @@ export const metaReducers: MetaReducer<State>[] = ENV !== "production" ? [logger
 			position: "top right",
 			actionClasses: []
 		}),
-		StarkSessionUiModule.forRoot(),
+		StarkSessionUiModule.forRoot({
+			enableTimeoutWarningDialogEffects: true


You can revert this change when you rename this option to "timeoutWarningDialogDisabled" as I mentioned in my comments above ;)

christophercr

Some small changes to do.
Also be careful with the fit in the unit tests. You can easily notice this by checking the comments from Coveralls, in this case your PR decreased the coverage in almost 30%! So obviously this indicates there is an fit somewhere

christophercr · 2018-11-22T11:44:13Z

docs/TIMEOUT_WARNING.md

+@NgModule({
+	imports: [
+		StarkSessionUiModule.forRoot({
+			timeoutWarningDialogDisabled: true


This option should not be defined if they want the dialog to be shown. In fact this should be put in the code example below :p

christophercr · 2018-11-22T11:45:00Z

docs/TIMEOUT_WARNING.md

+@NgModule({
+	imports: [
+		StarkSessionUiModule.forRoot(),
+		...


The option should be passed here :p

christophercr · 2018-11-22T11:47:19Z

docs/TIMEOUT_WARNING.md

+})
+```
+
+To indicate that you don't wish to use this effect, just set `timeoutWarningDialogDisabled` value to true.


"To indicate that you don't wish to display such warning dialog, "

christophercr · 2018-11-22T11:47:27Z

docs/TIMEOUT_WARNING.md

+
+To indicate that you don't wish to use this effect, just set `timeoutWarningDialogDisabled` value to true.
+
+If you don't pass this value (or if you set it to false), it will be set to false by default:


This text is a bit repetitive... I suggest to add in the line before: "This option is "false" by default.

christophercr · 2018-11-22T11:49:15Z

packages/stark-core/src/modules/session/session.module.ts

@@ -20,7 +22,8 @@ import { StarkAppContainerComponent } from "./components";
 		StoreModule.forFeature("StarkSession", starkSessionReducers),
 		UIRouterModule.forChild({
 			states: SESSION_STATES
-		})
+		}),
+		EffectsModule.forFeature([StarkSettingsEffects])


This change is wrong... I think this file got messed up with a rebase or something. Just revert these changes.

christophercr · 2018-11-22T11:57:55Z

packages/stark-ui/src/modules/session-ui/effects/session-timeout-warning.effect.spec.ts

+
+			(<Spy>mockObserver.next).calls.reset();
+
+			// the effects should keep on running with any action other than the "stopping action"


Remove the other than the "stopping action" part ;)

christophercr · 2018-11-22T11:57:59Z

packages/stark-ui/src/modules/session-ui/effects/session-timeout-warning.effect.spec.ts

+
+			(<Spy>mockObserver.next).calls.reset();
+
+			// the effects should keep on running with any action other than the "stopping action"


Remove the other than the "stopping action" part ;)

christophercr · 2018-11-22T12:01:55Z

packages/stark-ui/src/modules/session-ui/session-ui.module.ts

 	imports: [
 		CommonModule,
 		UIRouterModule.forChild({
 			states: SESSION_UI_STATES
 		}),
+		StarkSessionModule,
+		StarkLoggingModule,


Please remove these two modules otherwise we are creating a hard link between the different modules of Stark.

Our goal is to have soft links, meaning that we just require the services (which the developer can provide by his own) but not the Stark modules themselves.

christophercr · 2018-11-22T12:05:24Z

packages/stark-ui/src/modules/session-ui/entities/stark-session-ui-config.ts

+ */
+export class StarkSessionUiConfig {
+	/**
+	 * If the warning dialog effect is disabled or not


Let's expand this description so that it is clear we use an effect for this. What about this?

"Whether the warning dialog should be displayed or not. If true, then the Ngrx Effects in charge of displaying the dialog won't be executed."

much better

christophercr · 2018-11-22T12:10:46Z

docs/TIMEOUT_WARNING.md

+Stark provides a nice feature when it comes to session expiration handling: in case the user's session is about to end due
+to inactivity (the user is idle for some time) and you want to warn him about this, a dialog will be displayed.
+
+This is what the `SessionTimeoutWarningDialogComponent` does and also asks the user if its session should be kept alive.


Let's add this in the same line to explain how we do this ;)

"This warning dialog is displayed by an NGRX Effect that triggers when a StarkSessionActionTypes.SESSION_TIMEOUT_COUNTDOWN_START action is dispatched (by the Session service when the user becomes idle)."

christophercr

I just noticed some other small changes to be done... sorry

christophercr · 2018-11-22T14:23:16Z

packages/stark-ui/src/modules/session-ui/assets/translations/en.ts

+			STAY_CONNECTED: "Stay connected",
+			TITLE: "Session timeout warning",
+			WILL_EXPIRE_IN: "Your session will expire in ",
+			WISH_STAY_CONNECTED: "Do you wish to stay connected ?"


There is a whitespace before the question mark ? :s

christophercr · 2018-11-22T14:23:20Z

packages/stark-ui/src/modules/session-ui/assets/translations/fr.ts

+			STAY_CONNECTED: "Rester connecté",
+			TITLE: "Avertissement d'expiration de session",
+			WILL_EXPIRE_IN: "Votre session va expirer dans ",
+			WISH_STAY_CONNECTED: "Voulez-vous rester connecté ?"


There is a whitespace before the question mark ? :s

In French, we have a whitespace before the question mark 😊

In fact, if I remember well my typing lessons (yep, I've had that, boring as hell), the convention is not to put a white space before, to treat it like a regular point. Word, OpenOffice, etc, always try to put one before, but it is not the official convention ^^ So I do think Christopher is right, I'll remove it, my bad :(

Ok, I have no idea about this so you can do what think it is best 😉

christophercr · 2018-11-22T14:23:23Z

packages/stark-ui/src/modules/session-ui/assets/translations/nl.ts

+			STAY_CONNECTED: "Verbonden blijven",
+			TITLE: "Sessietime-out waarschuwing",
+			WILL_EXPIRE_IN: "Uw sessie zal binnen ",
+			WISH_STAY_CONNECTED: "Wilt u verbonden blijven ?"


There is a whitespace before the question mark ? :s

christophercr · 2018-11-22T14:24:16Z

...ges/stark-ui/src/modules/session-ui/components/session-timeout-warning-dialog.component.html

@@ -0,0 +1,18 @@
+<div class="stark-timeout-warning-dialog" role="alertdialog" aria-busy="true" aria-live="assertive">
+	<header><i></i></header>


I don't think this line is needed, there is nothing related to those elements in the CSS

christophercr · 2018-11-22T14:25:00Z

...ges/stark-ui/src/modules/session-ui/components/session-timeout-warning-dialog.component.html

@@ -0,0 +1,18 @@
+<div class="stark-timeout-warning-dialog" role="alertdialog" aria-busy="true" aria-live="assertive">
+	<header><i></i></header>
+		<h2 translate>STARK.SESSION_TIMEOUT.TITLE</h2>


We should follow the API from Angular Material for Dialogs, basically we should add these directives except the MatDialogClose.

You can see how they are used in the example

Mallikki requested review from christophercr and SuperITMan November 21, 2018 14:36

christophercr suggested changes Nov 21, 2018

View reviewed changes

christophercr added this to the 10.0.0-beta.2 milestone Nov 21, 2018

christophercr added type: feature comp: stark-ui labels Nov 21, 2018

Mallikki force-pushed the feature/session-timeout-warning branch 2 times, most recently from 2f03dba to 7887ba6 Compare November 22, 2018 10:31

christophercr suggested changes Nov 22, 2018

View reviewed changes

Mallikki force-pushed the feature/session-timeout-warning branch from 7887ba6 to e5e6436 Compare November 22, 2018 12:52

christophercr suggested changes Nov 22, 2018

View reviewed changes

Mallikki force-pushed the feature/session-timeout-warning branch 3 times, most recently from 2619f3d to 103b7a3 Compare November 23, 2018 16:00

christophercr approved these changes Nov 23, 2018

View reviewed changes

feat(stark-ui): implementation of a session timeout warning

578b8a2

Mallikki force-pushed the feature/session-timeout-warning branch from 103b7a3 to 578b8a2 Compare November 23, 2018 16:29

christophercr merged commit cf7883a into NationalBankBelgium:master Nov 23, 2018

christophercr mentioned this pull request Nov 23, 2018

core: session #119

Closed

6 tasks

Mallikki deleted the feature/session-timeout-warning branch December 11, 2018 11:39

		@@ -0,0 +1,43 @@
		# Session Timeout Warning configuration

		If you wish your project to warn the user that its session is about to end, you can use our `SessionTimeoutWarningDialogComponent`.


		If you wish your project to warn the user that its session is about to end, you can use our `SessionTimeoutWarningDialogComponent`.

		To do so, you'll have to modifiy the `app.module.ts` file of your application.


		To indicate that you don't wish to use this effect, just set `timeoutWarningDialogDisabled` value to true.

		If you don't pass this value (or if you set it to false), it will be set to false by default:


		(<Spy>mockObserver.next).calls.reset();

		// the effects should keep on running with any action other than the "stopping action"

		@@ -0,0 +1,18 @@
		<div class="stark-timeout-warning-dialog" role="alertdialog" aria-busy="true" aria-live="assertive">
		<header><i></i></header>

feat(stark-ui): implementation of a session timeout warning #876

feat(stark-ui): implementation of a session timeout warning #876

Conversation

Mallikki commented Nov 21, 2018 • edited by christophercr Loading

PR Checklist

PR Type

What is the new behavior?

Does this PR introduce a breaking change?

Other information

coveralls commented Nov 21, 2018

coveralls commented Nov 21, 2018 • edited Loading

Mallikki commented Nov 21, 2018

christophercr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophercr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophercr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperITMan Nov 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mallikki commented Nov 21, 2018 •

edited by christophercr

Loading

coveralls commented Nov 21, 2018 •

edited

Loading

christophercr left a comment •

edited

Loading

SuperITMan Nov 22, 2018 •

edited

Loading