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

Closes #15621: User notifications #16800

Merged
merged 34 commits into from
Jul 15, 2024
Merged

Closes #15621: User notifications #16800

merged 34 commits into from
Jul 15, 2024

Conversation

jeremystretch
Copy link
Member

@jeremystretch jeremystretch commented Jul 3, 2024

Closes: #15621

  • Implement dynamic registration for event types
  • Add event types for job failures and errors
  • Alter event handling logic to enqueue events by canonical type rather than by the corresponding ObjectChange action type
  • Introduce three new models: Subscription, NotificationGroup, and Notification
  • Add post_save signal receiver to automatically generate notification for subscribed users
  • Extend EvenRule to support sending notifications
  • Add notifications dropdown in top navigation bar
  • Provide user views for subscriptions & notifications
  • Add model documentation

@jeremystretch jeremystretch added this to the v4.1 milestone Jul 3, 2024
@jeremystretch jeremystretch marked this pull request as ready for review July 9, 2024 00:48
@jeremystretch jeremystretch requested a review from arthanson July 9, 2024 00:48
@arthanson
Copy link
Collaborator

Getting the following when checkout the code:

django.core.management.base.SystemCheckError: SystemCheckError: System check identified some issues:

ERRORS:
extras.Notification.event_type: (fields.E004) 'choices' must be an iterable (e.g., a list or tuple).

Copy link
Collaborator

@arthanson arthanson left a comment

Choose a reason for hiding this comment

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

See previous comment on getting SystemCheckError wasn't able to actually run it to test.

Also, should add model docs for the new models?

netbox/core/events.py Show resolved Hide resolved
@jeremystretch jeremystretch requested a review from arthanson July 11, 2024 16:02
@arthanson

This comment was marked as resolved.

@arthanson

This comment was marked as resolved.

@arthanson

This comment was marked as resolved.

Copy link
Collaborator

@arthanson arthanson left a comment

Choose a reason for hiding this comment

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

Several crashes

netbox/extras/forms/bulk_import.py Outdated Show resolved Hide resolved
netbox/extras/signals.py Show resolved Hide resolved
@arthanson
Copy link
Collaborator

There is no confirmation of delete on a notification. Not very critical if you delete it and you don't want, but everything else in NetBox has a confirmation on delete so not sure if this was by design.

@jeremystretch
Copy link
Member Author

jeremystretch commented Jul 12, 2024

There is no confirmation of delete on a notification.

Yeah, I modeled it after GitHub notifications: Clicking through to an object will mark the notification as "read," but not delete it. Deletion is a manual task, but does not require confirmation, so that notifications can be easily dismissed.

(I'm open to feedback about this during the beta, but suspect this approach will satisfy most users.)

Copy link
Collaborator

@arthanson arthanson left a comment

Choose a reason for hiding this comment

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

There is just one minor UI in the notification list there is an info icon that I think isn't supposed to be there (see screenshot) otherwise looks good:
Notifications | NetBox 2024-07-15 21-03-18

@jeremystretch
Copy link
Member Author

I added the icon to help convey each notification's urgency. We're currently using only info, but success, warning, and error are available as well.

@jeremystretch jeremystretch merged commit b0e7294 into feature Jul 15, 2024
6 checks passed
@jeremystretch jeremystretch deleted the 15621-notifications branch July 15, 2024 18:24
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants