From b68142cfa5e4f3c73c0138a2f6001f2d8dd671cf Mon Sep 17 00:00:00 2001
From: Awais Ansari <79941147+awais-ansari@users.noreply.github.com>
Date: Wed, 5 May 2021 18:26:29 +0500
Subject: [PATCH] style: update piazza config form according to hifi design and
 add action test case (#88)

---
 .../discussions/DiscussionsSettings.jsx       |  4 +-
 .../apps/lti/LtiConfigForm.jsx                | 41 +++++++++++--------
 .../app-config-form/apps/lti/messages.js      |  6 +--
 .../discussions/data/redux.test.js            | 18 +++++++-
 .../discussions/data/slice.js                 |  7 ++++
 .../discussions/messages.js                   |  5 +++
 6 files changed, 59 insertions(+), 22 deletions(-)

diff --git a/src/pages-and-resources/discussions/DiscussionsSettings.jsx b/src/pages-and-resources/discussions/DiscussionsSettings.jsx
index c6b076447..500b07c31 100644
--- a/src/pages-and-resources/discussions/DiscussionsSettings.jsx
+++ b/src/pages-and-resources/discussions/DiscussionsSettings.jsx
@@ -30,7 +30,7 @@ const SETTINGS_STEP = 'settings';
 function DiscussionsSettings({ courseId, intl }) {
   const dispatch = useDispatch();
   const { path: pagesAndResourcesPath } = useContext(PagesAndResourcesContext);
-  const { status } = useSelector(state => state.discussions);
+  const { status, hasValidationError } = useSelector(state => state.discussions);
 
   useEffect(() => {
     dispatch(fetchApps(courseId));
@@ -119,6 +119,8 @@ function DiscussionsSettings({ courseId, intl }) {
             <Stepper.Step
               eventKey={SETTINGS_STEP}
               title={intl.formatMessage(messages.settings)}
+              description={hasValidationError ? intl.formatMessage(messages.Incomplete) : ''}
+              hasError={hasValidationError}
             >
               <AppConfigForm
                 courseId={courseId}
diff --git a/src/pages-and-resources/discussions/app-config-form/apps/lti/LtiConfigForm.jsx b/src/pages-and-resources/discussions/app-config-form/apps/lti/LtiConfigForm.jsx
index 8a83f8e53..043f06791 100644
--- a/src/pages-and-resources/discussions/app-config-form/apps/lti/LtiConfigForm.jsx
+++ b/src/pages-and-resources/discussions/app-config-form/apps/lti/LtiConfigForm.jsx
@@ -1,16 +1,20 @@
-import React from 'react';
+import React, { useEffect } from 'react';
 import PropTypes from 'prop-types';
 import { FormattedMessage, injectIntl, intlShape } from '@edx/frontend-platform/i18n';
 import { Card, Form, Hyperlink } from '@edx/paragon';
 import { useFormik } from 'formik';
 import * as Yup from 'yup';
+import { useDispatch } from 'react-redux';
 
+import {
+  updateValidationStatus,
+} from '../../../data/slice';
 import messages from './messages';
-import AppConfigFormDivider from '../shared/AppConfigFormDivider';
 
 function LtiConfigForm({
   appConfig, app, onSubmit, intl, formRef, title,
 }) {
+  const dispatch = useDispatch();
   const {
     handleSubmit,
     handleChange,
@@ -32,12 +36,15 @@ function LtiConfigForm({
   const isInvalidConsumerSecret = touched.consumerSecret && errors.consumerSecret;
   const isInvalidLaunchUrl = touched.launchUrl && errors.launchUrl;
 
+  useEffect(() => {
+    dispatch(updateValidationStatus({ hasError: Object.keys(errors).length > 0 }));
+  }, [errors]);
+
   return (
-    <Card className="mb-5 pt-3 px-5 pb-5" data-testid="ltiConfigForm">
+    <Card className="mb-5 p-4" data-testid="ltiConfigForm">
       <Form ref={formRef} onSubmit={handleSubmit}>
-        <h3>{title}</h3>
-        <AppConfigFormDivider />
-        <p>
+        <h3 className="mb-3">{title}</h3>
+        <p className="mb-4">
           <FormattedMessage
             id="authoring.discussions.appDocInstructions"
             defaultMessage="{documentationPageLink} to set up the tool, then paste your consumer key and consumer secret below:"
@@ -55,42 +62,42 @@ function LtiConfigForm({
             }}
           />
         </p>
-        <Form.Group controlId="consumerKey" isInvalid={isInvalidConsumerKey}>
-          <Form.Label>{intl.formatMessage(messages.consumerKey)}</Form.Label>
+        <Form.Group controlId="consumerKey" isInvalid={isInvalidConsumerKey} className="mb-4">
           <Form.Control
+            floatingLabel={intl.formatMessage(messages.consumerKey)}
             onChange={handleChange}
             onBlur={handleBlur}
             value={values.consumerKey}
           />
           {isInvalidConsumerKey && (
-          <Form.Control.Feedback type="invalid">
-            {errors.consumerKey}
+          <Form.Control.Feedback type="invalid" hasIcon={false}>
+            <span className="x-small">{errors.consumerKey}</span>
           </Form.Control.Feedback>
           )}
         </Form.Group>
-        <Form.Group controlId="consumerSecret" isInvalid={isInvalidConsumerSecret}>
-          <Form.Label>{intl.formatMessage(messages.consumerSecret)}</Form.Label>
+        <Form.Group controlId="consumerSecret" isInvalid={isInvalidConsumerSecret} className="mb-4">
           <Form.Control
+            floatingLabel={intl.formatMessage(messages.consumerSecret)}
             onChange={handleChange}
             onBlur={handleBlur}
             value={values.consumerSecret}
           />
           {isInvalidConsumerSecret && (
-          <Form.Control.Feedback type="invalid">
-            {errors.consumerSecret}
+          <Form.Control.Feedback type="invalid" hasIcon={false}>
+            <span className="x-small">{errors.consumerSecret}</span>
           </Form.Control.Feedback>
           )}
         </Form.Group>
         <Form.Group controlId="launchUrl" isInvalid={isInvalidLaunchUrl}>
-          <Form.Label>{intl.formatMessage(messages.launchUrl)}</Form.Label>
           <Form.Control
+            floatingLabel={intl.formatMessage(messages.launchUrl)}
             onChange={handleChange}
             onBlur={handleBlur}
             value={values.launchUrl}
           />
           {isInvalidLaunchUrl && (
-          <Form.Control.Feedback type="invalid">
-            {errors.launchUrl}
+          <Form.Control.Feedback type="invalid" hasIcon={false}>
+            <span className="x-small">{errors.launchUrl}</span>
           </Form.Control.Feedback>
           )}
         </Form.Group>
diff --git a/src/pages-and-resources/discussions/app-config-form/apps/lti/messages.js b/src/pages-and-resources/discussions/app-config-form/apps/lti/messages.js
index fba37ceea..2f20eabe5 100644
--- a/src/pages-and-resources/discussions/app-config-form/apps/lti/messages.js
+++ b/src/pages-and-resources/discussions/app-config-form/apps/lti/messages.js
@@ -12,7 +12,7 @@ const messages = defineMessages({
   },
   consumerKeyRequired: {
     id: 'authoring.discussions.consumerKey.required',
-    defaultMessage: 'Consumer Key is a required field.',
+    defaultMessage: 'Consumer key is a required field',
     description: 'Tells the user that the Consumer Key field is required and must have a value.',
   },
   consumerSecret: {
@@ -22,7 +22,7 @@ const messages = defineMessages({
   },
   consumerSecretRequired: {
     id: 'authoring.discussions.consumerSecret.required',
-    defaultMessage: 'Consumer Secret is a required field.',
+    defaultMessage: 'Consumer secret is a required field',
     description: 'Tells the user that the Consumer Secret field is required and must have a value.',
   },
   launchUrl: {
@@ -32,7 +32,7 @@ const messages = defineMessages({
   },
   launchUrlRequired: {
     id: 'authoring.discussions.launchUrl.required',
-    defaultMessage: 'Launch URL is a required field.',
+    defaultMessage: 'Launch URL is a required field',
     description: 'Tells the user that the Launch URL field is required and must have a value.',
   },
 });
diff --git a/src/pages-and-resources/discussions/data/redux.test.js b/src/pages-and-resources/discussions/data/redux.test.js
index 4fd624d9d..8392b35fb 100644
--- a/src/pages-and-resources/discussions/data/redux.test.js
+++ b/src/pages-and-resources/discussions/data/redux.test.js
@@ -5,7 +5,7 @@ import { history } from '@edx/frontend-platform';
 import initializeStore from '../../../store';
 import { getAppsUrl } from './api';
 import {
-  FAILED, SAVED, DENIED, selectApp,
+  FAILED, SAVED, DENIED, selectApp, updateValidationStatus,
 } from './slice';
 import { fetchApps, saveAppConfig } from './thunks';
 import { LOADED } from '../../../data/slice';
@@ -102,6 +102,7 @@ describe('Data layer integration tests', () => {
           selectedAppId: null,
           status: FAILED,
           saveStatus: SAVED,
+          hasValidationError: false,
         }),
       );
     });
@@ -119,6 +120,7 @@ describe('Data layer integration tests', () => {
           selectedAppId: null,
           status: DENIED,
           saveStatus: SAVED,
+          hasValidationError: false,
         }),
       );
     });
@@ -135,6 +137,7 @@ describe('Data layer integration tests', () => {
         selectedAppId: null,
         status: LOADED,
         saveStatus: SAVED,
+        hasValidationError: false,
       });
       expect(store.getState().models.apps.legacy).toEqual(legacyApp);
       expect(store.getState().models.apps.piazza).toEqual(piazzaApp);
@@ -159,6 +162,7 @@ describe('Data layer integration tests', () => {
         selectedAppId: null,
         status: LOADED,
         saveStatus: SAVED,
+        hasValidationError: false,
       });
       expect(store.getState().models.apps.legacy).toEqual(legacyApp);
       expect(store.getState().models.apps.piazza).toEqual(piazzaApp);
@@ -188,6 +192,14 @@ describe('Data layer integration tests', () => {
     });
   });
 
+  describe('updateValidationStatus', () => {
+    test.each([true, false])('sets hasValidationError value to %s ', (hasError) => {
+      store.dispatch(updateValidationStatus({ hasError }));
+
+      expect(store.getState().discussions.hasValidationError).toEqual(hasError);
+    });
+  });
+
   describe('saveAppConfig', () => {
     test('network error', async () => {
       history.push(`/course/${courseId}/pages-and-resources/discussions/configure/piazza`);
@@ -210,6 +222,7 @@ describe('Data layer integration tests', () => {
           selectedAppId: 'piazza',
           status: LOADED,
           saveStatus: FAILED,
+          hasValidationError: false,
         }),
       );
     });
@@ -235,6 +248,7 @@ describe('Data layer integration tests', () => {
           selectedAppId: 'piazza',
           status: DENIED, // We set BOTH statuses to DENIED for saveAppConfig - this removes the UI.
           saveStatus: DENIED,
+          hasValidationError: false,
         }),
       );
     });
@@ -288,6 +302,7 @@ describe('Data layer integration tests', () => {
           selectedAppId: 'piazza',
           status: LOADED,
           saveStatus: SAVED,
+          hasValidationError: false,
         }),
       );
       expect(store.getState().models.appConfigs.piazza).toEqual({
@@ -352,6 +367,7 @@ describe('Data layer integration tests', () => {
           selectedAppId: 'legacy',
           status: LOADED,
           saveStatus: SAVED,
+          hasValidationError: false,
         }),
       );
       expect(store.getState().models.appConfigs.legacy).toEqual({
diff --git a/src/pages-and-resources/discussions/data/slice.js b/src/pages-and-resources/discussions/data/slice.js
index c10b4af0d..49092f150 100644
--- a/src/pages-and-resources/discussions/data/slice.js
+++ b/src/pages-and-resources/discussions/data/slice.js
@@ -21,6 +21,8 @@ const slice = createSlice({
     selectedAppId: null,
     status: LOADING,
     saveStatus: SAVED,
+    // ValidationError is the Flag that represents a form validation status.
+    hasValidationError: false,
   },
   reducers: {
     loadApps: (state, { payload }) => {
@@ -42,6 +44,10 @@ const slice = createSlice({
       const { status } = payload;
       state.saveStatus = status;
     },
+    updateValidationStatus: (state, { payload }) => {
+      const { hasError } = payload;
+      state.hasValidationError = hasError;
+    },
   },
 });
 
@@ -50,6 +56,7 @@ export const {
   selectApp,
   updateStatus,
   updateSaveStatus,
+  updateValidationStatus,
 } = slice.actions;
 
 export const {
diff --git a/src/pages-and-resources/discussions/messages.js b/src/pages-and-resources/discussions/messages.js
index b7ea0d55c..163bd1513 100644
--- a/src/pages-and-resources/discussions/messages.js
+++ b/src/pages-and-resources/discussions/messages.js
@@ -39,6 +39,11 @@ const messages = defineMessages({
     defaultMessage: 'Provider selection',
     description: 'A label for the first step of a wizard where the user chooses a discussion tool to configure.',
   },
+  Incomplete: {
+    id: 'authoring.discussions.Incomplete',
+    defaultMessage: 'Incomplete',
+    description: 'A description for the second step of the app configuration stepper.',
+  },
 });
 
 export default messages;