ENH Display toast notifications #39

emteknetnz · 2021-03-15T01:54:43Z

Issue #13

maxime-rainville

Just going back to what I said on the original PR. The Login component is mixing concerns. It should be split in two:

one part that handles the UI (That's what should be in the pattern library)
one part that handles the communication with the backend, the displaying of toasts, etc

The pattern library is still broken for me. You need to update that to use disconnected Component as well.

client/src/components/LoginSession/LoginSession.js

client/src/components/LoginSession/tests/LoginSession-test.js

client/src/components/LoginSession/LoginSession.js

emteknetnz · 2021-03-30T03:23:25Z

@maxime-rainville I've split the components and responded to all the other feedback, good for peer review again

maxime-rainville

Some things that ought to be in the LoginSessionCantianer is still in the LoginSession component.

I haven't reviewed the fine print of the JS tests because the final shape of the JSX components is still fluid.

client/src/components/LoginSession/LoginSessionContainer.js

client/src/components/LoginSession/LoginSession.js

client/src/components/LoginSession/LoginSessionContainer.js

client/src/components/LoginSession/tests/LoginSession-story.js

maxime-rainville · 2021-03-30T23:10:47Z

client/src/components/LoginSession/tests/LoginSession-story.js

@@ -10,6 +10,14 @@ const props = {
    Created: '2021-01-20 00:33:41',
    LastAccessed: '2021-03-11 03:47:22',
    LogOutEndpoint: 'admin/loginsession/remove',
+    logout: (beforeRequest, receivedResponse, caughtException) => {


A better way of passing mock event handler to your pattern libraries stories is to use @storybook/addon-actions.

import { action } from '@storybook/addon-actions'; const logout = action('logout'); logout.toString = () => 'logout';

That will log an entry in the storybook action logger.

It's not a mock event handler. The logout() function needs to call beforeRequest() and receivedResponse() in order for the LoginSession state and thus text to update

No longer required, can use logout: () => 1 just to satisfy the func.required propType

src/Middleware/LoginSessionController.php

client/src/components/LoginSession/LoginSession.js

client/src/components/LoginSession/LoginSessionContainer.js

maxime-rainville

I took some extra time to refactor <LoginSessionContainer /> to be a functional and update its matching tests. What you did wasn't horrible or anything, but since you struggled with this part, I thought it would be a good learning experience to illustrate how to write test for a functional component following best practices.

I left lots of detailed comments on the commit to explain why I did it that way.

Have a read and let me know if you have any further questions.

Otherwise, the rest of the PR is great. I left some suggestion here and there but nothing that absolutely needs to be addressed.

maxime-rainville · 2021-04-06T21:44:22Z

client/src/components/LoginSession/LoginSession.js

        if (!await confirm(confirmMessage, { title: confirmTitle, confirmLabel: buttonLabel })) {
            return;
        }
-
-        logOut();
-    }
-
-    if (loading.fadeOutComplete) {
-        return null;
+        props.logout();


To my eye this looks better, but the current code is fine.

if (await confirm(confirmMessage, { title: confirmTitle, confirmLabel: buttonLabel })) { props.logout(); }

logout is not define in your propTypes.

maxime-rainville · 2021-04-06T21:46:28Z

client/src/components/LoginSession/LoginSession.js

+          {!props.IsCurrent && <Button
+            color="link"
+            className="login-session__logout"
+            onClick={() => attemptLogOut()}


That's a bit more succinct.

Suggested change

onClick={() => attemptLogOut()}

onClick={attemptLogOut}

maxime-rainville · 2021-04-06T22:49:11Z

client/src/components/SessionManagerField/SessionManagerField.js

-          </li>)
-            )}
+      <ul className="session-manager-field list-unstyled">
+        {props.loginSessions.map((loginSession) => (


Suggested change

{props.loginSessions.map((loginSession) => (

{props.loginSessions.map(loginSession => (

maxime-rainville · 2021-04-06T22:50:04Z

client/src/components/SessionManagerField/SessionManagerField.js

@@ -1,22 +1,22 @@
 /* global window */
 import React from 'react';
-import LoginSession from '../LoginSession/LoginSession';
+import LoginSessionContainer from '../LoginSession/LoginSessionContainer';


Suggested change

import LoginSessionContainer from '../LoginSession/LoginSessionContainer';

import LoginSessionContainer from 'components/LoginSession/LoginSessionContainer';

maxime-rainville · 2021-04-06T22:59:47Z

client/src/components/LoginSession/LoginSession.js

@@ -111,39 +52,37 @@ function LoginSession(props) {
    );

    return (
-      <div className={`login-session ${(loading.complete && !loading.failed) ? 'hidden' : ''}`}>
+      <div className={`login-session ${(props.complete && !props.failed) ? 'hidden' : ''}`}>


It would be clearer if you use the classnames lib for this kind of string building.

maxime-rainville · 2021-04-07T04:12:44Z

client/src/components/LoginSession/tests/LoginSessionContainer-test.js

+        httpResolve({
+            error: false,
+            success: global.success,
+            message: 'amazing success'
+        });
+        await logoutRequest;


We fake our HTTP response.

maxime-rainville · 2021-04-07T04:13:49Z

client/src/components/LoginSession/tests/LoginSessionContainer-test.js

+        expect(loginSession.props()).toMatchObject({
+            ...sessionData,
+            complete: true,
+            failed: false,
+            submitting: false
+        });


We test the end state of our component, once the session has been invalidated.

maxime-rainville · 2021-04-07T04:15:12Z

client/src/components/LoginSession/tests/LoginSessionContainer-test.js

+        expect(props.displayToastSuccess).toBeCalledWith('amazing success');
+        expect(props.displayToastFailure).not.toBeCalled();


These mock calls are a bit smarter. We're not only testing the that our mocks have been called, but that they've been called with the expected success message.

maxime-rainville · 2021-04-07T04:17:40Z

client/src/components/LoginSession/tests/LoginSessionContainer-test.js

+        expect(props.displayToastSuccess).not.toBeCalled();
+    });
+
+    it('Handles HTTP Request failure', async () => {


Our initial test was only testing a regular failure. We were not testing a scenario where we had an actual HTTP failure.

maxime-rainville · 2021-04-07T04:18:36Z

client/src/components/LoginSession/tests/LoginSessionContainer-test.js

+            submitting: true
+        });
+
+        httpReject();


Calling the Promise's reject method will cause our catch block to run.

maxime-rainville · 2021-04-07T21:51:12Z

@emteknetnz Reverted my commits and split them off into a separate PR creative-commoners#4

emteknetnz mentioned this pull request Mar 15, 2021

Allow removal of non-current Sessions #13

Closed

8 tasks

emteknetnz force-pushed the pulls/ui-rendering/backend-updates branch 2 times, most recently from d539204 to 62479be Compare March 15, 2021 03:37

emteknetnz changed the title ~~ENH Remove coupling to RememberLoginHash.logout_across_devices~~ ENH Display toast notificaitons, remove coupling to RememberLoginHash.logout_across_devices Mar 15, 2021

emteknetnz changed the title ~~ENH Display toast notificaitons, remove coupling to RememberLoginHash.logout_across_devices~~ ENH Display toast notifications, remove coupling to RememberLoginHash.logout_across_devices Mar 15, 2021

emteknetnz force-pushed the pulls/ui-rendering/backend-updates branch 2 times, most recently from b660a0e to 92ee1fe Compare March 16, 2021 00:13

emteknetnz changed the title ~~ENH Display toast notifications, remove coupling to RememberLoginHash.logout_across_devices~~ ENH Display toast notifications Mar 16, 2021

bergice force-pushed the ui-rendering branch from 5e4d4ea to 802a37f Compare March 17, 2021 01:48

emteknetnz changed the base branch from ui-rendering to 1 March 23, 2021 04:33

emteknetnz force-pushed the pulls/ui-rendering/backend-updates branch 11 times, most recently from 112bfe2 to e7d869b Compare March 25, 2021 04:12

emteknetnz marked this pull request as ready for review March 25, 2021 04:13

maxime-rainville requested changes Mar 28, 2021

View reviewed changes

client/src/components/LoginSession/LoginSession.js Outdated Show resolved Hide resolved

client/src/components/LoginSession/LoginSession.js Outdated Show resolved Hide resolved

emteknetnz force-pushed the pulls/ui-rendering/backend-updates branch 6 times, most recently from 1059772 to a4ac5e9 Compare March 29, 2021 02:15

emteknetnz commented Mar 29, 2021

View reviewed changes

client/src/components/LoginSession/tests/LoginSession-test.js Outdated Show resolved Hide resolved

emteknetnz commented Mar 29, 2021

View reviewed changes

client/src/components/LoginSession/LoginSession.js Outdated Show resolved Hide resolved

emteknetnz mentioned this pull request Mar 29, 2021

Refactor http response codes + unit tests to LoginSessionController #45

Closed

3 tasks

emteknetnz force-pushed the pulls/ui-rendering/backend-updates branch 7 times, most recently from 3af340e to fa6edd3 Compare March 30, 2021 03:16

maxime-rainville requested changes Mar 30, 2021

View reviewed changes

emteknetnz force-pushed the pulls/ui-rendering/backend-updates branch from fa6edd3 to 5183610 Compare March 31, 2021 03:19

emteknetnz mentioned this pull request Apr 1, 2021

FIX Adjust time formats creative-commoners/silverstripe-session-manager#2

Closed

maxime-rainville requested changes Apr 1, 2021

View reviewed changes

client/src/components/LoginSession/LoginSession.js Outdated Show resolved Hide resolved

client/src/components/LoginSession/LoginSession.js Show resolved Hide resolved

client/src/components/LoginSession/LoginSessionContainer.js Show resolved Hide resolved

emteknetnz force-pushed the pulls/ui-rendering/backend-updates branch 2 times, most recently from 592f4ec to a621235 Compare April 5, 2021 23:18

emteknetnz mentioned this pull request Apr 5, 2021

Pulls/UI rendering/backend update container hook #47

Closed

emteknetnz force-pushed the pulls/ui-rendering/backend-updates branch from a621235 to a3ae336 Compare April 5, 2021 23:48

ENH Display toast notifications

b749681

emteknetnz force-pushed the pulls/ui-rendering/backend-updates branch from a3ae336 to b749681 Compare April 5, 2021 23:53

maxime-rainville approved these changes Apr 7, 2021

View reviewed changes

maxime-rainville force-pushed the pulls/ui-rendering/backend-updates branch from 1306663 to b749681 Compare April 7, 2021 21:46

This was referenced Apr 7, 2021

Refactor LoginSessionContainer to functional component creative-commoners/silverstripe-session-manager#3

Closed

Refactor LoginSessionContainer to be a functional component. creative-commoners/silverstripe-session-manager#4

Closed

maxime-rainville merged commit 71c16a4 into silverstripe:1 Apr 8, 2021

maxime-rainville mentioned this pull request Apr 8, 2021

Refactor LoginSessionContainer to be a functional component #48

Merged

emteknetnz deleted the pulls/ui-rendering/backend-updates branch April 8, 2021 04:24

This was referenced Jun 12, 2023

ENH: Use the PsrLogMessageProcessor so that custom factory created loggers can use context. silverstripe/silverstripe-auditor#53

Draft

AuditHookSessionManagerTest can never run - checks against non-existent class name silverstripe/silverstripe-auditor#55

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH Display toast notifications #39

ENH Display toast notifications #39

emteknetnz commented Mar 15, 2021 •

edited

Loading

maxime-rainville left a comment

emteknetnz commented Mar 30, 2021

maxime-rainville left a comment

maxime-rainville Mar 30, 2021

emteknetnz Mar 31, 2021

emteknetnz Apr 5, 2021

maxime-rainville left a comment •

edited

Loading

maxime-rainville Apr 6, 2021

maxime-rainville Apr 6, 2021

maxime-rainville Apr 6, 2021

maxime-rainville Apr 6, 2021

maxime-rainville Apr 6, 2021

maxime-rainville Apr 6, 2021

maxime-rainville Apr 7, 2021

maxime-rainville Apr 7, 2021

maxime-rainville Apr 7, 2021

maxime-rainville Apr 7, 2021

maxime-rainville Apr 7, 2021

maxime-rainville commented Apr 7, 2021

	{props.loginSessions.map((loginSession) => (
	{props.loginSessions.map(loginSession => (

	import LoginSessionContainer from '../LoginSession/LoginSessionContainer';
	import LoginSessionContainer from 'components/LoginSession/LoginSessionContainer';

		expect(props.displayToastSuccess).toBeCalledWith('amazing success');
		expect(props.displayToastFailure).not.toBeCalled();

ENH Display toast notifications #39

ENH Display toast notifications #39

Conversation

emteknetnz commented Mar 15, 2021 • edited Loading

maxime-rainville left a comment

Choose a reason for hiding this comment

emteknetnz commented Mar 30, 2021

maxime-rainville 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

maxime-rainville 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

maxime-rainville commented Apr 7, 2021

emteknetnz commented Mar 15, 2021 •

edited

Loading

maxime-rainville left a comment •

edited

Loading