[#436] [FEATURE] Récupération du niveau par compétence pour un utilisateur via une api sécurisée (US-527) #436

florianEnoh · 2017-06-06T13:48:28Z

Création du endpoint : /api/users
Récupération toutes les compétences par domaines via Airtable

coveralls · 2017-06-09T14:24:26Z

Coverage increased (+0.08%) to 94.672% when pulling ce771eb on get-user-level-by-skills into bb25b8e on dev.

coveralls · 2017-06-09T14:42:33Z

Coverage increased (+0.08%) to 94.672% when pulling ce771eb on get-user-level-by-skills into bb25b8e on dev.

coveralls · 2017-06-09T15:15:21Z

Coverage increased (+0.08%) to 94.576% when pulling 256fe94 on get-user-level-by-skills into 53ae5a7 on dev.

coveralls · 2017-06-12T08:29:45Z

Coverage increased (+0.08%) to 94.576% when pulling f38807b on get-user-level-by-skills into 53ae5a7 on dev.

kemenaran

Sinon LGTM

kemenaran · 2017-06-12T08:37:46Z

api/lib/application/users/user-controller.js

+      .verify(token)
+      .then((user_id) => {
+        return UserRepository.findUserById(user_id)
+          .then((foundedUser) => {


La beauté des promesses, c'est que normalement ici tu n'es pas obligé d'imbriquer les then, tu peux juste chaîner les promesses :

return authorizationToken .verify(token) .then(user_id => UserRepository.findUserById(user_id)) .then(foundedUser => reply(userSerializer.serialize(foundedUser)).code(201)) .catch((err) => { // … });

Yes well seen

kemenaran · 2017-06-12T08:37:52Z

api/lib/application/users/user-controller.js

+    const token = request.headers.authorization;
+    return authorizationToken
+      .verify(token)
+      .then((user_id) => {


Yes, mais dans la PR de la connexion, c'est la clé user_id qui utilisée pour la signature.
Je l'ai laissé ainsi en attendant

kemenaran · 2017-06-12T08:39:39Z

api/lib/application/users/user-controller.js

+        if(err instanceof NotFoundError) {
+          err = 'Cet utilisateur est introuvable';
+        } else {
+          err = 'Le token n\'est pas valid';


Parfois, pour éviter le \' disgracieux, je mets une apostrophe courbe : err = 'Le token n’est pas valide';.

kemenaran · 2017-06-12T08:41:59Z

api/lib/application/users/user-controller.js

+          err = 'Cet utilisateur est introuvable';
+        } else {
+          err = 'Le token n\'est pas valid';
+        }


Ça me semble un peu confondant que l'erreur par défaut soit "Le token n'est pas valide". Il n'y aurait pas moyen de détecter cette erreur là spécifiquement, pour faciliter le déboguage ? Un truc du genre :

if(err instanceof NotFoundError) { err = 'Cet utilisateur est introuvable'; } else if (/* err.code = token_pas_valide, qqc comme ça */) { err = 'Le token n’est pas valide'; } else { err = 'Une erreur est survenue lors de l’authentification de l’utilisateur'; }

…nstead of false

coveralls · 2017-06-12T10:32:19Z

Coverage increased (+0.07%) to 94.559% when pulling ab0c50a on get-user-level-by-skills into 53ae5a7 on dev.

coveralls · 2017-06-28T14:49:28Z

Coverage increased (+0.3%) to 94.807% when pulling dc24b31 on get-user-level-by-skills into 545f92f on dev.

jbuget · 2017-06-28T14:50:43Z

I've deployed this PR to http://get-user-level-by-skills.pix.beta.gouv.fr. Please check it out

Akhilian · 2017-06-29T08:19:40Z

api/lib/application/tools/index.js

@@ -0,0 +1,18 @@
+const ToolsController = require('./tools-controller');


On commence à avoir plusieurs endroits pour rafraichir le cache.
Proposition:

On renomme en CacheController => /api/cache/all

On deplace la purge de cache POST /api/courses/{id} vers ce controller

DELETE est ok. Je crois qu'on peux utiliser PURGE également.

pour la remarque 2) nous avons créée une issue #452

Akhilian · 2017-06-29T08:23:21Z

api/tests/unit/application/tools/index_test.js

+    });
+
+    it('should exist', (done) => {
+      return expectRouteToExist({method: 'DELETE', url: '/api/tools/cache'}, done);


Pas forcément nécessaire sortir une function.
On pourra aussi utiliser injectThen() et se passer du done

Akhilian · 2017-06-29T08:38:16Z

api/tests/unit/application/tools/tools-controller_test.js

+
+    describe('Success cases', () => {
+
+      it('should delete cache entry with key provided', () => {


Le nom ne match pas avec ce qu'on test vraiment.
Je m'attendais à un truc du genre.
sinon.assert.callWith(cache.del, 'test-cache-key');

Pour le moment, je peux modifier request.payload['cache-key'] dans removeEntry par TOTO sans aucun test rouge.

Akhilian · 2017-06-29T08:41:07Z

api/tests/unit/application/tools/tools-controller_test.js

+
+      it('should delete cache entry with key provided', () => {
+        // Given
+        cache.del.returns(1);


P-e extraire une variable pour expliquer le 1 (ex: countOfDeletedEntries = 1).

Akhilian · 2017-06-29T08:41:33Z

api/tests/unit/application/tools/tools-controller_test.js

+
+      it('should reply with Error, when cache key is not found', () => {
+        // Given
+        cache.del.returns(0);


P-e extraire une variable pour expliquer le 0 (ex: noDeletedEntries = 0).

Akhilian · 2017-06-29T09:40:27Z

api/lib/settings.js

@@ -50,7 +51,7 @@ module.exports = (function() {
      secret: 'test-recaptcha-key'
    };

-    config.authentication.secret = 'shhhhhhhhh';
+    config.authentication.secret = 'test-jwt-key';


On a 2 façons de surcharger les configurations pour les tests, ça serait cool d'harmoniser.

Akhilian · 2017-06-29T09:53:45Z

api/tests/acceptance/application/users-controller-get-profile_test.js

+        });
+      });
+
+      it('should return 401  HTTP status code, when authorization is valid but user not found', () => {


Est-ce qu'on ne devrait pas retourner 404 quand l'authentification est réussi (= token valid et bien formé) mais que la ressource n'existe pas ?

Akhilian · 2017-06-29T10:00:11Z

api/tests/acceptance/application/users-controller-get-profile_test.js

+
+      it('should return 401  HTTP status code, when authorization is valid but error occurred', () => {
+        authorizationToken.verify.resolves(4);
+        UserRepository.findUserById.returns(Promise.reject(new Error()));


Ici, on devrait renvoyer une erreur 5xx (500 ou autre) parce que l'erreur vient du serveur.

Akhilian · 2017-06-29T10:01:47Z

api/tests/unit/domain/models/profile_test.js

+          areaId: 'recvoGdo0z0z0pXWZ'
+        }];
+
+      const expectedCompetences = [


Expected en dernier ?

Voir dans le then

Akhilian · 2017-06-29T10:07:42Z

api/tests/unit/infrastructure/repositories/competence-repository_test.js

+    }];
+
+  beforeEach(function() {
+    cache.flushAll();


Ici, on flush le cache avant chaque test et après, on stub.
Une seule façon de faire ?

On peux éventuellement faire un spy() si on veux

Yes c'est bon

coveralls · 2017-06-29T16:31:27Z

Coverage increased (+0.3%) to 94.778% when pulling ff2c09e on get-user-level-by-skills into 545f92f on dev.

jbuget · 2017-06-29T16:33:48Z

I've deployed this PR to http://get-user-level-by-skills.pix.beta.gouv.fr. Please check it out

jbuget · 2017-07-06T08:48:59Z

ETQ usager, JV connaitre mon niveau par compétence via une API sécurisée

jbuget · 2017-07-06T08:49:00Z

ETQ usager, JV connaitre mon niveau par compétence via une API sécurisée

Add endpoint GET /api/users

47d2516

florianEnoh added the Development in progress label Jun 6, 2017

hypernikao added 12 commits June 8, 2017 12:29

Add setting keys

c036cd8

Add single test for endpoint

e2a0432

Amend endpoint handler

23b50d3

Add error cases test

1915da9

Add jsonwebtoken verifier service

13ce22e

Acceptance test in progress

515f93d

Add user repository

dc519b7

Add rules for invalid id

40b0cb4

Add test case when token is valid but not founded user

e8d27bb

Fix lint errors

8bffb28

ADD jsonwebtoken module

2ad9d07

try to fix config auth key

ce771eb

Merge branch 'dev' into get-user-level-by-skills

256fe94

florianEnoh changed the title ~~[#] [FEATURE] Récupération du niveau par compétence pour un utilisateur via une api sécurisée~~ [#436] [FEATURE] Récupération du niveau par compétence pour un utilisateur via une api sécurisée Jun 12, 2017

florianEnoh changed the title ~~[#436] [FEATURE] Récupération du niveau par compétence pour un utilisateur via une api sécurisée~~ [#436] [FEATURE] Récupération du niveau par compétence pour un utilisateur via une api sécurisée (US-527) Jun 12, 2017

extract magic number (for rebuild)

f38807b

kemenaran reviewed Jun 12, 2017

View reviewed changes

hypernikao added 6 commits June 12, 2017 11:25

Add checking key Bearer

d697e27

Improve thenable on controller

4b86c53

Add new Error InvalidTokenError

cd7171e

Refactoring of jwt-verify to reject promise with an Technical Error i…

ce670d5

…nstead of false

Add tech error to controller

9c1e672

Add Apostrophe

ab0c50a

Akhilian reviewed Jun 29, 2017

View reviewed changes

Akhilian added [Tech] Changes required and removed [Tech] Review asked labels Jun 29, 2017

hypernikao added 11 commits June 29, 2017 14:40

Rename toolsController into CacheController

0edbb20

Simplify user-controller error

9c1d123

Rename buildProfile by getByUserId

efa4e1a

Remove useless intermediate variable

06b47ef

Uniformize the config file for API

9e18544

Fix status code on user-controller-profile

c9ac378

Remove useless function

984429d

Remove magic numbers on cache-controller_test

c67829b

Add default description to profile_test

1959dca

Sanitize competence-repository_test

34a05ac

Reformat code in profile-serializer_test

dd0f7b0

florianEnoh added [Tech] Review asked and removed [Tech] Changes required labels Jun 29, 2017

hypernikao and others added 2 commits June 29, 2017 16:53

Fix lint error

a515de2

Fix build

ff2c09e

jbuget merged commit 14b14c4 into dev Jun 29, 2017

jbuget deleted the get-user-level-by-skills branch June 29, 2017 16:34

jbuget added [Tech] Changes validated and removed [Tech] Review asked labels Jun 29, 2017

jbuget modified the milestone: 1.14.0 Jun 29, 2017

		@@ -0,0 +1,18 @@
		const ToolsController = require('./tools-controller');


		describe('Success cases', () => {

		it('should delete cache entry with key provided', () => {

[#436] [FEATURE] Récupération du niveau par compétence pour un utilisateur via une api sécurisée (US-527) #436

[#436] [FEATURE] Récupération du niveau par compétence pour un utilisateur via une api sécurisée (US-527) #436

Conversation

florianEnoh commented Jun 6, 2017

coveralls commented Jun 9, 2017

coveralls commented Jun 9, 2017

coveralls commented Jun 9, 2017

coveralls commented Jun 12, 2017

kemenaran 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

coveralls commented Jun 12, 2017

coveralls commented Jun 28, 2017

jbuget commented Jun 28, 2017

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

coveralls commented Jun 29, 2017

jbuget commented Jun 29, 2017

jbuget commented Jul 6, 2017

jbuget commented Jul 6, 2017