Add newsletter templates #5887
Add newsletter templates #5887
Conversation
mrcasals
force-pushed
the
newsletter-templates
branch
5 times, most recently
from
b5e514d
to
702c2a5
Compare
It seems to be a reserved column name
mrcasals
force-pushed
the
newsletter-templates
branch
from
7fa858f
to
c551055
Compare
|
@agustibr thanks! 👀 |
|
@decidim/core everything should be good now! Can you review the PR please? |
|
@decidim/core this has been approved by @decidim/product, can you review it please? 😄 |
mrcasals
requested review from
microstudi and
tramuntanal
and removed request for
tramuntanal
There was a problem hiding this comment.
Buff, huge work here!
I've left some trivial comments and suggestions. 😄
Also I've found that when you preview a template, there's no way to change your mind and go back and select another template. Is it possible to add an "back to all templates" button or similar?
decidim-core/db/migrate/20200327082257_migrate_newsletters_to_templates.rb
Show resolved
Hide resolved
decidim-core/db/migrate/20200327082257_migrate_newsletters_to_templates.rb
Show resolved
Hide resolved
|
|
||
| class AddIdToContentBlocksScope < ActiveRecord::Migration[5.2] | ||
| def change | ||
| add_column :decidim_content_blocks, :scope_id, :integer |
There was a problem hiding this comment.
scope_id is very confusing because Decidim has the concept of Sope as a model, but I can't think of an alternative: parent_id, related_id, ...
There was a problem hiding this comment.
Yeah, me neither... 🤷
There was a problem hiding this comment.
can we use container_id so we get rid of the confusion?
There was a problem hiding this comment.
container_id makes me think that the container block is contained inside another resource... What about resource_id?
This reverts commit bbbeaa4.
|
@tramuntanal all changes addressed, can you check them please?
I intentionally left the "New newsletter" button in the sidebar so you can go back to that page, adding a "back" button seemed redundant to me! |
|
|
||
| class AddIdToContentBlocksScope < ActiveRecord::Migration[5.2] | ||
| def change | ||
| add_column :decidim_content_blocks, :scope_id, :integer |
There was a problem hiding this comment.
can we use container_id so we get rid of the confusion?
|
@tramuntanal I've renamed that field to |
There was a problem hiding this comment.
The idea of using container_id was to avoid relating this field with the concept of Decidim::Scope.
Using scoped_resource_id relates this field with resources. We would have to rename it again if someday we use content blocks in participatory spaces or faqs. 😢
Anyway, I don't want to be a stopper for this feature, so I'm approving.
* Add ID to content blocks scope * Add basic newsletter template * List templates * Fix menu highlight * Create newsletters from template * Use cells as templates in newsletters * Update newsletters * Use public name in newsletter templates list * Rename `scope` column to `scope_name` It seems to be a reserved column name * Fix rubocop complaints * Fix tests * Fix lints * Fix locales * Fix more specs * Fix more specs * Fix specs * Lint ERB file * Migrate newsletters to templates * Preview newsletter templates * Lint code * Normalize locales * Add new newsletter template * Add locales for newsletter templates forms * Add image to new newsletter template * Preview newsletter template images * Make previews work for multiple images * Improve templates gallery * Refactor classes to help implementing new templates * Add docs * Fix errors * Fix specs * Remove wrong check * Use correct image extension * Add changelog * Improve docs * Improve message * Add index on content blocks scope_id * Remove uniqueness of index * Add link on docs * Use lorem ipsum as preview text * Fix index * Fix index column name * Normalize locales * Fix spec to match expected value * Fix test * Improve docs * Revert "Remove wrong check" This reverts commit bbbeaa4. * Rename content block `scope_id` to `scoped_resource_id` * Fix migration name
* develop: (65 commits) Add newsletter templates (decidim#5887) Send email with order summary on order checkout (decidim#6006) Change small details on documentation (decidim#5890) feat(budgets): Projects filter by multiple categories (decidim#5992) Participant renewable verifications (decidim#5854) Update .simplecov (decidim#5949) Remove legacy assembly types (decidim#5617) Don't follow the header x forwarded host by default (decidim#5899) Add two CTA on initiative (decidim#5838) Conversations with more than one participant (decidim#5861) Fix supported versions in SECURITY.md (decidim#5957) Add a parameter to specify a list of whitelist ip on /system (decidim#5669) Fix error 500 when showing new debate notifications (decidim#5964) Upgrade sassc and sassc-rails dependency (decidim#5910) Add minimum projects rule to Budgets (decidim#5865) Update changelog with current develop Fix bad formatted changelog entries fix move proposal endorsements migration (decidim#5953) Fix the scopes picker rendereding escaped characters (decidim#5939) Revert "Fix the scopes picker rendereding escaped characters (decidim#5793)" (decidim#5937) ...
🎩 What? Why?
This PR adds a system of newsletter templates to choose from. See #5727 (comment) for a more detailed description of the system, product-wise.
📌 Related Issues
📋 Subtasks
newsletter_templatescope. It should have the template and its fields (body, which is i18n HTML).CHANGELOGentry📷 Screenshots (optional)
Newsletter template gallery:
Previewing the old template:
Using the old tempalte to send a newsletter:
Previewing the new template (I've scrolled the iframe to show how it looks):
Using the new template to send newsletters: