Skip to content

Jcn 408 add trim in data dispatcher#7

Open
juanmusto wants to merge 14 commits into
masterfrom
JCN-408-add-trim-in-data-dispatcher
Open

Jcn 408 add trim in data dispatcher#7
juanmusto wants to merge 14 commits into
masterfrom
JCN-408-add-trim-in-data-dispatcher

Conversation

@juanmusto

Copy link
Copy Markdown

DESCRIPCIÓN DEL REQUERIMIENTO

Se requiere modificar el package para evitar que se guarden espacios intencionales en los datos recibidos.
Se debe modificar la property pristineData, usando el package https://npmjs.com/lodash.clonedeepen vez del {... request.data } actual

DESCRIPCIÓN DE LA SOLUCIÓN
Se agrego la funcion trimObjValues en utils.js que hace un trim de todos los values de la data recibida.
Se agrego el package mencionado anteriorme para usar la funcion de clonedeep para el objeto pristineData

Comment thread lib/utils.js Outdated
Comment thread lib/utils.js
Comment thread lib/dispatcher.js
Comment thread tests/dispatcher-test.js

@fedeatanasoff fedeatanasoff left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no se si ya esta implementado pero en el desarrollo al menos no veo la implementacion de este punto

Considerar que una Api puede recibir un Object o un Object Array

Comment thread lib/dispatcher.js Outdated
constructor(request) {
this._validateRequest(request);

const trimData = trimObjectValues({ ...request.data });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aca un par de cosas:

  • podes usar directamente request.data
  • porque no lo utilizas directamente en las props que queres setear?

@juanmusto juanmusto Oct 19, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fede, aca estoy usando { ...request.data } para no alterar el objeto original que me vino en request.data
en cuanto al segundo comentario que props te referis ?

Esta contemplado pasarle un Array Object

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

que tan chequeado esta que cloneObject te altere la data?
porque sino podrias tirar ahi algo como
this.pristineData = request.data && cloneObj(request.data);

@juanmusto juanmusto Oct 26, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tenes razon, el cloneObject no altera el objeto original.
Ahi agregue el cambio que me comentaste arriba

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hay dos cosas que no me cierran del todo, primero que ...request.data si el objeto tiene muchos niveles a fin de cuenta vas a estar modificando los objetos internos, por otro lado si el objeto es muy picante en niveles capaz la recursiva de trimObjectValues puede ser un problema aunque es la fácil para salir ahora.

@juanmusto juanmusto requested a review from mdanelutti October 31, 2022 18:44

@mdanelutti mdanelutti left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toca revisar la recursiva.

Comment thread lib/utils.js
});

return object;
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

esta recursiva esta mal, porque el return {} adultera datos, ademas en el medio se pierden datos y por ultimo entra en casos donde no es necesario, por ejemplo el el value del k es true entra a la recursiva y eso no esta ok.

@mdanelutti mdanelutti left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unos últimos cambios y estamos

Comment thread lib/utils.js Outdated
'use strict';

const omit = require('lodash.omit');
const cloneObj = require('lodash.clonedeep');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

esto quedo redundante, ya no se usa acá y lo exportas para usarlo en otro archivo, iría derecho a lib/dispatcher.js en la linea 9 con este require

Comment thread lib/dispatcher.js Outdated
this.data = request.data || {};
this.pristineData = request.data && { ...request.data };
this.data = trimObjectValues(request.data && cloneObj(request.data));
this.pristineData = request.data && cloneObj(request.data);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahora que this.data tiene un cloneObj, me suena que this.pristineData no necesita el clone, si el objeto es muy grande estamos metiendo un segundo clone que no necesitamos.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants