Refactor files structure and do some little fixes #6

sandybemonkey · 2018-08-08T09:37:03Z

not really tested, TODO add more tests

rdubigny · 2018-08-09T08:16:00Z

app.js

-import idHintHelper from './helpers/idHintToken';
-import logoutHelper from './helpers/logout';
-import authorizationHelper from './helpers/authorization';
+import utilsHelper from './helpers/utils';
 import getAccessTokenHelper from './helpers/accessToken';

 const app = express();
 const port = process.env.PORT || '3000';


put this line closer to his usage like https://github.com/sandybemonkey/fd_mock/blob/use-csv-database/src/app.js#L29

rdubigny · 2018-08-09T08:16:24Z

app.js

+ * session config
+ * @type {{secret: string, cookie: {}, saveUninitialized: boolean}}
+ */
+const sess = {


sess => session

rdubigny · 2018-08-09T08:17:16Z

app.js

+/**
+ * session config for production
+ */
+if (app.get('env') === 'production') {


are you sure it's needed ? This project intend to be the simpliest as possible. We must keep in mind that PHPers must understand this code

I think that simple enough even me a PHPer can understand that piece of code!!

rdubigny · 2018-08-09T08:44:04Z

package.json

@@ -18,6 +18,7 @@
    "debug": "^3.1.0",
    "ejs": "^2.6.1",
    "express": "^4.16.3",
+    "express-session": "^1.15.6",


we should try to avoid adding external library to keep the codebase as simple as possible. Are you sure this one is necessary ?

session was built in but like body-parser or ejs you now have to add this dependencies separately

rdubigny · 2018-08-09T08:46:03Z

app.js

+  app.set('trust proxy', 1); // trust first proxy
+  sess.cookie.secure = true; // serve secure cookies
+}
+
 if (process.env.NODE_ENV !== 'test') {


I have added comments in https://github.com/sandybemonkey/fd_mock/blob/use-csv-database/src/app.js, it could be a good thing to have the same comments in both files

rdubigny · 2018-08-09T08:54:23Z

package-lock.json

@@ -0,0 +1,5783 @@
+{


this package-lock is not in sync with the package.json: when I run npm i the file is modified. please do the following:

rm -rf node_modules package-lock.json

npm i

git add package-lock.json

git commit -m "update package lock"

git push

You can also read this thread https://stackoverflow.com/questions/45022048/why-does-npm-install-rewrite-package-lock-json

rdubigny · 2018-08-09T08:56:25Z

app.js

-import idHintHelper from './helpers/idHintToken';
-import logoutHelper from './helpers/logout';
-import authorizationHelper from './helpers/authorization';
+import utilsHelper from './helpers/utils';


please use destructured import to avoid too generic naming like "utilsHelper":

import { getAuthorizationUrl } from './helpers/utils';

rdubigny · 2018-08-09T08:57:48Z

app.js

@@ -31,34 +47,40 @@ app.get('/', (req, res) => {
  res.render('pages/index');
 });

+/**


you can use line comment syntax instead of group syntax comment for line comment:

// Init authorization and login process

rdubigny · 2018-08-09T09:00:16Z

app.js

-import idHintHelper from './helpers/idHintToken';
-import logoutHelper from './helpers/logout';
-import authorizationHelper from './helpers/authorization';
+import utilsHelper from './helpers/utils';
 import getAccessTokenHelper from './helpers/accessToken';


please use destructured import to avoid useless variables like "getAccessTokenHelper":

import { getAccessToken } from './helpers/utils';

rdubigny · 2018-08-09T09:49:16Z

helpers/accessToken.js

@@ -5,30 +5,28 @@
 import axios from 'axios';
 import querystring from 'querystring';
 import getUserHelper from './user';
-import idHintToken from './idHintToken';
 import config from '../config/config.json';


Please destructure this import

rdubigny · 2018-08-09T09:50:11Z

app.js

 });

 app.get('/end', (req, res) => {
  // resetting the id token hint.
-  idHintHelper.resetHintToken();
+  req.session.id_token = null;
+  req.session.userInfo = null;
  res.render('pages/end');
 });



please add a message in the console when the server start (see https://github.com/sandybemonkey/fd_mock/blob/use-csv-database/src/app.js#L33)

rdubigny · 2018-08-09T09:53:04Z

helpers/accessToken.js

      /**
       * Use to send the access token to an data provider.
+       * @return an object of data from the provider.


an object of data ?

rdubigny · 2018-08-09T09:53:42Z

helpers/accessToken.js

@@ -5,30 +5,28 @@
 import axios from 'axios';
 import querystring from 'querystring';
 import getUserHelper from './user';


use destructured import

rdubigny · 2018-08-09T09:57:34Z

helpers/accessToken.js

@@ -42,9 +40,11 @@ exports.getAccessToken = async (res, queryCode) => {
  await axios.post(url, querystring.stringify(body), headerConfig)


querystring.stringify => To my opinion this is typically the kind of library we should avoid. A simple reduce in native javascript can do the job.

you should use the params option and remove this lib.

See axios/axios#739 (comment).

Try with params but use querystring as the documention said.

rdubigny · 2018-08-09T10:01:14Z

helpers/user.js

-exports.getUser = async (token, res) => {
-  if (!token) res.sendStatus(401);
+exports.getUser = async (req, res) => {
+  if (!req.accessToken) res.status(401).send('Access token is required');


you should return here, to stop the execution of the function here

rdubigny · 2018-08-09T10:04:03Z

test/helpers/utils.spec.js

+
+describe('helpers/authorization', () => {
+  it('should return a correct authorization url to call the API "/api/v1/authorize"', () => {
+    // Setup


I rather remove this comments as we won't repeat this on every tests made since it will weighten the reading without adding enough useful information

rdubigny · 2018-08-09T10:04:34Z

views/pages/end.ejs

@@ -6,7 +6,7 @@
                Guichet des Chiroquois
            </h1>
            <div class="subtitle">
-                <h4>Merci de votre visite, afin que votre deconnection soit totale, veuillez fermer votre navigateur.</h4>
+                <h4>Merci de votre visite, veuillez fermer votre navigateur afin que la deconnection soit complète.</h4>


why do the user has to close is browser ?

rdubigny · 2018-08-09T10:05:46Z

views/pages/profile.ejs

@@ -8,20 +8,15 @@
            <div class="subtitle">
                <div class="card">
                    <div class="card-content">


can you factorise the page structure with ejs inheritance

rdubigny · 2018-08-09T10:08:07Z

@sandybemonkey do you plan to have an offline mode like db ?

rdubigny · 2018-08-09T10:10:07Z

views/pages/end.ejs

@@ -6,7 +6,7 @@
                Guichet des Chiroquois
            </h1>
            <div class="subtitle">
-                <h4>Merci de votre visite, afin que votre deconnection soit totale, veuillez fermer votre navigateur.</h4>
+                <h4>Merci de votre visite, veuillez fermer votre navigateur afin que la deconnection soit complète.</h4>


end.ejs => logged-out.ejs

plus, can you remove the empty login.ejs file ?

rdubigny · 2018-08-09T10:16:57Z

helpers/accessToken.js

-  const clientId = config.CLIENT_SECRET;
-  const secretKey = config.SECRET_KEY;
-
+exports.getAccessToken = async (res, req) => {


I made a comment about using the export keyword instead of this #5 (comment) . Did you see it ?

rdubigny · 2018-08-09T10:18:04Z

helpers/accessToken.js

-  const clientId = config.CLIENT_SECRET;
-  const secretKey = config.SECRET_KEY;
-
+exports.getAccessToken = async (res, req) => {


function with req & res as parameters = controller (or middleware)

=> plz move this file out of the helpers folder

rdubigny · 2018-08-09T10:19:00Z

helpers/accessToken.js

@@ -42,9 +40,11 @@ exports.getAccessToken = async (res, queryCode) => {
  await axios.post(url, querystring.stringify(body), headerConfig)
    .then(response => response.data)
    .then((tokenData) => {
-      idHintToken.setHintToken(tokenData.id_token);
+      req.accessToken = tokenData.access_token;
+      req.session.id_token = tokenData.id_token;
      /**


plz do not use jsdoc inside functions

rdubigny · 2018-08-09T10:21:49Z

helpers/accessToken.js

+const tokenUrl = config.TOKEN_URL;
+const redirectUrl = config.REDIRECT_URL;
+const clientId = config.CLIENT_SECRET;
+const secretKey = config.SECRET_KEY;

 /**
 * Init FranceConnect authentication login process.
 * Make every http call to the different API endpoints.
 * @see @link{ https://partenaires.franceconnect.gouv.fr/fcp/fournisseur-service# }


plz mention this link in only one comment in this file

Refactor files structure and do some little fixes

4d30b6d

sandybemonkey requested a review from rdubigny August 8, 2018 09:37

SandyCT97 added 4 commits August 8, 2018 12:12

Add doc in app.js

2d75c8b

Rename a test file, add a test and some fixes

6d537ef

Fix wrong source for session.userInfo and test

57d76bc

Fix linting errors

677d01e

rdubigny reviewed Aug 9, 2018

View reviewed changes

rdubigny requested changes Aug 9, 2018

View reviewed changes

SandyCT97 added 4 commits August 9, 2018 13:53

Fix first pull request comments

92c0002

Fixes based on pull request comments

7be8f2c

Connect FS to FD mock

6d832ad

Connect FS to FD mock

eee4e3f

sandybemonkey merged commit 4e36c05 into master Aug 10, 2018

sandybemonkey deleted the CLEAN_CODE_AND_REPO branch August 10, 2018 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor files structure and do some little fixes #6

Refactor files structure and do some little fixes #6

sandybemonkey commented Aug 8, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

sandybemonkey Aug 9, 2018

rdubigny Aug 9, 2018

sandybemonkey Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

sandybemonkey Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny commented Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

rdubigny Aug 9, 2018

		@@ -42,9 +40,11 @@ exports.getAccessToken = async (res, queryCode) => {
		await axios.post(url, querystring.stringify(body), headerConfig)

Refactor files structure and do some little fixes #6

Refactor files structure and do some little fixes #6

Conversation

sandybemonkey commented Aug 8, 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

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

rdubigny commented Aug 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment