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.

Sign up for GitHub

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

Jump to bottom

feat: [AXM-252] add settings for edx-ace push notifications #2541

Merged
merged 12 commits into from
Apr 29, 2024
Merged

feat: [AXM-252] add settings for edx-ace push notifications #2541

merged 12 commits into from
Apr 29, 2024

Conversation

NiedielnitsevIvan
Copy link

feat: [AXM-252] add settings for edx-ace push notifications

@NiedielnitsevIvan NiedielnitsevIvan self-assigned this Apr 16, 2024
@NiedielnitsevIvan NiedielnitsevIvan force-pushed the Ivan_Niedielnitsev/AXM-252/feature/Prepare-edx-platform-to-use-push-notifications-with-edx-ace branch 5 times, most recently from 7cfb4e5 to ad69d41 Compare April 23, 2024 08:49
@NiedielnitsevIvan NiedielnitsevIvan force-pushed the Ivan_Niedielnitsev/AXM-252/feature/Prepare-edx-platform-to-use-push-notifications-with-edx-ace branch 2 times, most recently from 607f860 to 6ffdce6 Compare April 23, 2024 15:16
@NiedielnitsevIvan NiedielnitsevIvan marked this pull request as ready for review April 24, 2024 10:03
monteri
monteri reviewed Apr 24, 2024
View reviewed changes
lms/djangoapps/mobile_api/notifications/views.py

def create(self, request, *args, **kwargs):
if not getattr(settings, 'PUSH_NOTIFICATIONS_SETTINGS', None):
return Response('Push notifications are not configured.', status.HTTP_501_NOT_IMPLEMENTED)
Copy link

Choose a reason for hiding this comment

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

👍

openedx/core/djangoapps/ace_common/settings/common.py Outdated
@@ -22,3 +25,29 @@ def plugin_settings(settings): # lint-amnesty, pylint: disable=missing-function
settings.ACE_ROUTING_KEY = ACE_ROUTING_KEY

settings.FEATURES['test_django_plugin'] = True
settings.FCM_APP_NAME = 'fcm-edx-platform'

if getattr(settings, 'FIREBASE_SETTING_UP', None) is None:
Copy link

Choose a reason for hiding this comment

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

This variable name means Firebase setup status? Maybe something like FIREBASE_SETUP_STATUS will work here?

Copy link
Author

Choose a reason for hiding this comment

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

Yep, renamed.

openedx/core/djangoapps/ace_common/utils.py Outdated
@@ -0,0 +1,16 @@
"""
Utility functions for edx ace.
Copy link

Choose a reason for hiding this comment

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

Suggested change
Utility functions for edx ace.
Utility functions for edx-ace.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

openedx/core/djangoapps/ace_common/utils.py
"""
try:
import firebase_admin # pylint: disable=import-outside-toplevel
except ImportError:
Copy link

Choose a reason for hiding this comment

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

Maybe log here with text: "Could not import firebase_admin package"?

Copy link
Author

Choose a reason for hiding this comment

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

Logs added.

openedx/core/djangoapps/notifications/policies.py Outdated
course_key
)

if not course_notif_preference.get_notification_type_config(app_name, notification_type).get('push', False):
Copy link

Choose a reason for hiding this comment

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

Suggested change
if not course_notif_preference.get_notification_type_config(app_name, notification_type).get('push', False):
if not course_notification_preference.get_notification_type_config(app_name, notification_type).get('push', False):

Copy link

Choose a reason for hiding this comment

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

Maybe create a variable before the if statement to reduce the condition size and provide more clarity here

Copy link
Author

Choose a reason for hiding this comment

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

Done

@NiedielnitsevIvan NiedielnitsevIvan force-pushed the Ivan_Niedielnitsev/AXM-252/feature/Prepare-edx-platform-to-use-push-notifications-with-edx-ace branch from 7ee15a8 to ec8c7fa Compare April 25, 2024 07:19
@NiedielnitsevIvan NiedielnitsevIvan mentioned this pull request Apr 26, 2024
Merged
@NiedielnitsevIvan NiedielnitsevIvan force-pushed the Ivan_Niedielnitsev/AXM-252/feature/Prepare-edx-platform-to-use-push-notifications-with-edx-ace branch 2 times, most recently from 4f01cbb to d979145 Compare April 26, 2024 14:08
@monteri monteri force-pushed the Ivan_Niedielnitsev/AXM-252/feature/Prepare-edx-platform-to-use-push-notifications-with-edx-ace branch from d979145 to 81ebaa4 Compare April 29, 2024 13:41
@monteri monteri merged commit dd1844b into mob-develop Apr 29, 2024
62 checks passed
GlugovGrGlib pushed a commit that referenced this pull request May 2, 2024
@NiedielnitsevIvan @GlugovGrGlib
feat: [AXM-252] add settings for edx-ace push notifications (#2541)
011711e 
* feat: [AXM-252] create policy for push notifications

* feat: [AXM-252] add API for store device token

* feat: [AXM-252] add settings for edx-ace push notifications

* chore: [AXM-252] add edx-ace and django-push-notification to dev requirements

* chore: [AXM-252] update edx-ace version

* fix: [AXM-252] add create token edndpoint to urls

* chore: [AXM-252] update django push notifications version

* style: [AXM-252] fix code style issues after review

* chore: [AXM-252] bump edx-ace version

* refactor: [AXM-252] some push notif policy refactoring

* chore: [AXM-252] change edx-ace branch to mob-develop

* chore: [AXM-252] recompile requirements after rebase
monteri pushed a commit that referenced this pull request May 22, 2024
@NiedielnitsevIvan @vzadorozhnii
feat: [AXM-252] add settings for edx-ace push notifications (#2541)
c987e7e 
* feat: [AXM-252] create policy for push notifications

* feat: [AXM-252] add API for store device token

* feat: [AXM-252] add settings for edx-ace push notifications

* chore: [AXM-252] add edx-ace and django-push-notification to dev requirements

* chore: [AXM-252] update edx-ace version

* fix: [AXM-252] add create token edndpoint to urls

* chore: [AXM-252] update django push notifications version

* style: [AXM-252] fix code style issues after review

* chore: [AXM-252] bump edx-ace version

* refactor: [AXM-252] some push notif policy refactoring

* chore: [AXM-252] change edx-ace branch to mob-develop

* chore: [AXM-252] recompile requirements after rebase
NiedielnitsevIvan added a commit that referenced this pull request May 27, 2024
@NiedielnitsevIvan
feat: [AXM-252] add settings for edx-ace push notifications (#2541)
97392f5 
* feat: [AXM-252] create policy for push notifications

* feat: [AXM-252] add API for store device token

* feat: [AXM-252] add settings for edx-ace push notifications

* chore: [AXM-252] add edx-ace and django-push-notification to dev requirements

* chore: [AXM-252] update edx-ace version

* fix: [AXM-252] add create token edndpoint to urls

* chore: [AXM-252] update django push notifications version

* style: [AXM-252] fix code style issues after review

* chore: [AXM-252] bump edx-ace version

* refactor: [AXM-252] some push notif policy refactoring

* chore: [AXM-252] change edx-ace branch to mob-develop

* chore: [AXM-252] recompile requirements after rebase
NiedielnitsevIvan added a commit that referenced this pull request Jun 10, 2024
@NiedielnitsevIvan
feat: [AXM-252] add settings for edx-ace push notifications (#2541)
81ff17b 
* feat: [AXM-252] create policy for push notifications

* feat: [AXM-252] add API for store device token

* feat: [AXM-252] add settings for edx-ace push notifications

* chore: [AXM-252] add edx-ace and django-push-notification to dev requirements

* chore: [AXM-252] update edx-ace version

* fix: [AXM-252] add create token edndpoint to urls

* chore: [AXM-252] update django push notifications version

* style: [AXM-252] fix code style issues after review

* chore: [AXM-252] bump edx-ace version

* refactor: [AXM-252] some push notif policy refactoring

* chore: [AXM-252] change edx-ace branch to mob-develop

* chore: [AXM-252] recompile requirements after rebase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monteri monteri Awaiting requested review from monteri

Assignees

@NiedielnitsevIvan NiedielnitsevIvan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@NiedielnitsevIvan @monteri