doofinder / doofinder-magento2

Open Software License 3.0
10 stars 8 forks source link

PLUGINRANGERS-316 | Added a endpoint to replace Doofinder script with single script #323

Closed davidmolinacano closed 1 month ago

davidmolinacano commented 1 month ago

Required by:

magento-replacement-single-script

sofia-doofinder commented 1 month ago

@davidmolinacano ahora mismo se utiliza esta función a la hora de cargar la capa para poner la moneda y el idioma:

includeLocaleAndCurrency

eso ya no se utilizaría? deberíamos quitarlo?

davidmolinacano commented 1 month ago

@davidmolinacano ahora mismo se utiliza esta función a la hora de cargar la capa para poner la moneda y el idioma:

includeLocaleAndCurrency

eso ya no se utilizaría? deberíamos quitarlo?

Una vez que esté todo migrado no se usaría, pero de momento creo que sí se está usando.

sofia-doofinder commented 1 month ago

@davidmolinacano ahora mismo se utiliza esta función a la hora de cargar la capa para poner la moneda y el idioma:

includeLocaleAndCurrency

eso ya no se utilizaría? deberíamos quitarlo?

Una vez que esté todo migrado no se usaría, pero de momento creo que sí se está usando.

Tenemos que dejarlo para retrocompatibilidad no? si puedes añade un comentario en la función para que sepamos por qué está eso. Y entiendo que para los scripts nuevos no interferiría, no?

davidmolinacano commented 1 month ago

@davidmolinacano ahora mismo se utiliza esta función a la hora de cargar la capa para poner la moneda y el idioma:

includeLocaleAndCurrency

eso ya no se utilizaría? deberíamos quitarlo?

Una vez que esté todo migrado no se usaría, pero de momento creo que sí se está usando.

Tenemos que dejarlo para retrocompatibilidad no? si puedes añade un comentario en la función para que sepamos por qué está eso. Y entiendo que para los scripts nuevos no interferiría, no?

¡Añadido! De todos modos no he entendido bien la pregunta y es posible que haya entendido mal el propósito de esta tarea. Por asegurarme: el único objetivo es tener un endpoint que reemplace un script por otro cuando se llame, no? No he entendido lo de los "scripts nuevos".

sofia-doofinder commented 1 month ago

@davidmolinacano ahora mismo se utiliza esta función a la hora de cargar la capa para poner la moneda y el idioma:

includeLocaleAndCurrency

eso ya no se utilizaría? deberíamos quitarlo?

Una vez que esté todo migrado no se usaría, pero de momento creo que sí se está usando.

Tenemos que dejarlo para retrocompatibilidad no? si puedes añade un comentario en la función para que sepamos por qué está eso. Y entiendo que para los scripts nuevos no interferiría, no?

¡Añadido! De todos modos no he entendido bien la pregunta y es posible que haya entendido mal el propósito de esta tarea. Por asegurarme: el único objetivo es tener un endpoint que reemplace un script por otro cuando se llame, no? No he entendido lo de los "scripts nuevos".

Me refiero a que una vez que se reemplace el script por el script único y se cargue la capa en la web del cliente se va a seguir llamando a la función includeLocaleAndCurrency, pero entiendo que esta función no haría nada porque no va a hacer pattern matching con los casos que tiene, no? solo por confirmar que no va a intentar meter "código malo" en el script único. La prueba sería ver el código que se inyecta en la web del cliente una vez se reemplaza el script utilizando el endpoint que has hecho.

davidmolinacano commented 1 month ago

Pues por suerte no afecta en absoluto :smile:

Antes:

image

Después:

image

davidmolinacano commented 1 month ago

Ahora bien, lo que he visto es que para que se muestre hay que borrar la caché de Magento usando bin/magento cache:clean

sofia-doofinder commented 1 month ago

Ahora bien, lo que he visto es que para que se muestre hay que borrar la caché de Magento usando bin/magento cache:clean

bien visto! Si puedes comentaselo a producto para que tengan conocimiento de esto y por si lo tienen que poner en alguna página de soporte o algo por el estilo. Porque nosotros desde el plugin creo que no podemos limpiar esa caché no? (bueno y aunque pudiéramos no creo que fuera buena idea...)

davidmolinacano commented 1 month ago

Ahhhh pues justo estaba programando un eliminador de Full Page Cache (solo de esta para evitar problemas)

davidmolinacano commented 1 month ago

En este commit está incluido: https://github.com/doofinder/doofinder-magento2/pull/323/commits/2fc84cbb981ca065a3c057c28d05150fe9786e76

sofia-doofinder commented 1 month ago

Cuando he leído la tarea me imaginaba que al implementar un endpoint sería con el objetivo de que desde Doofinder les pasáramos ya el script construído y fuera solo remplazarlo en la DB. Pero veo que al final el script se construye del lado de Magento.

En ese caso, no sería mejor hacerlo usando UpgradeData? https://developer.adobe.com/commerce/php/development/prepare/extension-lifecycle/#data-upgrade

De esta forma nos ahorramos tener que llamar a cada uno de los clientes a su endpoint, además de tener que manejar los casos en los que no han actualizado su plugin.

Y otra cuestión si se hace con endpoint, para los clientes que no se actualicen el plugin ahora, hasta cuando probaremos atacar el endpoint? puede ser que el cliente tarde 3 meses en actualizar el plugin,o más.

Creo que se le puede dar una vuelta.

Qué creen, @davidmolinacano, @sofia-doofinder ?

Necesitamos que sea así porque no queremos que el script se les reemplace automáticamente, si no que cuando acepten unas condiciones en el admin de doomanager entonces se haga el reemplazo. En el admin de doomanager se le va a pedir al usuario que se actualice a la última versiónd el plugin y después en función de si tenga personalizaciones o no se hará la llamada a este endpoint o será soporte quien ayude a los clientes a hacer el cambio.

davidmolinacano commented 1 month ago

Debido a ciertas limitaciones de Magento, la REST API no puede devolver directamente un array asociativo, devuelve un array simple: https://github.com/magento/magento2/issues/30676

Commit que se ve afectado por esto: https://github.com/doofinder/doofinder-magento2/pull/323/commits/4a0d21da5d20ed786c4bd4e95014a00666b6bf8e