Skip to content

Conversation

@mgax
Copy link
Member

@mgax mgax commented Apr 16, 2025

Fixes #481. This PR supersedes #492.

I was able to replicate a recent newsletter edition with very small spacing differences. I'm pretty happy with the editing experience and have somewhat tested this using Litmus. The main issue with testing is that images are served locally, so Litmus can't load them; we'd need to either deploy this to staging, or use a tunnelling tool like ngrok.

Things that I'm not so happy about:

  • Links in the accent block aren't styled as white text in some Outlook versions.
  • The vertical spacing is a bit fiddly, since I mostly had to work with padding, not margins (margins can overlap, padding can't).
  • There's no code to migrate the archive from Mailchimp just yet. I'm hoping that an AI might make writing that code not too painful. Import works.
  • The detail view of a newsletter will load the email-formatted HTML from a variable into the shadow DOM of a<div>, so that its styling doesn't break the header and footer. I'm not sure how well this plays with search engines. The content is now rendered using the Declarative Shadow DOM syntax.
  • The import_newsletter_archive command doesn't work for TWiW issues 138 through 172. Fixed.

@mgax mgax mentioned this pull request Apr 16, 2025
6 tasks
@mgax
Copy link
Member Author

mgax commented Apr 28, 2025

I've added a management command to import a newsletter edition:

./manage.py import_newsletter https://mailchi.mp/wagtail/twiw-11034221 "Issue #180"

It will skip over the date and the footer, detect headings, copy images, and preserve links in the text. It doesn't copy the "Packages" section using the right block type (AccentRichTextBlock) but I'm wondering if that might be best cleaned up manually.

@mgax mgax marked this pull request as ready for review April 28, 2025 15:14
@vossisboss
Copy link
Collaborator

FYI Alex. I got all the packages installed and such to test your branch but I'm running into a migration error that reads:

cannot ALTER TABLE "newsletter_newsletterpage" because it has pending trigger events

Baptiste suggested that we split one of the migration files in two. Apparently Postgres doesn't like it when you try to combine certain actions together. I'm going to attempt that and see if it fixes the issue. If I have no luck, I may need you to take another look at this or help me through the fiddly migration parts.

@mgax
Copy link
Member Author

mgax commented May 1, 2025

I couldn't replicate the migration error, but I've encountered such errors in the past, and I think it's best to avoid them. I've split the migration into two steps.

Comment on lines 29 to 41
migrations.RemoveField(
model_name="newsletterpage",
name="body",
),
migrations.RemoveField(
model_name="newsletterpage",
name="intro",
),
migrations.RenameField(
model_name="newsletterpage",
old_name="content",
new_name="body",
),
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend splitting the RemoveField operations to another migration. The problem tends to happen if you have both a data migration operation (in this case RunPython(convert_body_to_streamfield)) and a schema migration operation in a single migration. Migrations are run in a single transaction (if the backend supports DDL transactions), and the database may not allow data manipulation in a DDL transaction.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point. I've also noticed that images were not getting preserved in the data migration so I've fixed that too.

@Stormheg
Copy link
Member

Stormheg commented May 9, 2025

@mgax I ran into issues with the migration script not parsing the images correctly. I've pushed a fixup commit for that, please review.

Copy link
Member

@Stormheg Stormheg left a comment

Choose a reason for hiding this comment

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

@mgax couple minors comments.

Impressed by the migration of the old issues (Issues <= 138). Though this is not how they originally looked, though I don't think it is super important to keep their original look. They look clean now.

The detail view of a newsletter will load the email-formatted HTML from a variable into the shadow DOM of a

, so that its styling doesn't break the header and footer. I'm not sure how well this plays with search engines.

Have you considered loading the email HTML in an iframe? That should accomplish mostly the same thing. Iframes have been around for so long it is more likely they behave nicely in the context of SEO - if that is even important.


@register_setting
class NewsletterSettings(BaseGenericSetting):
footer = StreamField(NewsletterContentBlock(), blank=True, use_json_field=True)
Copy link
Member

Choose a reason for hiding this comment

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

@mgax this and the StreamField below don't require setting use_json_field anymore, see https://docs.wagtail.org/en/stable/releases/6.0.html#streamfield-no-longer-requires-use-json-field-true

Suggested change
footer = StreamField(NewsletterContentBlock(), blank=True, use_json_field=True)
footer = StreamField(NewsletterContentBlock(), blank=True)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in d00d9b0.

class NewsletterPage(NewsletterPageMixin, Page):
date = models.DateField()
preview = models.TextField(blank=True)
body = StreamField(NewsletterContentBlock(), blank=True, use_json_field=True)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
body = StreamField(NewsletterContentBlock(), blank=True, use_json_field=True)
body = StreamField(NewsletterContentBlock(), blank=True)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in d00d9b0.

search_fields = Page.search_fields + [
index.SearchField("intro"),
index.SearchField("body"),
index.SearchField("body"),
Copy link
Member

Choose a reason for hiding this comment

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

Accidental double?

Suggested change
index.SearchField("body"),

Comment on lines 80 to 84
<p>
Want to change how you receive these emails? You can
<a href="*|UPDATE_PROFILE|*">update your preferences</a> or
<a href="*|UNSUB|*">unsubscribe from this list</a>.
</p>
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: it looks like these end up on the web page version. Shouldn't these links be hidden? They don't go anywhere.

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in bec1ad0.

Comment on lines 28 to 30
<mj-text align="center" css-class="rich-text">
<a href="{{ page.full_url }}">View this email online</a>
</mj-text>
Copy link
Member

Choose a reason for hiding this comment

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

Same as below - shouldn't this be hidden on the web version of the page?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in bec1ad0.

Comment on lines 8 to 16
<script>
(function() {
const container = document.getElementById("newsletter-container");
const shadow = container.attachShadow({mode: "open"});
const wrapper = document.createElement("div");
wrapper.innerHTML = JSON.parse(document.getElementById("email-html-data").textContent);
shadow.appendChild(wrapper);
})();
</script>
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: maybe there is an alternative for the shadow dom?

Issue: otherwise this should be converted to a dedicated JS script that is loaded on this page and creates the shadow DOM.

Adding inline JS is not acceptable as we're trying to add a Content Security Policy (see #468) in the future, which does not allow for inline JS

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in c65e242. It turns out you can inline the shadow dom content.

@mgax
Copy link
Member Author

mgax commented May 26, 2025

Have you considered loading the email HTML in an iframe?

Getting the iframe height is notoriously difficult. Also, I suspect the search engines might index the content at the iframe's src url, which is not great.

But it turns out that you can inline the content for the shadow dom, and it seems search engine will index it: https://dev.to/stuffbreaker/seo-and-web-components-2023-edition-3l6i

@mgax
Copy link
Member Author

mgax commented May 26, 2025

I also made a import_newsletter_archive management command, which includes a hardcoded list of all the TWiW issues from when we moved to Mailchimp until now. It turns out that the import script only works from issue 173 onwards; the markup of the older issues is different. I've commented out the ones that fail to import.

Also @vossisboss, did we skip issue 163? It doesn't seem to be in my personal email archive either.

@mgax
Copy link
Member Author

mgax commented May 26, 2025

It turns out that the import script only works from issue 173 onwards; the markup of the older issues is different. I've commented out the ones that fail to import.

The importing is fixed now, I've checked all of the back issues by opening them in the browser and having a quick glance. This PR is ready for another review.


And I almost forgot: thank you @Stormheg for the thorough review and the fix to the data migration code 🙌

@mgax mgax requested a review from Stormheg May 26, 2025 15:48
@vossisboss
Copy link
Collaborator

vossisboss commented May 28, 2025

@mgax We may have skipped an issue. I wouldn't be surprised. It will just be a part of Wagtail lore now. I am testing your code right now. I pushed the Wagtail 7.0 upgrade today, so would you mind rebasing this to the latest version?

I don't have a lot more time left in my day to test this so I might pick this up again in the morning and send you my notes. I can confirm that the database issue was solved though and I got the development server to start.

@mgax
Copy link
Member Author

mgax commented Jun 3, 2025

I pushed the Wagtail 7.0 upgrade today, so would you mind rebasing this to the latest version?

@vossisboss I did a merge with main, since it looks like we're squashing PRs anyway. I can rebase and force-push if that makes more sense.

@mgax
Copy link
Member Author

mgax commented Sep 4, 2025

I had some time today and I've polished this PR. I think it's just about ready for prime time.

@vossisboss I've implemented the short list of things that you've flagged:

  • Buttons can now reference a page, not just a URL
  • Paragraphs in the "Accent Rich Text" block have no margin
  • New social buttons for Mastodon and Bluesky. Since Mailchimp doesn't seem to have a Mastodon icon on their CDN, I've reused the SVG icons from the wagtail.org footer (and rendered them as 80x80 PNGs with no transparency).
  • I have not hardcoded the footer text because I'd already implemented a way to edit it. It's under "Settings > Newsletter settings". I've now added a 10px spacing between the body and the footer.
  • I've checked that it's safe to duplicate pages – the newsletter Mailchimp state is not copied over.

Also, I've bumped wagtail-newsletter to the newly released v0.2.3, and updated the list of existing newsletters to import. @vossisboss please check that I've got the numbering right!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Port "This Week in Wagtail" to wagtail-newsletter

4 participants