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

Added signal for notifications #234

Merged
merged 1 commit into from
Jun 23, 2023
Merged

Added signal for notifications #234

merged 1 commit into from
Jun 23, 2023

Conversation

AhtishamShahid
Copy link
Contributor

@AhtishamShahid AhtishamShahid commented Jun 12, 2023 •
edited
Loading

Description:
Thir PR adds new signal USER_NOTIFICATION_REQUESTED It will be used to create notifications for users.

ISSUE: Link to GitHub issue
https://2u-internal.atlassian.net/browse/INF-876

Reviewers:

  • tag reviewer
  • tag reviewer

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Commits are squashed

Post merge:

  • Create a tag
  • Check new version is pushed to PyPI after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

@AhtishamShahid AhtishamShahid requested a review from a team as a code owner June 12, 2023 06:50
@mariajgrimaldi
Copy link
Member

Is this ready for our review? can you please fill in the PR cover letter? Thanks!

@AhtishamShahid AhtishamShahid marked this pull request as draft June 12, 2023 13:28
@AhtishamShahid
Copy link
Contributor Author

Is this ready for our review? can you please fill in the PR cover letter? Thanks!

Some tests are failing so I converted it to a draft. Meanwhile, I will also add a description for this PR.

@AhtishamShahid AhtishamShahid marked this pull request as ready for review June 13, 2023 08:27
@AhtishamShahid AhtishamShahid mentioned this pull request Jun 13, 2023
Merged
@asadazam93
Copy link

@mariajgrimaldi The PR is ready for review

@AhtishamShahid AhtishamShahid changed the title Ahtisham/inf 876 Added signal for notifications Jun 16, 2023
rgraber
rgraber suggested changes Jun 20, 2023
View reviewed changes
openedx_events/learning/data.py Show resolved Hide resolved
openedx_events/learning/data.py Show resolved Hide resolved
openedx_events/learning/signals.py Outdated
# Warning: This event is currently incompatible with the event bus, list/dict cannot be serialized yet
#
USER_NOTIFICATION = OpenEdxPublicSignal(
event_type="org.openedx.notifications.user_notification.v1",
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is in the learning domain (which I think it is) it should start with org.openedx.learning
I also think a name like org.openedx.learning.user.notification.sent would be a bit more in keeping with how we've named other things. We could call the event USER_NOTIFICATION_SENT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This signal is supposed to be used for sending notifications, it is not triggered after notification is sent. I am updating name like this org.openedx.learning.user.notification

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should clarify a little more about what this event means. Our other event names are all of the form "something happened," so I think we should be consistent. Robert suggested "USER_NOTIFICATION_REQUESTED" which I think makes sense. That would make the event type org.openedx.learning.user.notification.requested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated .

rgraber
rgraber reviewed Jun 20, 2023
View reviewed changes
CHANGELOG.rst Outdated
@@ -14,6 +14,12 @@ Change Log
Unreleased
----------

[8.1.1] - 2023-06-08
Copy link
Contributor

Choose a reason for hiding this comment

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

I would call this 8.2.0. usually x.x.1 means a bug patch rather than a new feature or model

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

rgraber
rgraber approved these changes Jun 22, 2023
View reviewed changes
Copy link
Contributor

@rgraber rgraber left a comment

Choose a reason for hiding this comment

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

One nit, but otherwise LGTM

@AhtishamShahid
feat: added user notifcation signal
520f222 
feat: added new version and changelog

fix: added notification signal to KNOWN_UNSERIALIZABLE_SIGNALS

feat: added course_id in notification data

fix: updated version and event name

fix: added updated event name in KNOWN_UNSERIALIZABLE_SIGNALS

refactor: updated notification name

refactor: changed name notificaiotns to notification
@rgraber rgraber merged commit 8788be9 into openedx:main Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asadazam93 asadazam93 asadazam93 approved these changes

@rgraber rgraber rgraber approved these changes

@saadyousafarbi saadyousafarbi Awaiting requested review from saadyousafarbi

@mariajgrimaldi mariajgrimaldi Awaiting requested review from mariajgrimaldi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AhtishamShahid @mariajgrimaldi @asadazam93 @rgraber