Fixes #22683 - Refactor PasswordStrength folder structure #5240

sharvit · 2018-02-07T08:30:36Z

I have been starting from the PasswordStrength because it was the easiest (the files are most organized).

http://projects.theforeman.org/issues/22473

The current goal is to make this component perfect so we can use it later as a boilerplate.
Before continuing to the other components, I want to make sure everyone is agree about the new structure.

Goals:

Migrate to a domain level folder structure
Have integration test for the connected component
Have unit tests for all the other parts
Make it scaleable
Introduce redux selectors

In order to do so, i refactor the component, the tests, the actions and the reducers.

I migrated the actions from updatePassword and checkPasswordsMatch to updatePassword, updatePasswordConfirmation and created a selector call doesPasswordsMatch.

I have been trying many different variations and i feel i most like this structure:

PasswordStrength
├── __tests__
│   ├── __snapshots__
│   │   ├── integration.test.js.snap
│   │   ├── PasswordStrengthActions.test.js.snap
│   │   ├── PasswordStrengthReducer.test.js.snap
│   │   └── PasswordStrength.test.js.snap
│   ├── integration.test.js // integration test
│   ├── PasswordStrengthActions.test.js // actions unit tests
│   ├── PasswordStrengthReducer.test.js // reducer unit tests
│   ├── PasswordStrengthSelectors.test.js // selectors unit tests
│   └── PasswordStrength.test.js // component unit tests
├── index.js // integration file
├── PasswordStrength.js // component
├── PasswordStrength.scss // styles
├── PasswordStrengthActions.js // actions
├── PasswordStrengthConstants.js // consts
├── PasswordStrengthReducer.js // reducers
└── PasswordStrengthSelectors.js // selectors

Why i moved the tests into the __tests__ folder?
The file list just getting to long and it can get confusing.

Why do we need index.js? Why not connecting the component into the store from the PasswordStrength.js?

When we lose the reference to the unconnected component, it became hard to unit-test them because you must use a store.
It feels right when index.js responsibility is to integrate the different parts of the domain.

How do we scale it to have more inner components?
Creating a components folder and put them inside.

Looking forward to get some feedback here.

@amirfefer @ohadlevy @waldenraines @tstrachota @danseethaler @priley86

theforeman-bot · 2018-02-07T08:30:44Z

Issues: #22473

sharvit · 2018-02-07T08:45:26Z

webpack/assets/javascripts/react_app/components/PasswordStrength/index.js

+const mapDispatchToProps = dispatch => bindActionCreators(actions, dispatch);
+
+// export reducers
+export const reducers = { passwordStrength: reducer };


The passwordStrength key must match the key in mapStateToProps, otherwise, it will break!
This is the reason i decided to move the key deceleration from reducers/index.js into this object.

I think it would be more reliable if we keep all the configuration of the PasswordStrength inside the folder, so the combineReducers method should consume the key and the reducer.

sharvit · 2018-02-07T08:45:43Z

webpack/assets/javascripts/react_app/redux/reducers/index.js

    statistics,
    hosts,
    notifications,
    toasts,
-    passwordStrength,
+    ...passwordStrengthReducers,


See:
https://github.com/theforeman/foreman/pull/5240/files#diff-be94fe9aa3f3ab8239d637d315cfb884R19

it a bit confused situation when you have both ways implemented here.
IMHO if we adopt this approach (which i can see some benefits from) we should change this entire file

Will do it in a different pr 👍

priley86 · 2018-02-07T22:08:20Z

@sharvit i like the use of redux-selectors... can you explain a bit more about the domain driven folder structure? I guess typically actions/reducers are used by a single connected component, but what if they are used by many different connected components (or potentially unrelated connected components)? I would just appreciate some background reading material on this...The folder structure proposed seems like it would help in most cases though...

sharvit · 2018-02-08T09:27:26Z

Thanks @priley86, this is actually a very good question.

Let's say I have another component called PasswordGenerator written in the same way and I want to use some staff from the PasswordStrength.

import { updatePassword } from '../PasswordStrength/PasswordStrengthActions';
import { doesPasswordsMatch } from '../PasswordStrength/PasswordStrengthSelectors';
import { reducers as PasswordStrengthReducers } from '../PasswordStrength';

import PasswordGenerator from './PasswordGenerator';

const PasswordStrengthReducerKey = Object.keys(PasswordStrengthReducers)[0];

// map state to props
const mapStateToProps = ({ [PasswordStrengthReducerKey]: passwordStrength }) => ({
  password: passwordStrength.password,
  passwordConfirmation: passwordStrength.passwordConfirmation,
  doesPasswordsMatch: doesPasswordsMatch(passwordStrength),
});

// map action dispatchers to props
const mapDispatchToProps = dispatch => bindActionCreators({ updatePassword }, dispatch);

export default connect(mapStateToProps, mapDispatchToProps)(PasswordGenerator);

This kind of makes me wonder, should we strict the export reducers to only one reducer?

Maybe it can be?

export const reducer = { key: 'passwordStrength', reducer: PasswordStrengthReducer };

Or?

export const reducerKey = 'passwordStrength';
export const reducer = PasswordStrengthReducer;

@priley86 you can see the discussion lead us to this decision here:
https://community.theforeman.org/t/suggestion-refactor-the-react-app-folder-structure/7843/6

TL;DR
It makes development confusing when you work on a component and the component files are split into many different directories.

sharvit · 2018-02-08T09:43:24Z

This post gave us some motivation:
https://marmelab.com/blog/2015/12/17/react-directory-structure.html

More cool links about the topic:
https://gist.github.com/ryanflorence/daafb1e3cb8ad740b346
https://www.reddit.com/r/reactjs/comments/47mwdd/a_better_file_structure_for_reactredux/

priley86 · 2018-02-08T15:03:09Z

@sharvit thanks a lot for the links... i like the new structure much better (and it makes sense to me). Will start trying to migrate this way soon myself.

react-app/components/PasswordStrength
├── __tests__
│   ├── __snapshots__
│   │   ├── index.test.js.snap
│   │   ├── PasswordStrengthActions.test.js.snap
│   │   ├── PasswordStrengthReducer.test.js.snap
│   │   └── PasswordStrength.test.js.snap
│   ├── index.test.js // integration test
│   ├── PasswordStrengthActions.test.js // actions unit tests
│   ├── PasswordStrengthReducer.test.js // reducer unit tests
│   ├── PasswordStrengthSelectors.test.js // selectors unit tests
│   └── PasswordStrength.test.js // component unit tests
├── index.js // integration file
├── PasswordStrength.js // component
├── PasswordStrength.scss // styles
├── PasswordStrengthActions.js // actions
├── PasswordStrengthConstants.js // consts
├── PasswordStrengthReducer.js // reducers
└── PasswordStrengthSelectors.js // selectors

but is there ever a case for:

react_app/redux/reducers/index.js (root reducer for the domain reducers mentioned above)
react_app/redux/reducers/common/someCommonReducer/someCommonReducerEverywhere.js

I still don't know if that use case exists? Obviously we are trying to avoid it with the new "domain" path though...

sharvit · 2018-02-08T15:31:39Z

@priley86, I honestly not sure about it.

From one side, having a common reducer feels like a great tool in my toolbox.
From the other side, we never had the need to use it and it may feel like an anti-pattern so I think we can avoid that (even for the long-term).

I think we should continue in this direction and wait to see if this use-case appear.

priley86 · 2018-02-08T22:51:00Z

@sharvit yea.. it makes sense... i also see the case for continuing to nest "shared" or "common" appropriately (like you have currently in Foreman)... so basically, I would think you have the structure you've proposed above for each component, and nesting appropriately within screens/App like it is here:
https://gist.github.com/ryanflorence/daafb1e3cb8ad740b346#shared-modules

It kind of sticks for me, but not sure what others think...

waldenraines · 2018-02-09T18:24:55Z

Overall I like this approach. I can't wrap my head around why they call it a "selector" but I do understand why it's factored out. Do we want to just put these in the reducers file though? Will they be used outside of the context of a reducer?

sharvit · 2018-02-11T10:08:09Z

@waldenraines - Usually, they used to do stuff like getTodoById(id) selectTodosFromToday() selectTodosByCategory(category)

priley86 · 2018-02-11T17:07:32Z

@sharvit reviewing this approach once more (and would like to call it out)... one of the things I like most is you've introduced react_app/components/PasswordStrength/__tests__/index.test.js integrational tests. I personally see more value from writing these kind of tests than for us to be responsible for writing a lot of e2e tests running in the selenium browser (as we might have previously). Maybe QE team can help with those sort of e2e tests? If we do become responsible, maybe we can look at something easy to configure like cypress.io, but i'm not sure what tools we have available for that.

I read these articles on the integration tests topic and I agree w/ them (integration can be more high value than unit or e2e for us):
https://kentcdodds.com/post/write-integration-tests/
https://hackernoon.com/low-effort-high-value-integration-tests-in-redux-apps-d3a590bd9fd5

Was having this sort of a debate w/ a friend recently ;)

priley86 · 2018-02-13T05:01:45Z

@sharvit i had no issues with this structure. Very nice work. I've started a parallel PR for our v2v efforts here.

I need to do a bit more homework with the tests you've started here, but was at least able to render the mapStateToProps results (giving me some simple integration tests).

I'd also like to propose we adopt something similar to Ryan Florence's route hierarchy/shared module structure noted above (link). Feel free to revise as you'd like 😸. I'm happy with it though...

sharvit · 2018-02-13T09:58:16Z

@priley86 we kind of had the same debate here.
We wrote unit tests that doing tons of integration tests because they don't mock the imported modules. We only did mocking for the ajax calls.

So we end up asking ourselves, How should we write those unit-tests? and what about integration tests?
We can't migrate all our tests to be pure unit-tests without having a solution for integration-tests.

I believe our products will be more reliable if we will split them into as many stand-alone packages as we can.

e2e testing can be leaner now since we will have integration testing for all inner packages.
In this case, e2e don't need to do more than just loading the app, navigate between the pages and make sure they contain the relevant packages inside. I don't see a need to interact with the inner components.

We are currently doing kind of an e2e testing in our rails stack and I would love moving them into the js stack.
I have no experience with tools like cypress.io but from watching their videos, it feels to me like a closed-garden and an overkill for our needs.

We are still not a single-page-app, we are not using react-router and big parts of our UI are still old legacy code written with .erb files and jquery.
In our react-redux stack, we mostly have connected components which we mount into to relevant part in the UI example.
So we can't really adopt the full screens approach at the moment, but I am a big fan of this approach and embrace everyone to use it 👍
We will keep monitoring how it works for other and will adopt it later if it will still be relevant. This pr is a step into this future.

It's nice to see you could adapt it so fast! 👍 while I'm still waiting for more feedback...

Let's keep updating each other, I think it works well.

amirfefer · 2018-02-13T12:44:02Z

@sharvit 👍 Great change, I like it

ohadlevy · 2018-02-18T12:19:59Z

@amirfefer can you review please? thanks

amirfefer

Overall I like this change.
Consider combine here the Integration helper PR I mentioned in the inline comments

amirfefer · 2018-02-19T12:59:58Z

webpack/assets/javascripts/react_app/components/PasswordStrength/PasswordStrengthSelectors.js

@@ -0,0 +1,2 @@
+export const doesPasswordsMatch = ({ password, passwordConfirmation }) =>


I like the selector pattern
here you can find a nice explanation for that.
should we adopt this pattern in the entire application?

There's a nice library that uses this pattern: redux-reselect

Yes, I believe we should adopt this pattern but I wouldn't run to migrating, mostly for new features.

Doesn't the redux-reselect syntax feel a bit confusing?

was actually going to suggest some kind of memoized selector as well (after seeing that redux-reselect) previously... not much code there: https://github.com/reactjs/reselect/blob/master/src/index.js

@priley86 @amirfefer Now when I understand that reselect act as a memoized selector and I read more about it, it makes a lot of sense to use it (because it's memorized).

But, don't you think it takes a very simple concept like selectors and makes it really complicated to write and understand?
especially for developers how new to the concept of redux.

it is just minor performance gain in many cases. I think we abstract that method into a common helper (either reselect or just write our own). Then it becomes less about beginner knowledge and just a function. Personal preference though...

amirfefer · 2018-02-19T13:26:55Z

.../javascripts/react_app/components/PasswordStrength/__tests__/PasswordStrengthActions.test.js

+
+describe('PasswordStrength actions', () => {
+  it('should update password', () =>
+    expect(updatePassword('some-password')).toMatchSnapshot());


I like this simplicity, though with async actions, should we adopt redux suggestion ?

Yes in general, I will try to experience with it more in later PR and we will continue to discuss it there.

amirfefer · 2018-02-19T13:29:31Z

webpack/assets/javascripts/react_app/components/PasswordStrength/__tests__/index.test.js

@@ -0,0 +1,75 @@
+import React from 'react';


I prefer to add integeration.test prefix

amirfefer · 2018-02-19T13:30:22Z

webpack/assets/javascripts/react_app/components/PasswordStrength/__tests__/index.test.js

+
+import PasswordStrength, { reducers } from '../index';
+
+const fixtures = {


extract this to another file? a specific file for the entire fixtures?

amirfefer · 2018-02-19T13:35:28Z

webpack/assets/javascripts/react_app/components/PasswordStrength/__tests__/index.test.js

+document.getElementById = jest.fn(id => ({ value: fixtures[id].password }));
+
+describe('PasswordStrength integration test', () => {
+  const generateStore = () => createStore(combineReducers({ ...reducers }));


instead of generate store each time, would it be better to add integration setup (helper) ?
something like this: https://github.com/theforeman/foreman/pull/5205/files#diff-6d91af9e34087f473783a4de94e3c806
maybe it better to combined those two PRs?
I took inspiration from this blog

Yes, i think it makes sense thanks for pointing out 👍

Will update soon

amirfefer · 2018-02-19T13:38:55Z

webpack/assets/javascripts/react_app/components/PasswordStrength/__tests__/index.test.js

+
+  const setInputValue = (input, value) => {
+    input.instance().value = value; // eslint-disable-line no-param-reassign
+    input.simulate('change', { target: { value } });


I think you can do it in one line:

input.simulate('change', {target: {value: 'My new value'}});

Not in this case, i was disappointed to see that the plugin takes the value from the input instead of from the event.

amirfefer · 2018-02-19T13:43:15Z

webpack/assets/javascripts/react_app/redux/reducers/index.js

    statistics,
    hosts,
    notifications,
    toasts,
-    passwordStrength,
+    ...passwordStrengthReducers,


it a bit confused situation when you have both ways implemented here.
IMHO if we adopt this approach (which i can see some benefits from) we should change this entire file

amirfefer · 2018-02-19T13:48:00Z

.../assets/javascripts/react_app/components/PasswordStrength/__tests__/PasswordStrength.test.js

+      const component = shallow(<PasswordStrength {...props} />);
+
+      expect(toJson(component)).toMatchSnapshot();
+      expect(document.getElementById.mock.calls).toMatchSnapshot('getElementById calls');


doesn't it better to use toBeCalled function in this case? (instead of snapshot)

Then I will only know that the function was called.

The snapshot contains the fact that the functions called twice and the arguments from each call:
https://github.com/theforeman/foreman/pull/5240/files#diff-04cd101c699dfc26787bd687fc50fba2R194

amirfefer · 2018-02-19T13:48:31Z

.../assets/javascripts/react_app/components/PasswordStrength/__tests__/PasswordStrength.test.js

+
+  describe('triggering', () => {
+    const setInputValue = (input, value) => {
+      input.instance().value = value; // eslint-disable-line no-param-reassign


same here, you can make it one line.

ohadlevy · 2018-02-25T09:23:50Z

what is the status of this pr? thanks

sharvit · 2018-02-25T09:29:50Z

@ohadlevy Amazing timing just pushed :)

Added an IntegrationTestHelper.js
The integration test is taking a snapshot of state and action (only state before)

@amirfefer any thoughts?

sharvit · 2018-04-29T16:04:12Z

I have just rebased it, @amirfefer @ohadlevy can you have another look?

amirfefer

Thanks @sharvit
LGTM 👍

tstrachota

I tested this and it works fine. I found one typo and a pf variable nitpick in the code.

I also have one question about userInputs that I'd like to be answered prior to merging this.

tstrachota · 2018-04-27T16:35:03Z

webpack/assets/javascripts/react_app/components/PasswordStrength/PasswordStrength.js

+          <CommonForm
+            label={__('Verify')}
+            touched={true}
+            error={doesPasswordsMatch ? verify.error : __('Password do not match')}


Typo: "Passwords do not match"

tstrachota · 2018-04-30T12:38:28Z

webpack/assets/javascripts/react_app/components/PasswordStrength/PasswordStrength.scss

@@ -1,4 +1,4 @@
-@import '../../../common/colors.scss';
+@import '../../common/colors.scss';


Nitpick: I know that you're just moving the file, but when we're touching it already, could you replace the font families with $font-family-base pf variable, please?

tstrachota · 2018-04-30T13:14:55Z

webpack/assets/javascripts/react_app/components/PasswordStrength/PasswordStrength.js

+    const userInputs =
+      userInputIds && userInputIds.length > 0
+        ? userInputIds.map(input => document.getElementById(input).value)
+        : [];


Just out of curiosity: What's shown in the strength meter when the password contains the username? I tried to simulate that but without any luck.

It should show 'WEAK'. I just tested it and it works.

I see, you're right. I confirm it works.

tstrachota · 2018-04-30T13:19:28Z

.../assets/javascripts/react_app/components/PasswordStrength/__tests__/PasswordStrength.test.js

+      const passwordInput = component.find(`input#${props.data.id}`);
+      setInputValue(passwordInput, 'some-value');
+
+      expect(props.updatePassword.mock.calls).toMatchSnapshot();


Could this be checked with toHaveBeenCalledWith instead? It would make the test more readable.

I personally think the snapshot is more readable because you move the expected data to a different file.

If I would use toHaveBeenCalledWith i would miss other calls. snapshot will snap the whole calls made to the function.

Having the expected data in a different file is exactly the reason why I think it's not readable :) You have to look at one more place to check if the test is correct. I consider the snapshots to be prone to such mistakes in general. In testing visual part of component's it's acceptable, but I'd like to avoid it elsewhere.
I've experienced it couple of times that the PR author just blindly updated tests without actually checking what's in the snapshot.

Anyway, it's not that big issue so I'm not going to block this PR and I'll open a discourse thread on that topic.

sharvit · 2018-04-30T15:54:05Z

Thanks for the review @tstrachota, I just updated.
Can you have another look please?

tstrachota

Snapshots need update after a string change.
Apart from that it's ready to go.

* migrate PasswordStrength to a domain level folder-structure ref #22473

sharvit · 2018-05-08T08:48:17Z

Thanks @tstrachota! updated 👍

tstrachota · 2018-05-09T11:42:34Z

Waiting for code owner review from @theforeman/packaging. @ekohl would you mind taking a look?

ekohl

Apologies for missing this. It's a false positive for packaging since this does touch package.json but not the actual packaging.

tstrachota · 2018-05-16T15:11:01Z

Merged. Thank you @sharvit and everybody involved for such long running efforts!

ares · 2018-05-16T15:58:56Z

Thanks for merging, please don't forget to set the redmine releaas afterwards. I set it to 1.19 now.

theforeman-bot added Needs testing Not yet reviewed labels Feb 7, 2018

sharvit commented Feb 7, 2018

View reviewed changes

priley86 mentioned this pull request Feb 13, 2018

refactor app structure, start clusters actions/reducers ManageIQ/manageiq-v2v#43

Merged

ohadlevy changed the title ~~WIP: Fixes #22473 - Refactor react_app folder structure~~ Fixes #22473 - Refactor react_app folder structure Feb 18, 2018

amirfefer suggested changes Feb 19, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Feb 19, 2018

ohadlevy mentioned this pull request Feb 19, 2018

TextualSummaries: part of client side rendering with React. ManageIQ/manageiq-ui-classic#3420

Merged

sharvit force-pushed the refactor/react-app-folder-structure branch from 007987f to d7cc82b Compare February 25, 2018 09:25

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Feb 25, 2018

sharvit force-pushed the refactor/react-app-folder-structure branch from 7e8300f to 96ab056 Compare April 29, 2018 16:01

amirfefer approved these changes Apr 30, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Apr 30, 2018

tstrachota suggested changes Apr 30, 2018

View reviewed changes

tstrachota added Waiting on contributor and removed Needs testing labels Apr 30, 2018

sharvit force-pushed the refactor/react-app-folder-structure branch from 96ab056 to a4f8806 Compare April 30, 2018 15:53

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Apr 30, 2018

tstrachota suggested changes May 7, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels May 7, 2018

Fixes #22683 - Domain level PasswordStrength entry

8a6aa93

* migrate PasswordStrength to a domain level folder-structure ref #22473

sharvit force-pushed the refactor/react-app-folder-structure branch from a4f8806 to 8a6aa93 Compare May 8, 2018 07:28

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels May 8, 2018

tstrachota approved these changes May 9, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label May 9, 2018

tstrachota added Ready to merge and removed Needs testing labels May 9, 2018

ekohl approved these changes May 16, 2018

View reviewed changes

tstrachota merged commit a671eca into theforeman:develop May 16, 2018

		@@ -0,0 +1,2 @@
		export const doesPasswordsMatch = ({ password, passwordConfirmation }) =>


		import PasswordStrength, { reducers } from '../index';

		const fixtures = {

		@@ -1,4 +1,4 @@
		@import '../../../common/colors.scss';
		@import '../../common/colors.scss';

Fixes #22683 - Refactor PasswordStrength folder structure #5240

Fixes #22683 - Refactor PasswordStrength folder structure #5240

Conversation

sharvit commented Feb 7, 2018 • edited Loading

theforeman-bot commented Feb 7, 2018

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

priley86 commented Feb 7, 2018

sharvit commented Feb 8, 2018

sharvit commented Feb 8, 2018

priley86 commented Feb 8, 2018 • edited Loading

sharvit commented Feb 8, 2018

priley86 commented Feb 8, 2018 • edited Loading

waldenraines commented Feb 9, 2018

sharvit commented Feb 11, 2018

priley86 commented Feb 11, 2018 • edited Loading

priley86 commented Feb 13, 2018 • edited Loading

sharvit commented Feb 13, 2018

amirfefer commented Feb 13, 2018 • edited Loading

ohadlevy commented Feb 18, 2018

amirfefer 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohadlevy commented Feb 25, 2018

sharvit commented Feb 25, 2018

sharvit commented Apr 29, 2018

amirfefer left a comment

Choose a reason for hiding this comment

tstrachota 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

sharvit commented Apr 30, 2018

tstrachota left a comment • edited Loading

Choose a reason for hiding this comment

sharvit commented May 8, 2018

tstrachota commented May 9, 2018

ekohl left a comment

Choose a reason for hiding this comment

tstrachota commented May 16, 2018

ares commented May 16, 2018

sharvit commented Feb 7, 2018 •

edited

Loading

priley86 commented Feb 8, 2018 •

edited

Loading

priley86 commented Feb 8, 2018 •

edited

Loading

priley86 commented Feb 11, 2018 •

edited

Loading

priley86 commented Feb 13, 2018 •

edited

Loading

amirfefer commented Feb 13, 2018 •

edited

Loading

tstrachota left a comment •

edited

Loading