Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send campaign #37

Merged
merged 2 commits into from
Jun 20, 2024
Merged

Send campaign #37

merged 2 commits into from
Jun 20, 2024

Conversation

mgax
Copy link
Member

@mgax mgax commented Jun 17, 2024

Fixes #24.
Fixes #25.

@@ -45,9 +46,29 @@ def url(self) -> str:
else:
return f"{base_url}/campaigns/edit?id={self.web_id}"

def get_report(self) -> "dict[str, Any]":
Copy link
Member Author

Choose a reason for hiding this comment

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

The type here is intentionally vague, so that it's easy to override this method, and the {% block campaign_status %} block, to show more (or different) information in the report.

@mgax mgax marked this pull request as ready for review June 17, 2024 16:08
@mgax mgax requested a review from olivierphi June 17, 2024 16:08
@olivierphi
Copy link

I started to test the branch on my local environment, and overall it seems to work pretty well 👍

I really like the spinner that is started just before the form is submitted, as it gives a much better user experience to the editor while the communication with Mailchimp takes place 🙂

I have 2 questions, that are not related to that specific PR but it seems that we're getting closer to being able to use that package for real now so I thought it could be worth testing a bit more thoroughly:

  • As Mailchimp "Merge Tags" use the *|TAGNAME|* form, if we try to use them in a Wagtail richtext they seem to be converted to italic text by Wagtail - which looks to be interpreting the content as Markdown? This prevents the use of Merge Tags 😔
  • On a side note: if I create a copy of a Wagtail Page, this copy seems to share the same Campaign as the original Page - and if the original Page was already sent as a newsletter campaign, the copy will also be considered as sent. Is this the expected behaviour?

Screenshot of Merge Tags displayed as italic text in the received email:

Screenshot from 2024-06-18 16-39-53

@mgax
Copy link
Member Author

mgax commented Jun 19, 2024

I really like the spinner that is started just before the form is submitted, as it gives a much better user experience to the editor while the communication with Mailchimp takes place 🙂

Thank you! Yeah, I think we have the opportunity to provide a nice user experience, and it's nice to have a slick interface 🙂

I have 2 questions, that are not related to that specific PR but it seems that we're getting closer to being able to use that package for real now so I thought it could be worth testing a bit more thoroughly:

  • As Mailchimp "Merge Tags" use the *|TAGNAME|* form, if we try to use them in a Wagtail richtext they seem to be converted to italic text by Wagtail - which looks to be interpreting the content as Markdown? This prevents the use of Merge Tags 😔

Huh, you raise a good point, I hadn't realised this. I've opened an issue: #39.

  • On a side note: if I create a copy of a Wagtail Page, this copy seems to share the same Campaign as the original Page - and if the original Page was already sent as a newsletter campaign, the copy will also be considered as sent. Is this the expected behaviour?

No, I don't think so. Since this was introduced in a former PR, and I'd rather get this one merged, I've opened another issue: #40.

@mgax mgax merged commit be7f5b4 into main Jun 20, 2024
12 checks passed
@mgax mgax deleted the send-campaign branch June 20, 2024 11:15
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.

Show campaign performance metrics Send campaign immediately
2 participants