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

make mobile app notification title and subtitle templatable #3845

Merged
merged 4 commits into from
Feb 8, 2024

Conversation

joeyorlando
Copy link
Contributor

@joeyorlando joeyorlando commented Feb 6, 2024

What this PR does

Closes #2050

https://www.loom.com/share/cca9af04f905456087f25e9cbf1845ab

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • CHANGELOG.md updated (or pr:no changelog PR label added if not required)

@joeyorlando joeyorlando requested a review from a team February 7, 2024 20:55
@joeyorlando joeyorlando marked this pull request as ready for review February 7, 2024 20:56
@joeyorlando joeyorlando requested a review from a team as a code owner February 7, 2024 20:56
@joeyorlando joeyorlando requested review from a team February 7, 2024 20:56
Copy link
Contributor

@imtoori imtoori left a comment

Choose a reason for hiding this comment

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

From a mobile point of view it looks great

Copy link
Contributor

@alyssawada alyssawada left a comment

Choose a reason for hiding this comment

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

doc updates look good! thanks :)

templatable (sans messaging backend usage)

finalizing things

final tweaks

fix test

add one more test case
@joeyorlando joeyorlando force-pushed the jorlando/mobile-notification-templating branch from 8359f64 to c071ce6 Compare February 8, 2024 20:43
@@ -10,6 +10,7 @@ class BaseMessagingBackend:

templater = None
template_fields = ("title", "message", "image_url")
skip_default_template_fields = False
Copy link
Contributor Author

@joeyorlando joeyorlando Feb 8, 2024

Choose a reason for hiding this comment

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

I added this to avoid having these auto-added as the default template values (see usage here):
Screenshot 2024-02-08 at 15 44 48
(this is a screenshot of the Web templates but the same values were showing up by default for Mobile app templates (even though the templates for these fields in the alert_receive_channel.messaging_backends_templates object were not set))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with the latest commit, c071ce6, labels cannot be (easily w/o decent refactor/hackery) used in the templates here because I am using the MessagingBackend/AlertTemplater approach. AlertTemplater will need to be refactored to support the concept of labels.

Otherwise 27ecdd4 supports the usage of labels in the templates (but as a caveat adds two new columns to the AlertReceiveChannel table, as it does not use the MessagingBackend approach)

Comment on lines +11 to +13
# this is a dirty hack to get around EXTRA_MESSAGING_BACKENDS being set in settings/ci-test.py
# we can't simply change the value because 100s of tests fail as they rely on the value being set to a specific value 🫠
# see where this value is used in the unitest.mock.patch calls down below for more context
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if there's a better approach here?

I wasn't very fond on updating 100+ tests in this PR, that're tightly coupled to the value of EXTRA_MESSAGING_BACKENDS in settings/ci-test.py

@joeyorlando joeyorlando requested a review from mderynck February 8, 2024 20:55
@joeyorlando joeyorlando merged commit dd73e58 into dev Feb 8, 2024
21 checks passed
@joeyorlando joeyorlando deleted the jorlando/mobile-notification-templating branch February 8, 2024 22:23
brojd pushed a commit that referenced this pull request Sep 18, 2024
# What this PR does

Closes #2050

https://www.loom.com/share/cca9af04f905456087f25e9cbf1845ab

## Checklist

- [x] Unit, integration, and e2e (if applicable) tests updated
- [x] Documentation added (or `pr:no public docs` PR label added if not
required)
- [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)
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.

Mobile push message is not taken from the template
5 participants