feat: send mobile braze notifications LEARNER-10298 #36272

jawad-khan · 2025-02-19T11:00:15Z

Description

Send braze notification to mobile users. This change will pick braze campaign ids from db and send notification trigger to braze.
LEARNER-10298
Useful information to include:

This change will effect mobile users only.

Supporting information

Jira issue: https://2u-internal.atlassian.net/browse/LEARNER-10298

Deadline

ASAP.

miankhalid · 2025-03-10T09:18:11Z

@jawad-khan can you fix the failing checks here? before we request Infinity folks to give this PR a pass, thanks!

AhtishamShahid · 2025-03-18T08:10:41Z

openedx/core/djangoapps/notifications/models.py

@@ -266,3 +267,25 @@ def get_core_config(self, app_name) -> Dict:
        }
        """
        return self.get_notification_types(app_name).get('core', {})
+
+
+class NotificationBrazeCampaigns(TimeStampedModel):


This should not be a model it should be a django setting, Emails sent through braze also require campaing_id that is stored in Django settings. Please review how Braze is integrated into edx-platform for emails.
https://github.com/openedx/edx-ace/blob/64023b0b6a92fd990bcae2c79cce2ae2b2d3de6c/edx_ace/channel/braze.py#L56

I wanted mobile team to have the flexibility of adding braze campaign ids whenever they want. Plus it gives extra control on disabling push notifications for a certain notification type.

AhtishamShahid · 2025-03-18T08:12:42Z

openedx/core/djangoapps/notifications/tasks.py

+        send_braze_notification_to_mobile_users(push_notification_audience, generated_notification)
+
+
+def send_braze_notification_to_mobile_users(audience_ids, notification_object):


Sending push notifications through braze should be part of edx-ace , a new channel can be created for this in edx-ace like this
https://github.com/openedx/edx-ace/blob/master/edx_ace/channel/push_notification.py

kdmccormick

Can you provide some information on the initiative that this is part of? e.g., is this linked to a product proposal, or are there any public tickets or docs explaining the need for the new braze model?

jawad-khan · 2025-04-09T06:30:05Z

Can you provide some information on the initiative that this is part of? e.g., is this linked to a product proposal, or are there any public tickets or docs explaining the need for the new braze model?

Unfortunately there is no public ticket or documentation for it yet. We are implementing push notification using braze for mobile devices. This PR is still in progress and we might remove this braze model.

kdmccormick · 2025-04-09T14:54:29Z

Okay, I have converted it to Draft since it is still in progress. Just to set expectations, the edx-platform maintenance team is unlikely to accept new code that ties the core platform to Braze (especially data models) unless there is a compelling product use case for the community.

jawad-khan · 2025-04-25T10:59:15Z

@kdmccormick I have decoupled core platform from edx-braze-client and removed data model. Also changing the status of PR since its almost ready.

kdmccormick · 2025-04-28T13:50:38Z

Hey @jawad-khan. By installing edx-braze-client as a base edx-platform requirement, this PR would still be adding Braze-specific logic and requirements to edx-platform, which we cannot accept.

That said, thank you for your work on this--I think it's much closer to what we need. What we need is for the vendor-specific logic to be implemented through a pluggable backend rather than through a base-requirement library. My understanding is that edx-ace is meant to be that pluggable backend, so I am little confused about this PR, and unsure what exactly to recommend.

Could you share the product proposal or design docs associated with this PR? That would help me recommend a path forward.

jawad-khan · 2025-04-28T19:26:16Z

Hi @kdmccormick. I am thinking of this solution in edx-ace. By removing edx-braze-client from base.in in edx-ace we can achieve this. What do you think of this solution?

I'll try to collect any public documentation we have for you tomorrow.

muhammadadeeltajamul · 2025-04-29T07:35:24Z

openedx/core/djangoapps/notifications/tasks.py

    sender_id = context.pop('sender_id', None)
    default_web_config = get_default_values_of_preference(app_name, notification_type).get('web', False)
    generated_notification_audience = []
+    push_notification_audience = []
+    is_push_notification_enabled = ENABLE_PUSH_NOTIFICATIONS.is_enabled()


It is better to use CourseWaffleFlag here. This will give more control when testing on stage and on prod

muhammadadeeltajamul · 2025-04-29T07:43:13Z

openedx/core/djangoapps/notifications/tests/test_tasks.py

+        (settings.NOTIFICATION_CREATION_BATCH_SIZE, 10, 7),
+        (settings.NOTIFICATION_CREATION_BATCH_SIZE + 10, 12, 10),


I think query count should be +2 not +3. One query for fetching waffle flag and second to get list of emails.

Purpose of this test case is to make sure that NOTIFICATION_CREATION_BATCH_SIZE is optimized. It is better to keep changes in this test case to minimum. i.e. we can set push waffle flag to False and see that if it only increases 1 query count. If change is more than +1, we need to update batch size accordingly

muhammadadeeltajamul · 2025-04-29T07:44:47Z

openedx/core/djangoapps/notifications/push/tasks.py

+        app_label="notifications", name="braze_push"
+    ).personalize(recipient, 'en', context)
+    message.options['emails'] = emails
+    message.options['braze_campaign'] = notification_type


I think it is better to add

message.options['skip_disable_user_policy'] = True

See PR

muhammadadeeltajamul · 2025-04-29T07:46:27Z

openedx/core/djangoapps/notifications/tests/test_tasks.py

@@ -254,6 +258,7 @@ def test_send_with_app_disabled_notifications(self, app_name, notification_type)
        self.assertIsNone(notification)

    @override_waffle_flag(ENABLE_NOTIFICATIONS, active=True)
+    @override_waffle_flag(ENABLE_PUSH_NOTIFICATIONS, active=True)


For test cases, which don't require push notifications to be enabled, I think we should skip override_waffle_flag for them

kdmccormick · 2025-05-05T12:56:01Z

openedx/core/djangoapps/notifications/config/waffle.py

+# .. toggle_use_cases: temporary, open_edx
+# .. toggle_creation_date: 2025-02-11
+# .. toggle_target_removal_date: 2026-02-11
+# .. toggle_warning: When the flag is ON, Notifications will go through using braze on mobile devices.
+# .. toggle_tickets: LEARNER-10298


toggle_tickets must be a public link.

If this truly is a temporary toggle with target removal of 2026, then the linked ticket should give us maintainers a way to check on the status of this removal plan.

I think we don't need this toggle ticket here, I just followed template from other waffle flags. Also I don't think we need it temporarily. I'll update toggle info.

kdmccormick · 2025-05-05T12:56:53Z

openedx/core/djangoapps/notifications/push/tasks.py

+    if notification_object.app_name != 'discussion':
+        return


Please explain this with a comment.

kdmccormick · 2025-05-05T12:58:38Z

openedx/core/djangoapps/notifications/push/tasks.py

+        sender = User.objects.get(id=sender_id)
+        recipient = Recipient(sender.id, sender.email)


I'm probably just confused here, but could you explain why sender and recipient refer to the same user?

We need a Recipient object to call PushNotificationMessageType.personalize method, otherwise we don't need this recipient. I have added send here in case we might need sender in future in the context.
I'll add a comment to clear this confusion.

kdmccormick · 2025-05-05T13:14:21Z

openedx/core/djangoapps/notifications/push/tasks.py

+def send_ace_msg_to_braze_push_channel(audience_ids, notification_object, sender_id):
+    """
+    Send mobile notifications using ace braze_push channel.


[This is my primary review comment]

I am thinking of openedx/edx-enterprise#2352 in edx-ace. By removing edx-braze-client from base.in in edx-ace we can achieve this. What do you think of this solution?

Thank you for linking that. I don't think that would solve the underlying issue, which is: you are building push notifications support specifically for Braze users. It is not clear to me why you are doing that instead of building general push notification support, with Braze as a configurable backend.

However, maybe there is a piece of context I am missing? I would be happy to discuss the appropriateness of a generic approach vs. a Braze specific approach, but I cannot do that, because there is no public supporting information for this PR-- the product goal, the timeline, the details of the overall initative-- these are all unavailable to me or any other community member. Without support information, this will not merge.

@kdmccormick We are working on product proposal which we can make public for open source community.

Perfect, thank you!

@kdmccormick here's the proposal: https://openedx.atlassian.net/wiki/spaces/COMM/pages/4982079504/Push+Notifications+in+Mobile+Apps
(GitHub ticket also created following the proposal guidelines).

This is perfect Moiz, thank you.

@kdmccormick is there anything else required for this PR to be moved to merged?

@kdmccormick I have made this approach generic and its not linked with braze anymore. Let me know if anything else is needed.

jawad-khan force-pushed the jawad/LEARNER-10298 branch 4 times, most recently from 12f4356 to 5cdb65d Compare March 13, 2025 09:00

AhtishamShahid requested changes Mar 18, 2025

View reviewed changes

kdmccormick requested changes Mar 21, 2025

View reviewed changes

AhtishamShahid mentioned this pull request Apr 9, 2025

feat: Added audit access expiry soon notification #36414

Open

kdmccormick marked this pull request as draft April 9, 2025 14:49

jawad-khan mentioned this pull request Apr 14, 2025

Braze push notifications for mobile users openedx/edx-ace#347

Merged

12 tasks

jawad-khan force-pushed the jawad/LEARNER-10298 branch 2 times, most recently from 35e4b16 to bc28321 Compare April 24, 2025 18:43

jawad-khan marked this pull request as ready for review April 24, 2025 18:43

jawad-khan force-pushed the jawad/LEARNER-10298 branch from bc28321 to d060159 Compare April 24, 2025 18:47

muhammadadeeltajamul requested changes Apr 29, 2025

View reviewed changes

jawad-khan changed the title ~~feat: send mobile braze notifications~~ feat: send mobile braze notifications LEARNER-10298 Apr 30, 2025

jawad-khan added 7 commits May 5, 2025 16:33

feat: send mobile braze notifications

08caae2

fix: fixed pylint issues

1f9b031

feat: Added push_notification flag and preferences

9bcd513

fix: fixed pylint issues

1b99696

fix: Moved braze logic to edx-ace

6e88b38

fix: Un delete admin file

52289b0

fix: Added review suggestions

078e4b5

jawad-khan force-pushed the jawad/LEARNER-10298 branch from a8147ee to 078e4b5 Compare May 5, 2025 11:37

kdmccormick requested changes May 5, 2025

View reviewed changes

kdmccormick marked this pull request as draft May 5, 2025 13:14

fix: Added review suggestions

46d1d69

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: send mobile braze notifications LEARNER-10298 #36272

feat: send mobile braze notifications LEARNER-10298 #36272

jawad-khan commented Feb 19, 2025 •

edited

Loading

miankhalid commented Mar 10, 2025

AhtishamShahid Mar 18, 2025 •

edited

Loading

jawad-khan Mar 20, 2025

AhtishamShahid Mar 18, 2025 •

edited

Loading

kdmccormick left a comment

jawad-khan commented Apr 9, 2025

kdmccormick commented Apr 9, 2025

jawad-khan commented Apr 25, 2025

kdmccormick commented Apr 28, 2025

jawad-khan commented Apr 28, 2025

muhammadadeeltajamul Apr 29, 2025

muhammadadeeltajamul Apr 29, 2025

muhammadadeeltajamul Apr 29, 2025

muhammadadeeltajamul Apr 29, 2025

kdmccormick May 5, 2025

jawad-khan May 5, 2025

kdmccormick May 5, 2025

kdmccormick May 5, 2025

jawad-khan May 5, 2025

kdmccormick May 5, 2025

jawad-khan May 8, 2025

kdmccormick May 8, 2025

moiz994 May 9, 2025

kdmccormick May 9, 2025

moiz994 May 12, 2025

jawad-khan May 12, 2025

		send_braze_notification_to_mobile_users(push_notification_audience, generated_notification)


		def send_braze_notification_to_mobile_users(audience_ids, notification_object):

		(settings.NOTIFICATION_CREATION_BATCH_SIZE, 10, 7),
		(settings.NOTIFICATION_CREATION_BATCH_SIZE + 10, 12, 10),

		sender = User.objects.get(id=sender_id)
		recipient = Recipient(sender.id, sender.email)

feat: send mobile braze notifications LEARNER-10298 #36272

Are you sure you want to change the base?

feat: send mobile braze notifications LEARNER-10298 #36272

Conversation

jawad-khan commented Feb 19, 2025 • edited Loading

Description

Supporting information

Deadline

miankhalid commented Mar 10, 2025

AhtishamShahid Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AhtishamShahid Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

kdmccormick left a comment

Choose a reason for hiding this comment

jawad-khan commented Apr 9, 2025

kdmccormick commented Apr 9, 2025

jawad-khan commented Apr 25, 2025

kdmccormick commented Apr 28, 2025

jawad-khan commented Apr 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jawad-khan commented Feb 19, 2025 •

edited

Loading

AhtishamShahid Mar 18, 2025 •

edited

Loading

AhtishamShahid Mar 18, 2025 •

edited

Loading