Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

se agrego el metodo downloadFile #8

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

WilliamSaya-lvl30
Copy link

@WilliamSaya-lvl30 WilliamSaya-lvl30 commented Apr 9, 2024

LINK DE TICKET:
https://janiscommerce.atlassian.net/browse/APPSRN-291

DESCRIPCIÓN DEL REQUERIMIENTO:

CONTEXTO

Se ha identificado que varias funcionalidades de las apps de janis, requieren descargar archivos desde diferentes fuentes externas. Actualmente, esta funcionalidad se implementa repetidamente en cada uno de los repositorios de las aplicaciones, lo que resulta en una duplicación significativa de código y aumenta la complejidad del mantenimiento.

NECESIDAD

Para mejorar la eficiencia del desarrollo y facilitar el mantenimiento a largo plazo de las aplicaciones, es fundamental centralizar la lógica para descargar archivos en un único lugar. Al implementar un nuevo método llamado downloadFile en el paquete @janiscommerce/app-request, podemos consolidar esta funcionalidad común en una única función reutilizable. Esto no solo reduce la duplicación de código en los diferentes repositorios de las aplicaciones, sino que también nos permite actualizar y mejorar la lógica de descarga de archivos de manera centralizada. Además, al utilizar un paquete compartido, podemos garantizar la coherencia en la implementación de la descarga de archivos en todas las aplicaciones de Janis. Esto proporciona un enfoque más eficiente y sostenible para el desarrollo de nuevas características y la resolución de problemas relacionados con la descarga de archivos en el futuro.

Acceptance criteria

  1. El método downloadFile debe ser implementado en el paquete @janiscommerce/app-request.
  2. Debe aceptar parámetros para la URL del archivo a descargar y la ubicación de destino en el dispositivo (path).
  3. El método debe utilizar npm: react-native-fs para realizar la descarga del archivo.
  4. En caso de errores durante la descarga, el error debe ser registrado en Crashlytics.
  5. El método debe retornar toda la información posible al completarse la descarga.
  6. Se debe proporcionar una documentación clara y ejemplos de uso para el método downloadFile.
  7. Se deben agregar pruebas unitarias para garantizar el funcionamiento correcto del método.

DESCRIPCIÓN DE LA SOLUCIÓN:
Se utiliza la libreria para realizar las descargas react-native-fs
Se agrego como peerDependencies junto a react-native.

Se creo el metodo downloadFile que recibe como parámetros obligatorios fileName, folderPath y url, este metodo se encargara de hacer la descarga solamente y retornar la respuesta del package.

Se agrego la función isFunction en helpers.

CÓMO SE PUEDE PROBAR?

Para linkear el componente con un proyecto, seguir la siguiente documentación
https://fizzmod.atlassian.net/wiki/spaces/JAPP/pages/2341765125/C+mo+trabajo+con+el+package+UI

OJO

Tener en cuenta que la respuesta del request.get cambia en la ultima versión, así que hay que en las apps que no tengan ese cambio implementado va a romper en todos los metodos GET. Ajustas estos metodos para las pruebas.

Para probar se puede importar en la Home la instancia de reques y el package RNFS
Ingresar este codigo en algun useeffect de la home.

import RNFS from 'react-native-fs';
import Request from 'src/utils/api/requestInstance';
await RNFS.mkdir(`${RNFS.DocumentDirectoryPath}/pdfs`);
const [downloadPDFRes, downloadPDFErr] = await promiseWrapper(
	Request.downloadAndFile({
		url: `https://www.w3.org/WAI/ER/tests/xhtml/testfiles/resources/pdf/dummy.pdf`,
	        fileName: `descarga.pdf`,
		folderPath: `${RNFS.DocumentDirectoryPath}/pdfs`,
	})

console.log('downloadPDFRes :', downloadPDFRes);

En el console.log podremos ver una rspuesta como esta, corroborando la cantidad de bytes descargados

type DownloadResult = {
  jobId: number;          // The download job ID, required if one wishes to cancel the download. See 'stopDownload'.
  statusCode: number;     // The HTTP status code
  bytesWritten: number;   // The number of bytes written to the file
};

CHANGELOG

### Feature

- [APPSRN-291](https://janiscommerce.atlassian.net/browse/APPSRN-291)
- Added downloadFile methods
- Added react-native-fs peerDependencies
- Added isFunction helper 

Copy link

@GonzaFran GonzaFran left a comment

Choose a reason for hiding this comment

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

@WilliamSaya-lvl30 se dejan comentarios sobre las validaciones

lib/request.js Outdated
if (!url || !isString(url)) throw new Error('url is not valid');

const validHeaders = headers && isObject(headers);
const validBegin = progress && isFunction(begin);

Choose a reason for hiding this comment

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

@WilliamSaya-lvl30 revisate que acá estas validando progress cuando debería ser begin && isFunction(begin)

RNFS.downloadFile({
fromUrl: url,
toFile: `${folderPath}/${fileName}`,
...(validHeaders && {headers}),

Choose a reason for hiding this comment

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

@WilliamSaya-lvl30 acá no se tendrían que usar los mismos headers que en las demás request?

Copy link
Author

Choose a reason for hiding this comment

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

@GonzaFran para lo que se utiliza no suele requerir headers, se agrego esa validación solo para cubrir hipoteticos casos futuros.

lib/request.js Outdated
!isObject(downloadRes) ||
!downloadRes ||
downloadRes.bytesWritten === 0 ||
downloadRes.statusCode === 403

Choose a reason for hiding this comment

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

@WilliamSaya-lvl30 esta validación no convendría que verfique si el statuscode es diferente a 200?

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.

3 participants