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

Page stats cassée #849

Merged
merged 16 commits into from Mar 1, 2023
Merged

Page stats cassée #849

merged 16 commits into from Mar 1, 2023

Conversation

laem
Copy link
Contributor

@laem laem commented Feb 21, 2023

Fixes #817

@netlify
Copy link

netlify bot commented Feb 21, 2023

Deploy Preview for nosgestesclimat ready!

Name Link
🔨 Latest commit da9f391
🔍 Latest deploy log https://app.netlify.com/sites/nosgestesclimat/deploys/63ff27055ea8b300073e3cdf
😎 Deploy Preview https://deploy-preview-849--nosgestesclimat.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Feb 21, 2023

Report for the pull request #849


🌐 Translation status

UI's texts

Language Nb. missing translations Status
en-us Ø ✔️

FAQ's questions

Language Nb. missing translations Status
en-us 14 ⬇️
Check missing rulesges,international,2tonnes,déprime,défaut,services-publics,services-publics-majoritaires,effet-qualité,bio-2,régime,pas-de-voiture,autres-carburants,électricité-dite-verte,impact-numérique

You will find more information about the translation in the dedicated file.

@laem laem marked this pull request as ready for review February 21, 2023 17:42
@laem
Copy link
Contributor Author

laem commented Feb 21, 2023

@Clemog @florianpanchout @EmileRolley

Je veux bien une revue attentive de l'un d'entre vous au moins.

Au début, je suis parti sur l'idée de générer côté serveur un JSON qui contiendrait une sélection autorisée des stats.

Mais ça demandait de réécrire une grande partie du code de la page, avec la subtilité du fetch dynamique de certaines données (le chart est paramétré sur ses propriétés de période).

Du coup, j'ai finalement fait une fonction qui ne fait qu'autoriser les requêtes, et filtrer les résultats des requêtes "Page" pour en exclure les URL de sondage et conférence.

Elle n'est pas testable sur branche de démo, mais seulement en local en mettant MATOMO_TOKEN dans votre .env.

Le but est de vérifier si grâce à cette PR, on empêche effectivement l'exposition des données qu'on veut limiter. Pas si simple.

d'autres requêtes que l'on n'a pas filtré et qui pourraient révéler
d'autres données
@lbranaa lbranaa added the bug Something isn't working label Feb 22, 2023
Co-authored-by: Emile Rolley <44124798+EmileRolley@users.noreply.github.com>
@@ -0,0 +1,75 @@
import fetch from 'node-fetch'
Copy link
Contributor

Choose a reason for hiding this comment

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

Est-ce qu'il ne faudrait pas un require ici ? "Cannot use import statement outside a module"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

En fait je l'ai enlevé, en node 18 il ne sert à rien ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK maintenant j'ai

ReferenceError - fetch is not defined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je me suis permis de mettre AWS_LAMBDA_JS_RUNTIME en v18

netlify/functions/get-stats.js Outdated Show resolved Hide resolved
@laem
Copy link
Contributor Author

laem commented Mar 1, 2023

Suite à la remarque de Denis sur le cache : je suis en train de le faire côté scalingo (les fonctions netlify étant incapables de le faire). Ça me parait en effet un peu bloquant de pourrir le serveur qui nous héberge gratuitement.

@florianpanchout
Copy link
Contributor

Suite à la remarque de Denis sur le cache : je suis en train de le faire côté scalingo (les fonctions netlify étant incapables de le faire). Ça me parait en effet un peu bloquant de pourrir le serveur qui nous héberge gratuitement.

"Pourrir" et "bloquant" me semblent peut être un peu fort pour une page qui reçoit en moyenne 10 visites par jour

@laem
Copy link
Contributor Author

laem commented Mar 1, 2023

"Pourrir" et "bloquant" me semblent peut être un peu fort pour une page qui reçoit en moyenne 10 visites par jour

Pas faux, j'avais oublié qu'on n'exposait pas cette page sur le site. En même temps c'est l'occasion d'enfin le faire.

@laem
Copy link
Contributor Author

laem commented Mar 1, 2023

La fonction netlify est déplacée presque à l'identique ici https://github.com/datagir/nosgestesclimat-server/pull/6

(c'est cool nodejs :) )

@laem
Copy link
Contributor Author

laem commented Mar 1, 2023

Je me permets de ne pas vous demander de review de https://github.com/datagir/nosgestesclimat-server/pull/6 étant donné la similarité.

@laem laem merged commit 241426b into master Mar 1, 2023
@laem laem deleted the stats-cassée branch March 1, 2023 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Page stat
5 participants