feat: [FC-0047] add functionality to send push notifications #282
Conversation
|
Thanks for the pull request, @NiedielnitsevIvan! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
openedx-webhooks
added
the
open-source-contribution
mphilbrick211
added
the
needs test run
mphilbrick211
removed
the
needs test run
requirements/base.in
Outdated
| @@ -7,3 +7,6 @@ attrs>=17.2.0 # Attributes without boilerplate | |||
| sailthru-client==2.2.3 | |||
| six | |||
| stevedore>=1.10.0 | |||
|
|
|||
| firebase-admin==6.5.0 | |||
| django-push-notifications==3.1.0 | |||
There was a problem hiding this comment.
Is there a reason to pin these specific versions? It's different than what's specified in setup.py and we don't usually specify patch versions of things so that we can pick up bug fixes automatically.
There was a problem hiding this comment.
Fixed, and test added.
NiedielnitsevIvan
force-pushed
the
NiedielnitsevIvan/FC-0047/feature/implement-push-notifications-chanel
branch
2 times, most recently
from
885a6ae
to
39a15d5
Compare
There was a problem hiding this comment.
Thanks @NiedielnitsevIvan for this pull request. It's clean and concise.
I have added few questions, please check them and let me know what do you think.
edx_ace/ace.py
Outdated
| msg.report( | ||
| 'channel_skipped', | ||
| f'Skipping channel {channel_type}' | ||
| ) |
There was a problem hiding this comment.
If we're using msg.report without log_level=INFO or a similar mechanism, I'm concerned that this message would generate log noise given the vast quantity of skipped messages.
I think it's only useful if we use log_level or a similar mechanism to differentiates between different message levels for effective log filtering.
There was a problem hiding this comment.
Changed to log.info
edx_ace/ace.py
Outdated
| try: | ||
| rendered_message = presentation.render(channel, msg) | ||
| except TemplateDoesNotExist as error: | ||
| msg.report( | ||
| 'template_error', | ||
| str(error) | ||
| ) | ||
| continue |
There was a problem hiding this comment.
I see this pattern is used already elsewhere but it's missing logging stacktrace.
There was a problem hiding this comment.
Legging message added.
edx_ace/channel/__init__.py
Outdated
| return channel | ||
|
|
||
| # Else the normal path: use the preferred channel for this message type | ||
| return possible_channels[0] if possible_channels else channels_map.get_default_channel(channel_type) |
There was a problem hiding this comment.
I'm unsure if this is valid anymore to return a single channel for a message.
Would you mind elaborating on why this change was made?
Can we send the same message to both Email and Push?
There was a problem hiding this comment.
This is the get_channel_for_message function, which returns only one channel for the received type and message, both in the old implementation and in the new one.
I reworked this to avoid duplicating code when adding a new channel.
This way, messages can be sent via both Email and Push channels at once.
There was a problem hiding this comment.
@NiedielnitsevIvan I'm having a hard time understanding this function and its role after this change. I suppose we may need to have a get_channel[s]_for_message function instead.
I understand that it returns a single channel, but I don't think that makes sense anymore given that we send multi-channel messages.
edx_ace/channel/push_notification.py
Outdated
| """ | ||
| device_tokens = self.get_user_device_tokens(message.recipient.lms_user_id) | ||
| if not device_tokens: | ||
| LOG.info('Recipient %s has no push token. Skipping push notification.', message.recipient.email_address) |
There was a problem hiding this comment.
Is logging email_address okay to log?
There was a problem hiding this comment.
Changed to user ID in LMS
edx_ace/channel/push_notification.py
Outdated
| ).values_list('registration_id', flat=True)) | ||
|
|
||
| @staticmethod | ||
| def sanitize_html(html_str: str) -> str: |
There was a problem hiding this comment.
Why this method is needed?
Also, sanitize_html indicates that this method prevents XSS, but its job is mostly to compress spaces.
| def sanitize_html(html_str: str) -> str: | |
| def cleanup_spaces(html_str: str) -> str: |
or
| def sanitize_html(html_str: str) -> str: | |
| def compress_spaces(html_str: str) -> str: |
| try: | ||
| send_message(token, message, settings.FCM_APP_NAME) | ||
| except Exception as e: | ||
| LOG.exception('Failed to send push notification to %s', token) |
edx_ace/renderers.py
Outdated
| over an :attr:`.ChannelType.PUSH`. | ||
| """ | ||
|
|
||
| subject = attr.ib() |
There was a problem hiding this comment.
I think using subject would be confusing since it's an email term.
Using title is more appropriate and consistent with the mobile experience.
Sorry it'll require changing many variable and file names, but I hope it's useful.
| subject = attr.ib() | |
| title = attr.ib() |
NiedielnitsevIvan
force-pushed
the
NiedielnitsevIvan/FC-0047/feature/implement-push-notifications-chanel
branch
2 times, most recently
from
37d002c
to
005f945
Compare
|
Hi @bmtcril and @OmarIthawi, the questions from the first round of review were fixed and test coverage was increased, PR is ready for the second round of review |
|
@bmtcril @OmarIthawi All linters are green except coverage for the patch, I'll try to increase coverage, but I think 95.97% is still good coverage |
|
@GlugovGrGlib I'm working on it now, but it's a non-trivial set of testing. 😄 I'll offer feedback as soon as I can make it work locally, which should hopefully be today. |
|
@bmtcril Totally not easy to test it at all. Do you want me to provide you with sandbox environment and testing android or IOS app build, so it's possible to see implemented push notifications in action? This won't provide enough insights in customizability and extendability, but at least will allow to do e2e testing. |
|
@GlugovGrGlib I've got the app building and a tutor env set up with the right PRs, but are there any docs on the specific oauth settings and firebase side of the setup? I think I can do it from there. |
|
@bmtcril Sure, I'll try to collect and provide notes on the configuration! |
|
@bmtcril I have created a page with instructions on wiki, latter I'm planning to include it into edx-platform PR, so it will be available on docs.openedx.org |
|
Thanks for your work. I need more time for review due to travel, and I'll review before the conference. That being said, please don't block for my review. |
NiedielnitsevIvan
force-pushed
the
NiedielnitsevIvan/FC-0047/feature/implement-push-notifications-chanel
branch
from
a7b98af
to
7c58b64
Compare
|
@GlugovGrGlib @NiedielnitsevIvan thanks for the updated instructions, it helped a lot! I think I'm pretty close, but running into a couple of issues: 1- I'm unsure about this step in the testing instructions: "Add your device token to FCM Devices with a link to your user from the admin panel." I don't know what values are necessary for iOS here, it seems like registration ID is a thing we get from Firebase but I haven't been able to find out how. 2- When trying to test sending a message via the push notifications admin panel I'm getting:
|
I doubt it's possible to get this device token somehow manually, so you should either enable this view for token synchronization, or you can [log `storage.pushToken`](https://github.com/openedx/openedx-app-ios/pull/461/files#diff-64dbb104fcc6b0b00344cc23aabed82ea80fe678c363d6b53073693f904f1362R31) on your IOS device, and create FCM device in LMS admin panel manually.
|
…t-push-notifications-chanel
There was a problem hiding this comment.
@GlugovGrGlib Got it, I was able to do a successful end-to-end test. Looks good to me, but I think we should wait for another thumb from @OmarIthawi before merging since this isn't an area I'm very familiar with.
There was a problem hiding this comment.
Thanks @NiedielnitsevIvan. I've added many minor changes but my original question is still the same: https://github.com/openedx/edx-ace/pull/282/files#r1650104933
When I saw possible_channels[0] it appeared like an anti-pattern as we seamingly pick a random channel and return it.
What would this library look like without the get_channel_for_message function?
I think the reason of this confusion is that this function tries to combine a couple concerns into a single place: Return the proper channel for the message given a specific type, but also factor in the transactional parameter which complicates the logic a bit.
As a maintainer of this package, I totally support the feature being added and this pull request but I'd like us to take the oppurtunity to identify this bad function design and put a plan to refactor it -- whether now or immediately after the merger of this pull request.
What do you think?
cc: @e0d
edx_ace/ace.py
Outdated
| """ | ||
| msg.report_basics() | ||
|
|
||
| channels_for_message = policy.channels_for(msg) | ||
|
|
||
| for channel_type in channels_for_message: | ||
| if limit_to_channels and channel_type not in limit_to_channels: | ||
| log.info('Skipping channel %s', channel_type) |
There was a problem hiding this comment.
Perhaps we should lower the log in this case? I'm imagining this will spam logs due to the sheer amount of notifications being sent.
| log.info('Skipping channel %s', channel_type) | |
| log.debug('Skipping channel %s', channel_type) |
edx_ace/channel/__init__.py
Outdated
| @@ -170,27 +171,29 @@ def get_channel_for_message(channel_type, message): | |||
| Channel: The selected channel object. | |||
| """ | |||
| channels_map = channels() | |||
| channel_names = [] | |||
|
|
|||
| if channel_type == ChannelType.EMAIL: | |||
| if message.options.get('transactional'): | |||
| channel_names = [settings.ACE_CHANNEL_TRANSACTIONAL_EMAIL, settings.ACE_CHANNEL_DEFAULT_EMAIL] | |||
| else: | |||
| channel_names = [settings.ACE_CHANNEL_DEFAULT_EMAIL] | |||
|
|
|||
There was a problem hiding this comment.
Removed.
edx_ace/channel/__init__.py
Outdated
| return possible_channels[0] | ||
|
|
||
| return channels_map.get_default_channel(channel_type) | ||
| if channel_type == ChannelType.PUSH: |
There was a problem hiding this comment.
Costmetic changes:
| if channel_type == ChannelType.PUSH: | |
| elif channel_type == ChannelType.PUSH: |
edx_ace/channel/__init__.py
Outdated
| return channel | ||
|
|
||
| # Else the normal path: use the preferred channel for this message type | ||
| return possible_channels[0] if possible_channels else channels_map.get_default_channel(channel_type) |
There was a problem hiding this comment.
@NiedielnitsevIvan I'm having a hard time understanding this function and its role after this change. I suppose we may need to have a get_channel[s]_for_message function instead.
I understand that it returns a single channel, but I don't think that makes sense anymore given that we send multi-channel messages.
| """ | ||
| Test that the channel is enabled when the settings are configured. | ||
| """ | ||
| self.assertTrue(PushNotificationChannel.enabled()) |
There was a problem hiding this comment.
| self.assertTrue(PushNotificationChannel.enabled()) | |
| assert PushNotificationChannel.enabled() |
There was a problem hiding this comment.
assert works much better with pytest by providing great debugging messages. Please avoid using self.assert... unless we have to.
|
|
||
| channel = PushNotificationChannel() | ||
|
|
||
| with self.assertRaises(FatalChannelDeliveryError): |
There was a problem hiding this comment.
| with self.assertRaises(FatalChannelDeliveryError): | |
| with pytest.raises(FatalChannelDeliveryError): |
| self.assertIsInstance(apns_config, APNSConfig) | ||
| self.assertEqual(apns_config.headers['apns-priority'], '5') | ||
| self.assertEqual(apns_config.headers['apns-push-type'], 'alert') |
There was a problem hiding this comment.
| self.assertIsInstance(apns_config, APNSConfig) | |
| self.assertEqual(apns_config.headers['apns-priority'], '5') | |
| self.assertEqual(apns_config.headers['apns-push-type'], 'alert') | |
| assert isinstance(apns_config, APNSConfig) | |
| assert apns_config.headers['apns-priority'] == '5' | |
| assert apns_config.headers['apns-push-type'] == 'alert' |
|
|
||
| channel = PushNotificationChannel() | ||
| tokens = channel.get_user_device_tokens(self.lms_user_id) | ||
| self.assertEqual(tokens, [gcm_device.registration_id]) |
| """ | ||
| channel = PushNotificationChannel() | ||
| tokens = channel.get_user_device_tokens(self.lms_user_id) | ||
| self.assertEqual(tokens, []) |
| @@ -16,7 +15,7 @@ class TestRender(TestCase): | |||
|
|
|||
| def test_missing_renderer(self): | |||
| channel = Mock( | |||
| channel_type=ChannelType.PUSH, | |||
| channel_type='unsupported_channel_type' | |||
This behavior with |
NiedielnitsevIvan
force-pushed
the
NiedielnitsevIvan/FC-0047/feature/implement-push-notifications-chanel
branch
from
84c09dc
to
b18c55a
Compare
There was a problem hiding this comment.
Thanks @NiedielnitsevIvan for taking the time to answer every review note and your patience for the time it took me to review.
Admittedly, the original code has confusing logic but it's probably out of the scope of this pull request. This function should have had if statements instead of array logic to make it clearer and ease code testing.
I think it makes sense not to block this code due to the logic has been written before.
Thank you so much, and let me know when you'd like me to merge the pull request.
edx_ace/channel/__init__.py
Outdated
| return channel | ||
|
|
||
| # Else the normal path: use the preferred channel for this message type | ||
| return possible_channels[0] if possible_channels else channels_map.get_default_channel(channel_type) |
There was a problem hiding this comment.
Minor style changes to improve readability.
| return possible_channels[0] if possible_channels else channels_map.get_default_channel(channel_type) | |
| if possible_channels: | |
| return possible_channels[0] | |
| else: | |
| return channels_map.get_default_channel(channel_type) |
There was a problem hiding this comment.
Thank you for your review, I have corrected the last comment, so the PR is ready to be merged and released for further use in this PR.
NiedielnitsevIvan
force-pushed
the
NiedielnitsevIvan/FC-0047/feature/implement-push-notifications-chanel
branch
2 times, most recently
from
c4a3459
to
9a8e9fe
Compare
NiedielnitsevIvan
force-pushed
the
NiedielnitsevIvan/FC-0047/feature/implement-push-notifications-chanel
branch
from
9a8e9fe
to
1026e58
Compare
|
Thanks @NiedielnitsevIvan! Please bump up the version to 1.10.0 and I'll squash and merge. |
Thanks, ready. |
|
@NiedielnitsevIvan 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
Thanks @NiedielnitsevIvan @GlugovGrGlib @bmtcril for making this happen. Code reviews might show the opposite, but I'm really happy to see this getting merged. Only with this feature edX ACE is getting utilized. Otherwise, it was a mostly a useless abstraction 😅 |
Description:
This PR is addressed at adding a new channel that can send push notifications to the OeX mobile app.
A more detailed description can be found in the relevant ADR here - #277
JIRA: FC-0047
Dependencies: openedx/edx-platform#34971
Testing instructions:
push_notificationchannel to ACE_ENABLED_CHANEL.Reviewers:
Merge checklist:
Post merge:
finished.
Author concerns:
This PR and the PR that adds the settings for the firebase-admin and django-push-notification package in edx-platform should be merged at the same time to avoid errors.