Produits Entr'ouvert » Corbo

Je trouverais plus chouette que corbo aspire les images des fils RSS pour les avoir en local, ça permettrait ainsi d'être sûr de les avoir en https.

Pour ne pas calculer le message à chaque fois, ça doit être un autre ticket (a priori je ne vois pas d'utilisé à rendre le texte du placeholder configurable).

Frédéric Péters a écrit :

Je trouverais plus chouette que corbo aspire les images des fils RSS pour les avoir en local, ça permettrait ainsi d'être sûr de les avoir en https.

Je ne voudrais pas avoir à gérer des situations ou une image change mais pas son nom.

Pour ne pas calculer le message à chaque fois, ça doit être un autre ticket (a priori je ne vois pas d'utilisé à rendre le texte du placeholder configurable).

Ok, je fais un ticket à part.

Je ne voudrais pas avoir à gérer des situations ou une image change mais pas son nom.

Alors la récupérer à chaque fois que la mise à jour du fil est demandée. Ou si ça ne suffit pas, faire proxy pour la récupérer à la volée; genre passer sur toutes les URL et les transformer en /foobar/<hash> et ajouter (hash, url d'origine) dans une table.

Je ne suis pas sûr de voir l'interet de récuperer les images chez nous et de les servir ensuite.

Je propose deux options, tu dis non aux deux ? Parce que pour les deux, l'intérêt c'est pouvoir servir les images en https, éviter les alertes sur le contenu mixte voire le non-chargement des images.

Ok, je vois. C'est pour l'affichage des images dans le flux RSS emis par corbo. Je refais le patch dans #12919

Ce n'était pas le cas dans le code d'avant mais il faut en fait gérer le cas où requests.get() ne retourne pas une 200.

« image_name = os.path.basename(img.attrib['src']) » ça me semble un peu casse-gueule avec des src du genre https://machin/truc/logo.png qui risque de se retrouver dix mille fois (ie des logo.png différents sur des sites et uri différentes, alors que le storage est unique) ?

Par ailleurs je comprends pas trop pourquoi on ne fait pas ça lors du système qui envoie le mail (un outil générique utils.send_html_email). Ici on se retrouve avec des machineries qui peuvent bien planter (host not found & co lors du requests.get) dans un save(), pas facile à remonter ("idéalement une alerte «il y a eu une erreur lors de la génération du mail»")... Bon, ça me semble un peu bizarre, mais j'ai pas suivi l'affaire.

Thomas Noël a écrit :

« image_name = os.path.basename(img.attrib['src']) » ça me semble un peu casse-gueule avec des src du genre https://machin/truc/logo.png qui risque de se retrouver dix mille fois (ie des logo.png différents sur des sites et uri différentes, alors que le storage est unique) ?

Dans ce cas je vérifier les checksums de ce logo.png et un logo.png distant. S'ils différent je laisse le storage le sauvegarder sous un nom différent, genre logo_dfdsfs.png et me retourner le nom de ce nouveau fichier.

Par ailleurs je comprends pas trop pourquoi on ne fait pas ça lors du système qui envoie le mail (un outil générique utils.send_html_email). Ici on se retrouve avec des machineries qui peuvent bien planter (host not found & co lors du requests.get) dans un save(), pas facile à remonter ("idéalement une alerte «il y a eu une erreur lors de la génération du mail»")... Bon, ça me semble un peu bizarre, mais j'ai pas suivi l'affaire.

L'idée du téléchargement des images lors des création des articles vient du fait que les annonces peuvent être visualisées dans un flux RSS visualisé dans une appli HTTPs, la cellule Feed de combo par exemple. Si les images dans le flux d'origine sont servies en HTTP elles risquent de ne pas s'afficher dans le lecteur en HTTPS.

Mettons une première entrée avec une première image, 2016/photo.jpeg. Le fichier n'existe pas, save() crée un fichier photo.jpeg, c'est ok.

Mettons une deuxième entrée avec une deuxième image, 2017/photo.jpeg. Le contenu est différent du image_name (photo.jpeg, au-dessus), donc storage.save() est appelé, et se trouve créer un nouveau fichier. À chaque appel de .save().

Ma proposition serait de nommer les fichiers de manière stable et unique (images/<id de l'objet>/<compteur>_<basename>).

Patch à jour, rebasé sur le master.

Manque le nouveau patch ? Ou l'ancien est en fait toujours ok ?

L'ancien est toujours bon.

Par rapport aux remarques de Fred il me semble qu'il manque l'idée d'ajouter l'id de l'objet dans le chemin vers le fichier statique.

Ack alors.

C'est compliqué quand il y a plusieurs patchs dans un même ticket, au final, celui-ci, c'est bien 0001-store-remote-images-on-annonce-save-13450.patch et 0002-consider-all-images-to-attach-as-remote-13450.patch ?

Ça serait bien d'attacher les patchs rebasés, parce que le patch ne s'applique pas sur master (déjà bêtement il contient tests/media/another_logo.png qui est arrivé dans le dépôt par ailleurs).

Et donc, maintenant qu'un seul des patchs a été poussé, quid de 0002-consider-all-images-to-attach-as-remote-13450.patch ?

Ack pour ce patch.