#166840878 User signup error swag #21

tolumide-ng · 2019-08-05T18:52:53Z

What does this PR do?

Enables user to signup on errorswag

Description of Task to be completed?

User can signup on errorswag

How should this be manually tested?

Navigate to the signup page on errorswag

Any background context you want to provide?

What are the relevant pivotal tracker stories?(if applicable)

#166840878

Screenshots

166840878

Onyimatics

Good job @tolumide-ng and @olorunwalawrence.
Please endeavour to remove all the unneeded comments on your code.

Onyimatics · 2019-08-06T08:39:26Z

src/pages/Signup/index.js

+    <div className="w-7/12 p-10">
+      {/* logo and inputs */}
+      <div className="w-full flex flex-col items-center p-2 mx-auto">
+        {/* <div className="w-5/6 mx-auto bg-white mt-10 mb-4 text-center flex flex-col items-center"> */}


you may need to remove these comments please.

Onyimatics · 2019-08-06T08:45:30Z

src/fakeData.js

+      }
+    }
+  ]
+};


Please what is the value of this file to this PR?

segunolalive

@olorunwalawrence @tolumide-ng

This is good. Tests need a bit more work but good job guys.

segunolalive · 2019-08-09T09:59:54Z

tests/signup.spec.js

+});
+const fake = jest.fn();
+
+describe('it should work', () => {


This should be the component being described

segunolalive · 2019-08-09T10:03:04Z

tests/signup.spec.js

+  it('renders the component', () => {
+    expect(wrapper.exists()).toBe(true);
+    expect(wrapper.find('Form').exists()).toBe(true);
+    expect(wrapper.find('button').exists()).toBe(true);


I think asserting that there's a submit button (type=submit)/ the text in the button is more useful than just that there's a button.

Assert the number of buttons. Other important form elements as well.

This test doesn't actually test that the component signs a user up. And that is reasonable because the component doesn't sign a user up. The action/network request does that. The component only displays what is appropriate based on the application state.

Also, add test for negative paths e.g when a user enters invalid data.

Hypothetical Solution

const jest.fn(() => 'lol') state = { signUp } // then in your test. sbmitBtn.simulate('click'); // then you should be able to do: expect(signUp).toHaveBeenCalled(); expect(signUp).toHaveBeenCalledTimes(1);

codexempire

Nice work Noticed the integration of various packages including toastr left some comment behind

Apart from the comment I also think you should try removing .DS_STORE from the PR

codexempire · 2019-08-13T15:58:15Z

src/pages/Signup/index.js

+import { Formik, Form, Field } from 'formik';
+import { signupAction } from '../../store/modules/auth/actions/signup';
+import logo from '../../assets/images/logo.png';
+// import signupCard from '../../assets/images/signup.png';


Remove comments from the code

src/pages/Signup/index.js

src/components/App.js

tolumide-ng · 2019-08-20T23:58:24Z

Good job @olorunwalawrence @tolumide-ng, However, I noticed your actions and thunk are not properly tested which caused the reduction in your test coverage.
You should ensure the thunks are tested to dispatch the right actions. Also, use the mock store dependency to mock the store and moxios to mock the API requests in your thunk.

You may also want to check out this test example - https://github.com/segunolalive/helloBooks/blob/development/client/__tests__/actions/authActions.spec.js
and this
https://github.com/andela/ah-kifaru-frontend/pull/27/files#diff-8

Thank you for your observation. The tests are already been catered for by @kechyy since we are using the same actions and reducers

Onyimatics

Well done guys. You may need to check out the comments I left.

Onyimatics · 2019-08-22T03:38:13Z

src/components/SocialMedia/social.spec.js

@@ -77,7 +79,7 @@ describe('SocialMediaLogin', () => {
      </Provider>
    );

-    expect(localStorage.getItem('token')).toEqual(userToken);
+    // expect(localStorage.getItem('token')).toEqual(userToken);


I suggest you reach out next time, instead of commenting this assertion out.
You may need to replace that assertion with

expect(JSON.parse(localStorage.getItem('token'))).toEqual(userToken); Thank you.

I suggest you reach out next time, instead of commenting this assertion out.
You may need to replace that assertion with

expect(JSON.parse(localStorage.getItem('token'))).toEqual(userToken); Thank you.

Hello @Onyimatics , thank you for the review. However, there is no problem with the previous assertion. I only forgot to remove the comments after testing what I needed on this part of the code. Thank you.

Onyimatics · 2019-08-22T03:48:35Z

src/pages/Signup/index.spec.js

+  location,
+  status: 'authenticationLoading',
+  SignUp: jest.fn(({ userData = {}, history, location }, cb) => {
+    cb({ data: true });


I suggest you consider making the name of this cb more descriptive.

Onyimatics · 2019-08-22T03:50:21Z

src/pages/Signup/index.spec.js

+  );
+};
+
+describe.only('SignupForm User Input Validation', () => {


I guess you must have forgotten to remove this .only

Yh, I did. Thank you

Onyimatics · 2019-08-22T04:11:33Z

src/store/modules/auth/actions/index.js

-    const { message } = error.response.data;
+    return url ? history.push(url) : history.push('/articles');
+  } catch ({ response }) {
+    const { message } = response.data;


instead of removing these lines

} catch (error) { const { message } = error.response.data; ```, I suggest you use an `if` statement there to also cater for the error coming from `error.response.data` , incase a user encounters a `network error`.

Oh True. However, In the case of a network failure, axios still returns a response since that it is what was stated on the try block.
Hence, I think
const message = response && response.data && response.data.message
might be okay. What do you think?

tohbay · 2019-08-22T07:57:47Z

__test__/App.spec.js

@@ -1,6 +1,7 @@
 import React from 'react';
 import { BrowserRouter, Switch } from 'react-router-dom';
 import thunk from 'redux-thunk';
+import { mount } from 'enzyme';


Hi @tolumide-ng and @olorunwalawrence,

Please remove the mount function since it has been configured globally on the project.

tohbay · 2019-08-22T07:57:56Z

__test__/components/navbar.spec.js

@@ -1,6 +1,7 @@
 import React from 'react';
 import { Provider } from 'react-redux';
 import configureStore from 'redux-mock-store';
+import { mount } from 'enzyme';


Hi @tolumide-ng and @olorunwalawrence,

Please remove the mount function since it has been configured globally on the project.

sorry this mount is on navbar component not on the signup

tohbay · 2019-08-22T08:47:33Z

enzyme.config.js

@@ -0,0 +1,4 @@
+import { configure } from 'enzyme';


I think there is no need for this file enzyme.config.js, the configurations therein are already implemented on __test__/setup.js

tohbay

Good job @tolumide-ng and @olorunwalawrence 👍

There are 4 commits, please squash them. Also, attend to the comments.

tolumide-ng · 2019-08-22T08:51:37Z

Good job @tolumide-ng and @olorunwalawrence 👍

There are 4 commits, please squash them. Also, attend to the comments.

Thank you for your review. However, commits cannot be squashed yet as instructed by the TTL since this is a collaborated PR

tolumide-ng · 2019-08-22T09:22:30Z

Good job @olorunwalawrence @tolumide-ng, However, I noticed your actions and thunk are not properly tested which caused the reduction in your test coverage.
You should ensure the thunks are tested to dispatch the right actions. Also, use the mock store dependency to mock the store and moxios to mock the API requests in your thunk.

You may also want to check out this test example - https://github.com/segunolalive/helloBooks/blob/development/client/__tests__/actions/authActions.spec.js
and this
https://github.com/andela/ah-kifaru-frontend/pull/27/files#diff-8

Thank you @timi-codes . The actions, thunk and reducers are not tested at all in this PR since this feature was implemented in conjunction with the login feature (assigned to @tobechukwuobitube and @kechyy ), they are already testing the actions and reducers appropriately. Thank you once again

timi-codes

@olorunwalawrence @tolumide-ng Good job on this pull request. However, Kindly attend to all the changes as soon as possible so that we can have this merged today.

timi-codes · 2019-08-22T09:20:35Z

package.json

    "glob": "^7.1.4",
    "html-webpack-plugin": "^3.2.0",
+    "i": "^0.3.6",


kindly remove this as it must have been installed in err.

timi-codes · 2019-08-22T09:20:42Z

package.json

    "jest-svg-transformer": "^1.0.0",
    "jwt-decode": "^2.2.0",
    "mini-css-extract-plugin": "^0.8.0",
+    "npm": "^6.11.0",


kindly remove this as it must have been installed in err.

src/components/App.js

timi-codes · 2019-08-22T09:44:19Z

src/index.css

@@ -0,0 +1,9 @@
+@tailwind base;


Please use the main.scssfor global stylings

I guess, this was when you changed the file configuration to sass as it was initially index.css when I setup the tailwindcss
I would remove that now

tolumide-ng · 2019-08-22T09:55:22Z

@olorunwalawrence @tolumide-ng Good job on this pull request. However, Kindly attend to all the changes as soon as possible so that we can have this merged today.

Changes attended. Please rereview

segunolalive

🎊 This is good.

Make the correction on the SignUp form header and merge.

segunolalive · 2019-08-22T13:27:50Z

src/pages/Signup/index.js

+  <div className="w-11/12 md:w-9/12 lg:w-7/12 mx-auto m-16 rounded-lg flex items-stretch bg-red-400 h-100">
+    <div className="md:flex max-w-lg w-7/12 bg-color height items-center signup-card hidden rounded-l-lg ">
+      <div className=" pl-8 font-bold text-left text-white text-2xl">
+        <div>Welcome back,</div>


Change Welcome back, to Welcome to ErrorSwag.

Wrap this text in an h1 tag or the most appropriate heading tag. This is important for accessibility and SEO.

Use <br> tags to break the text into multiple lines if needed.

- Corrects bug on password non-conformity - Adds Animation to show loading during user signup - Adds Social Media Buttons for user signup authentication - Fixes CSS on user signup - Implements state and reducers for signup - Creates UI for errorswag signup page - Returns relevant error message on user signup action - Implements recommended feedback on UI label and redux state params - Implements state and reducers for signup - Creates UI for errorswag signup page - Returns relevant error message on user signup action - Implements recommended feedback on UI label and redux state params [Delivers #166840878]

tolumide-ng force-pushed the ft-user-signup-errorSwag-166840878 branch from 00065de to 280da95 Compare August 5, 2019 19:24

Onyimatics suggested changes Aug 6, 2019

View reviewed changes

tolumide-ng force-pushed the ft-user-signup-errorSwag-166840878 branch 5 times, most recently from a7b6752 to 0cf2fdf Compare August 6, 2019 11:00

olorunwalawrence force-pushed the ft-user-signup-errorSwag-166840878 branch from 0cf2fdf to e1e82b7 Compare August 6, 2019 22:07

tolumide-ng temporarily deployed to errorswagfrontend-stagin-pr-21 August 6, 2019 22:07 Inactive

tolumide-ng force-pushed the ft-user-signup-errorSwag-166840878 branch from e1e82b7 to ddb969e Compare August 7, 2019 10:19

tolumide-ng temporarily deployed to errorswagfrontend-stagin-pr-21 August 7, 2019 10:19 Inactive

tolumide-ng force-pushed the ft-user-signup-errorSwag-166840878 branch from ddb969e to 6f1930c Compare August 7, 2019 10:59

tolumide-ng temporarily deployed to errorswagfrontend-stagin-pr-21 August 7, 2019 10:59 Inactive

tolumide-ng force-pushed the ft-user-signup-errorSwag-166840878 branch from 6f1930c to bf67ce4 Compare August 7, 2019 21:05

tolumide-ng temporarily deployed to errorswagfrontend-stagin-pr-21 August 7, 2019 21:06 Inactive

tolumide-ng force-pushed the ft-user-signup-errorSwag-166840878 branch from bf67ce4 to 927f99b Compare August 7, 2019 21:11

tolumide-ng temporarily deployed to errorswagfrontend-stagin-pr-21 August 7, 2019 21:12 Inactive

tolumide-ng force-pushed the ft-user-signup-errorSwag-166840878 branch from 927f99b to 2a8d4d9 Compare August 7, 2019 21:16

tolumide-ng temporarily deployed to errorswagfrontend-stagin-pr-21 August 7, 2019 21:16 Inactive

tolumide-ng force-pushed the ft-user-signup-errorSwag-166840878 branch from 2a8d4d9 to 710b910 Compare August 7, 2019 21:20

tolumide-ng deployed to errorswagfrontend-stagin-pr-21 August 7, 2019 21:21 View deployment

segunolalive suggested changes Aug 9, 2019

View reviewed changes

segunolalive requested review from codexempire and tohbay August 9, 2019 10:49

tolumide-ng force-pushed the ft-user-signup-errorSwag-166840878 branch 2 times, most recently from 8132b66 to d3839fb Compare August 9, 2019 22:27

codexempire reviewed Aug 13, 2019

View reviewed changes

olorunwalawrence force-pushed the ft-user-signup-errorSwag-166840878 branch from d3839fb to 5c9c31b Compare August 14, 2019 06:53

tolumide-ng force-pushed the ft-user-signup-errorSwag-166840878 branch from 5c9c31b to e3560d1 Compare August 14, 2019 12:40

tolumide-ng requested a review from Onyimatics August 14, 2019 16:55

Onyimatics suggested changes Aug 22, 2019

View reviewed changes

tohbay reviewed Aug 22, 2019

View reviewed changes

tohbay suggested changes Aug 22, 2019

View reviewed changes

tolumide-ng requested a review from tohbay August 22, 2019 08:58

tolumide-ng force-pushed the ft-user-signup-errorSwag-166840878 branch 2 times, most recently from 871a7a1 to c82892c Compare August 22, 2019 09:04

tolumide-ng requested a review from Onyimatics August 22, 2019 09:14

tolumide-ng force-pushed the ft-user-signup-errorSwag-166840878 branch from c82892c to 35b3024 Compare August 22, 2019 09:17

tolumide-ng requested a review from olorunwalawrence August 22, 2019 09:19

timi-codes suggested changes Aug 22, 2019

View reviewed changes

tolumide-ng force-pushed the ft-user-signup-errorSwag-166840878 branch 4 times, most recently from 5d3e1d7 to 74efdeb Compare August 22, 2019 10:25

tolumide-ng requested a review from timi-codes August 22, 2019 10:26

tolumide-ng force-pushed the ft-user-signup-errorSwag-166840878 branch 4 times, most recently from 33e1b37 to 63f7d0c Compare August 22, 2019 13:15

segunolalive suggested changes Aug 22, 2019

View reviewed changes

tolumide-ng force-pushed the ft-user-signup-errorSwag-166840878 branch from 63f7d0c to 304e38f Compare August 22, 2019 13:40

tolumide-ng force-pushed the ft-user-signup-errorSwag-166840878 branch from 304e38f to 0b55939 Compare August 22, 2019 13:49

tolumide-ng merged commit 5cf2ed9 into develop Aug 22, 2019

#166840878 User signup error swag #21

#166840878 User signup error swag #21

Conversation

tolumide-ng commented Aug 5, 2019

What does this PR do?

Description of Task to be completed?

How should this be manually tested?

Any background context you want to provide?

What are the relevant pivotal tracker stories?(if applicable)

Screenshots

Onyimatics left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

segunolalive 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

codexempire left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tolumide-ng commented Aug 20, 2019

Onyimatics 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

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

tohbay left a comment

Choose a reason for hiding this comment

tolumide-ng commented Aug 22, 2019

tolumide-ng commented Aug 22, 2019

timi-codes 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

timi-codes Aug 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tolumide-ng commented Aug 22, 2019

segunolalive left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codexempire left a comment •

edited

Loading

timi-codes Aug 22, 2019 •

edited

Loading