Skip to content

Add WhatsApp message title field to export and import processes#472

Open
erikh360 wants to merge 1 commit into
mainfrom
add-whatsapp-message-title
Open

Add WhatsApp message title field to export and import processes#472
erikh360 wants to merge 1 commit into
mainfrom
add-whatsapp-message-title

Conversation

@erikh360

Copy link
Copy Markdown
Contributor

Purpose

Why does this exist?

Solution

_What changed, and does this address the problem? _

Checklist

  • Added or updated unit tests
  • Added to release notes
  • Updated readme/documentation (if necessary)
  • Add support to FakeCMS in the flow tester (if necessary)

Comment thread home/tests/test_api.py
assert body["total_messages"] == 1
assert body["text"]["type"] == "Whatsapp_Message"
assert body["text"]["value"]["message"] == "*Default whatsapp Content 1* 🏥"
assert body["text"]["value"].get("title", "") == ""

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we expect this field to always be in the response, we should index it directly like we do with the other fields.

return {
"whatsapp_template_slug": "",
**entry,
"whatsapp_message_title": entry.get("whatsapp_message_title") or "",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"whatsapp_message_title": entry.get("whatsapp_message_title") or "",
"whatsapp_message_title": entry.get("whatsapp_message_title", ""),

(
"Whatsapp_Message",
{
"title": "",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need a test with a non-empty title?

expected_result = {
"type": "Whatsapp_Message",
"value": {
"title": "",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need a test with a non-empty title?

else:
self.whatsapp_body = whatsapp.value["message"].strip()
# Per-message WhatsApp title (not the page-level whatsapp_title)
self.whatsapp_message_title = whatsapp.value.get("title", "")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since title should always exist, I think we want value["title"] here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since we've done a django migration, but not a streamfield migration, that field might not exist on messages that haven't been updated. I don't think adding a field requires us to add a streamfield migration, although we might have to deal with the field being missing on old messages: https://docs.wagtail.org/en/v4.2/advanced_topics/streamfield_migrations.html#streamfield-data-migrations

Comment thread home/models.py
Comment on lines +1634 to +1639
# Provide a minimal snippet_viewset interface for chooser widgets when the
# snippet isn't registered in the admin (feature-flagged). Wagtail's
# SnippetChooserBlock accesses `target_model.snippet_viewset.icon`.
class snippet_viewset:
icon = "order"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this related to the WA title?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

no idea why it added this

else:
self.whatsapp_body = whatsapp.value["message"].strip()
# Per-message WhatsApp title (not the page-level whatsapp_title)
self.whatsapp_message_title = whatsapp.value.get("title", "")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since we've done a django migration, but not a streamfield migration, that field might not exist on messages that haven't been updated. I don't think adding a field requires us to add a streamfield migration, although we might have to deal with the field being missing on old messages: https://docs.wagtail.org/en/v4.2/advanced_topics/streamfield_migrations.html#streamfield-data-migrations

Comment thread home/serializers.py
# to retain the v2 api behaviour. We also return some fields as null or empty values, for the same reason
"type": "Whatsapp_Message",
"value": {
"title": "",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't need to add this to the v2 API, especially just a hardcoded empty string, since we can keep the v2 API the same and just add this to the v3 API.

Comment thread home/serializers.py

if str(text["type"]) == "Whatsapp_Message":
# Ensure the new title field always exists in API output
text["value"].setdefault("title", "")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here, we don't need this in the v2 API. What we do need, however, is for this to be present in the new v3 API, in serializers_v3.py

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.

3 participants