correctif(dolist): utilise des liens vers les logos des procedures plutôt que des attachements.inlined #9369

mfo · 2023-07-27T09:44:39Z

hs: https://secure.helpscout.net/conversation/2296819535/2034810/

contexte

l'ami dolist qui supporte pas les attachments inline 🤦 .

solution

on fait plus d'attachement inline pour le logo des demarches

what-the-diff · 2023-07-27T09:45:12Z

PR Summary

Incorporated 'mini_magick' library
A new software library called 'mini_magick' has been added to our project. This library will help us manage image files in a more efficient way.
Modification in 'DossierMailer'
In a file, which is responsible for sending emails, we've made changes to the method which attaches logos to the mail. Now, instead of using a pre-defined logo, we will use a link that leads to the logo image.
Update in 'Procedure' model
Within the 'Procedure' model, the method to provide the logo link (logo_url), has been altered. Instead of sending a normal logo image, it will now send a modified version of the logo which has been resized to a certain limit. This ensures consistency of the logo size across all instances.
Configuring 'active_storage' to use 'mini_magick'
The system configuration has been modified for our file storage element (active_storage). Earlier, it was using a tool called 'vips' to process image variations. Now, it will use the newly added 'mini_magick'.
This change will help benefit from 'mini_magick's efficient image management capabilities.

tchak · 2023-07-27T10:04:39Z

config/initializers/new_framework_defaults_7_0.rb

@@ -73,7 +73,7 @@
 # generate variants to use image processing macros and ruby-vips
 # operations. See the upgrading guide for detail on the changes required.
 # The `:mini_magick` option is not deprecated; it's fine to keep using it.
-Rails.application.config.active_storage.variant_processor = :vips


pourquoi ? vips est plus moderne et rapide

vu avec nico. ca tire 70 dependances sur l'infra dont le serveur X de firefox :/

aïe mince je comptais aussi sur vips pour les filigranes :/

la c'etait pour eviter de poluer l'infra pour un gain de perf marginal (on retaille qq logo...). A voir si le filigranes se fait ac imagemagick ?

On peut pas changer de distrib ? C'est vraiment complètement pété ubuntu...

tchak · 2023-07-27T10:05:19Z

app/models/procedure.rb

@@ -702,7 +702,7 @@ def missing_steps

  def logo_url
    if logo.attached?
-      Rails.application.routes.url_helpers.url_for(logo)
+      Rails.application.routes.url_helpers.url_for(logo.variant(resize_to_limit: [150, 150]).processed)


cette url va expirer au bout d'une heure

merci, a creuser le sujet je ne vois pas de solution plus simple que de mettre un expires lointain. Qu'en penses-tu ?
[autre option serait de mettre un bucket public, mais ca demande a migrer les pjs existantes etc...]. J'imagine qu'au bout de 6 mois on peut considerer ces liens ds les mails comme mort sans soucis.

on pourrait réorganiser les options du BlobSignedIdConcern pour qu'on puisse overrider le expires_in avec nil, ou 99.years

je passe juste l'expires_in a la methode url qui le supporte.
J'ai regardé d'autres pistes mais toutes m'avaient l'air overkill. Si on souhaite vraiment faire du public sans trop tritouiller les internes de rails, le service openstack_public serait le plus propre je pense: https://github.com/chaadow/activestorage-openstack#usage (ca demande juste une duplication préalable des procedures.logo existants).

pour l'expires, j'ai mis 6.months, est-ce vraiment necessaire de pousser au dela ? [hésites pas a affirmer un truc j'ai pas d'avis la dessus]

Pour moi, la bonne approche consiste à exposer une URL publique du genre /assets/logo/:key avec une redirection. Je ne suis pas fan des URL avec une longue expiration dans la nature.

merci, je merge ta PR et utilise cette route (l'idée était de laisser le storage servir les PJs directement plutôt que passer par notre infra pour le redirect. mais c'est top !

colinux

ça me paraît bon. Juste un retour sur la durée.

A vérifier: évidement sur dolist, mais aussi que brevo ne rajoute pas du tracking à notre insu sur l'url de l'image :/

EDIT: je suis en train de capter que l'expires va rajouter des query params dans l'url ; à voir si ça trigger pas certains blockers, mais on le saura à l'usage en fonction des retours

colinux · 2023-07-27T15:55:12Z

config/initializers/new_framework_defaults_7_0.rb

@@ -73,7 +73,7 @@
 # generate variants to use image processing macros and ruby-vips
 # operations. See the upgrading guide for detail on the changes required.
 # The `:mini_magick` option is not deprecated; it's fine to keep using it.
-Rails.application.config.active_storage.variant_processor = :vips


app/models/procedure.rb

…utôt que des attachements.inlined

…e logo dans les mails

mfo force-pushed the US/fix-email-embeded-logos branch from 425e8e4 to 5541db7 Compare July 27, 2023 09:53

tchak reviewed Jul 27, 2023

View reviewed changes

mfo force-pushed the US/fix-email-embeded-logos branch 6 times, most recently from c2a5ebf to 3456129 Compare July 27, 2023 15:05

colinux approved these changes Jul 27, 2023

View reviewed changes

mfo force-pushed the US/fix-email-embeded-logos branch 2 times, most recently from 343b7e0 to 2a29eba Compare July 28, 2023 08:43

mfo enabled auto-merge July 28, 2023 08:49

tchak mentioned this pull request Aug 3, 2023

feat(demarche): expose public logo url #9376

Merged

mfo force-pushed the US/fix-email-embeded-logos branch from 2a29eba to b093df6 Compare August 14, 2023 08:15

mfo disabled auto-merge August 14, 2023 08:19

mfo force-pushed the US/fix-email-embeded-logos branch 2 times, most recently from eb84b02 to 3de6b17 Compare August 14, 2023 09:26

mfo enabled auto-merge August 14, 2023 09:36

Martin added 2 commits August 14, 2023 13:09

correctif(dolist): utilise des liens vers les logos des procedures pl…

a3c6a2c

…utôt que des attachements.inlined

amelioration(mailer.logo): utilise procedure_logo_url pour afficher l…

bf6b5ea

…e logo dans les mails

mfo force-pushed the US/fix-email-embeded-logos branch from 3de6b17 to bf6b5ea Compare August 14, 2023 11:09

mfo added this pull request to the merge queue Aug 14, 2023

Merged via the queue into demarches-simplifiees:main with commit b781e4d Aug 14, 2023

mfo deleted the US/fix-email-embeded-logos branch August 14, 2023 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

correctif(dolist): utilise des liens vers les logos des procedures plutôt que des attachements.inlined #9369

correctif(dolist): utilise des liens vers les logos des procedures plutôt que des attachements.inlined #9369

mfo commented Jul 27, 2023 •

edited

Loading

what-the-diff bot commented Jul 27, 2023

tchak Jul 27, 2023

mfo Jul 27, 2023

colinux Jul 27, 2023

mfo Jul 27, 2023

colinux Jul 27, 2023

tchak Aug 1, 2023

tchak Jul 27, 2023

mfo Jul 27, 2023

colinux Jul 27, 2023

mfo Jul 27, 2023

tchak Aug 1, 2023

tchak Aug 1, 2023

mfo Aug 14, 2023

colinux left a comment •

edited

Loading

colinux Jul 27, 2023

correctif(dolist): utilise des liens vers les logos des procedures plutôt que des attachements.inlined #9369

correctif(dolist): utilise des liens vers les logos des procedures plutôt que des attachements.inlined #9369

Conversation

mfo commented Jul 27, 2023 • edited Loading

contexte

solution

what-the-diff bot commented Jul 27, 2023

PR Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinux left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfo commented Jul 27, 2023 •

edited

Loading

colinux left a comment •

edited

Loading