Add facebook login tests #101

miporto · 2017-05-03T04:38:33Z

Closes #98

The returned user schema should be properly checked before being inserted into the database

codecov · 2017-05-03T04:40:18Z

Codecov Report

Merging #101 into fb-integration will increase coverage by 4.97%.
The diff coverage is 90.47%.

@@                Coverage Diff                 @@
##           fb-integration     #101      +/-   ##
==================================================
+ Coverage           86.95%   91.93%   +4.97%     
==================================================
  Files                  21       21              
  Lines                 460      471      +11     
  Branches               19       20       +1     
==================================================
+ Hits                  400      433      +33     
+ Misses                 60       38      -22

Impacted Files	Coverage Δ
src/database/migrations/migrations.js	`100% <100%> (ø)`	⬆️
src/middlewares/login-router.js	`92.85% <80%> (+12.85%)`	⬆️
src/middlewares/facebook.js	`97.14% <93.33%> (+60.1%)`	⬆️
src/handlers/response.js	`92.13% <0%> (+3.37%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 910ef5f...a06d13f. Read the comment docs.

aibarbetta · 2017-05-03T04:57:12Z

src/middlewares/facebook.js

@@ -48,7 +48,7 @@ const handleLogin = (req, res, next, fUser) => {
        firstName: fUser.name,
        lastName: fUser.name,
        email: fUser.email,
-        country: fUser.location.name,
+        country: (fUser.hasOwnProperty('location')) ? fUser.location.name : '',


Le pondria 'unknown', para que se entienda si nos encontramos con el country vacio en el futuro. Seguro que esto mismo no puede pasar con todos los otros datos que pedimos al graph?

Revise y el test user de la api de facebook no esta devolviendo el mail solamente.

Salvo la ubicación el resto de los campos deberían llegar siempre como respuesta al request. De todas formas se podría agregar un valor default a los campos que no sean obligatorios (por si la información no llega a estar).

Segun la doc todos son obligatorios, pero si el usuario en facebook no dio su pais, calculo que tambien podria no haber dado su fecha de nacimiento, no? Habria que llenar todos esos con un valor por default.
Otra, si el test user no te devuelve mail no deberia romper? No estaria cumpliendo con el schema de alta de usuario

Ahora mismo no se hace ningún chequeo del user que devuelve Facebook. Puedo agregarlo aca pero se romperían los tests de Facebook hasta que resuelva como hacer para que el test user devuelva los campos que están faltando.

No se como funcionan esos test users pero, de este lado, no podemos poner un default si fb no te puede proveer ese dato? Y esto aplicaria a todos los datos, no solo pais

Si, podriamos chequear contra un schema para corroborar que tenga los datos que determinemos como obligatorios y despues revisar el resto y en el caso de que no esten ponerles el valor default que corresponda.

Implementado en 5416dbf

aibarbetta · 2017-05-03T05:22:36Z

src/test/routes/token.spec.js

+        });
+    });
+
+    it('should return the expected body response when correct credentials are sent', (done) => {


Estos dos tests felices de fb caen en la alta de un nuevo usuario, no? Se puede agregar el pedido de un token por fb de un usuario que ya exista?

Implementado en 98a57bf

Test login when using facebook token of an existing user in the database

miporto · 2017-05-03T06:04:52Z

Los tests fallaron porque los tokens caducaron. Pense que no vencían los de los test user, tengo que averiguar como hacer para que no expiren.

* Generate user for db in specific function. * Update tokens

* Make username lowercase * Set last name as last element in array of names

aibarbetta · 2017-05-03T13:40:50Z

src/middlewares/facebook.js

+        userName,
+        facebookUserId: user.id,
+        facebookAuthToken: user.authToken,
+        birthdate: user.birthday,


El cumpleaños va a venir siempre si o si? No puede pasar que alguno no lo complete?

En teoría la aplicación pide ese permiso (que por default no viene) y cuando te creas una cuenta en Facebook lo tenes que definir si o si.
Mas que nada por eso lo agregue, como lo pedimos y es obligatorio especificarlo en Facebook debería estar siempre este campo.

No veo la diferencia de cumpleaños con los otros campos, todos son obligatorios, mail es mas vital que cumpleaños y lo estamos dejando pasar como unknown por ejemplo

aibarbetta · 2017-05-03T13:44:08Z

src/middlewares/facebook.js

-          logger.warn(`FB GRAPH RESPONSE ${JSON.stringify(response)}`);
-          reject(error);
+          const errMsg = JSON.parse(response.body).error.message;
+          logger.warn(`FB GRAPH RESPONSE ${errMsg}`);


Alindemos el mensaje del logger si se van a quedar estos prints 😁

aibarbetta · 2017-05-03T13:46:38Z

src/middlewares/facebook.js

-        respond.internalServerError(error, res);
-      });
+      fUser.authToken = req.body.authToken;
+      db.general.createNewEntry(tables.users, createDbUserObject(fUser))


Yo meteria estas dos lineas en la función auxiliar también, que sea un "createFacebookUser" y listo

aibarbetta · 2017-05-03T13:50:15Z

src/middlewares/login-router.js

-    .then(fUser => facebook.handleLogin(req, res, next, fUser))
-    .catch(error => respond.internalServerError(error, res));
+    .then(fUser => {
+      respond.validateRequestBody(fUser, facebookUser)


Por que validamos que venga si o si con cumpleaños aca pero los demas campos los dejamos como potenciales unknown?

pd hay que cambiarle el nombre a esta funcion y sacarla de respond, ya no es un validateRequestBody ni pertenece al handler

Por que validamos que venga si o si con cumpleaños aca pero los demas campos los dejamos como potenciales unknown?

En realidad deberían ser obligatorios también los campos first_name, last_name y email porque los pedimos y son obligatorios para crear una cuenta en Facebook (al igual que el cumpleaños). Mi idea es cambiarlo en cuanto logre que los test users den esta informacion (o actualice los test cases para usar un usuario real que si los da).

pd hay que cambiarle el nombre a esta funcion y sacarla de respond, ya no es un validateRequestBody ni pertenece al handler

Sip, pero abriría un issue y lo cambiaría en una branch nueva. Este cambio impacta en muchos archivos y se desvirtúa este PR.

En realidad deberían ser obligatorios también los campos first_name, last_name y email porque los pedimos y son obligatorios para crear una cuenta en Facebook (al igual que el cumpleaños). Mi idea es cambiarlo en cuanto logre que los test users den esta informacion (o actualice los test cases para usar un usuario real que si los da).

De nuevo, no se como funcionan los test users, pero si no se les puede setear los datos que queremos que devuelvan decime como y te doy un token de mi cuenta.
Hay otra cosa igual, si un test user de facebook puede no tener esos campos, no significa que cualquier user podria no tenerlos y hay que contemplar que hacemos ahi? O los ponemos todos en el schema, y si no tiene alguno lo mandamos a completar su perfil de fb, o dejamos pasar a todos como unknown.

Sip, pero abriría un issue y lo cambiaría en una branch nueva. Este cambio impacta en muchos archivos y se desvirtúa este PR.

Abrilo, mi miedo es que se quede como un minor olvidado hasta el final 😣

aibarbetta · 2017-05-03T14:27:24Z

src/middlewares/facebook.js

+const createDbUserObject = (user) => {
+  const names = user.name.split(' ');
+  const defaultMissingValue = 'unknown';
+  const userName = names.join('_').toLowerCase();


Guarda con #52 despues de esto, los nombres de fb no son unicos

* Encapsulate facebook user creation in it's own function * Update tokens

* Change tokens for ones that comply with new mandatory attributes. * Only use default value with country

aibarbetta

Abrir el issue del wrapper de amanda y mergear 🎉

miporto added 4 commits May 3, 2017 01:35

Store Facebook auth token as string

407ba39

Only ask for location name if it exists

efdcb4c

The returned user schema should be properly checked before being inserted into the database

Add facebok test token constants

8991460

Add tests with facebook login

5950952

miporto changed the base branch from develop to fb-integration May 3, 2017 04:42

Merge branch 'fb-integration' into SS-98

52d6875

miporto requested a review from aibarbetta May 3, 2017 04:46

aibarbetta reviewed May 3, 2017

View reviewed changes

Add initiala facebook user

98a57bf

Test login when using facebook token of an existing user in the database

miporto added 4 commits May 3, 2017 09:49

Udate error code and send error message in reject

e4592a1

* Generate user for db in specific function. * Update tokens

Validate user returned by Fb api

5ee16e8

Set default value for missing attributes

7f1bfee

Change default values

5416dbf

* Make username lowercase * Set last name as last element in array of names

aibarbetta reviewed May 3, 2017

View reviewed changes

miporto added 2 commits May 3, 2017 12:18

Add fields to facebook request

7629357

* Encapsulate facebook user creation in it's own function * Update tokens

Change mandatory attributes of fb user

a06d13f

* Change tokens for ones that comply with new mandatory attributes. * Only use default value with country

aibarbetta approved these changes May 3, 2017

View reviewed changes

miporto merged commit 6e99c15 into fb-integration May 3, 2017

miporto deleted the SS-98 branch May 3, 2017 17:48

miporto mentioned this pull request May 4, 2017

Tests for facebook login #98

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add facebook login tests #101

Add facebook login tests #101

miporto commented May 3, 2017

codecov bot commented May 3, 2017 •

edited

Loading

aibarbetta May 3, 2017

miporto May 3, 2017

aibarbetta May 3, 2017

miporto May 3, 2017

aibarbetta May 3, 2017

miporto May 3, 2017

miporto May 3, 2017

aibarbetta May 3, 2017

miporto May 3, 2017

miporto commented May 3, 2017

aibarbetta May 3, 2017

miporto May 3, 2017

aibarbetta May 3, 2017

aibarbetta May 3, 2017

aibarbetta May 3, 2017

aibarbetta May 3, 2017

aibarbetta May 3, 2017

miporto May 3, 2017

aibarbetta May 3, 2017

aibarbetta May 3, 2017

aibarbetta left a comment

Add facebook login tests #101

Add facebook login tests #101

Conversation

miporto commented May 3, 2017

codecov bot commented May 3, 2017 • edited Loading

Codecov Report

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

miporto commented May 3, 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

aibarbetta left a comment

Choose a reason for hiding this comment

codecov bot commented May 3, 2017 •

edited

Loading