search

Mange / roadie-rails

Making HTML emails comfortable for the Rails rockstars
MIT License
367 stars 66 forks source link

remove asset digest #34

Closed tomasc closed 9 years ago

tomasc commented 9 years ago

Make sure to remove all digests from asset paths so that the assets can be found by the asset pipeline.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.03%) to 97.91% when pulling 41803abf8d5ed09578aec8511eddd2b939f89d5e on tomasc:master into 92ebd8b8b3f76f294cce2eb5cfdde91713d9bcdd on Mange:master.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.09%) to 97.79% when pulling a643b7dd46b0b4c527a0005590c223a877ad49a5 on tomasc:master into 92ebd8b8b3f76f294cce2eb5cfdde91713d9bcdd on Mange:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.03%) to 97.91% when pulling a643b7dd46b0b4c527a0005590c223a877ad49a5 on tomasc:master into 92ebd8b8b3f76f294cce2eb5cfdde91713d9bcdd on Mange:master.

Mange commented 9 years ago

Since this is a pretty "dangerous" proposition (see my comment on #15), I'm not sure if I want this in or not. I see the following options:

  1. Let it in as it is
    • CON: May lead to emails being very heavy to process if assets aren't properly compiled on the server and the developer would be none the wiser.
    • PRO: Less complexity for the user; helps handle complex cases; makes emails work when assets aren't deployed correctly instead of just crashing
  2. Let it in, but with a configuration parameter
    • CON: More configuration parameters to document, test and understand.
    • PRO: Opt-in to handling complex behavior
  3. Leave it out
    • CON: You cannot use AssetPipelineProvider in an environment with precompiled assets.
    • PRO: Roadie is never responsible for compiling assets when it shouldn't do it (digest is true).

I'm leaning towards alternative 1, but I'll have to sleep on it.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.02%) to 97.9% when pulling 3941838655690c063920b17289d03a0254f3a73c on tomasc:master into 92ebd8b8b3f76f294cce2eb5cfdde91713d9bcdd on Mange:master.

Mange commented 9 years ago

I'll accept this, for version 1.1, as it requires some documentation and extra thoughts. Would you mind having your fork active for now so I can merge at a later date, or would you prefer me to create a new branch to have this merged into?

tomasc commented 9 years ago

I can keep this active as long as needed.

Mange commented 9 years ago

I fast-tracked and released this as 1.0.5 because this was needed for new Rails 4.2 apps with their new default configuration (see #36).

Thank you for your contribution! :smile:

(Your commits were squashed; author information was kept intact)