Skip to content

[Refactor] Notification plugins #9735

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

Merged
merged 104 commits into from
Jul 26, 2025

Conversation

SchrodingersGat
Copy link
Member

@SchrodingersGat SchrodingersGat commented Jun 3, 2025

Breaking Change

This PR represents a breaking change to any custom plugin which implements notification functionality. To continue to function, any such plugin must be reworked to follow the new approach:

  • Each custom notification method must be implemented as a separate plugin
  • Each custom notification method must inherit the NotificationMixin class
  • Refer to the 3x built-in notification plugins for examples

Details

This PR is a refactor of how notification methods are defined and collected.

Currently, it is a separate collection mechanism to plugins, and loading the notification methods is complex and somewhat "brittle" (compared to the plugin registry which has been optimized),

The intent here is to just make each notification method a plugin, which is then in line with our general concept of plugins. It also ensures that notification methods cannot be "collected" from plugins which are installed but not active (which is unfortunately the case currently).

Feature - Plugin User Settings

It introduces a new concept, a "per user" setting structure against plugins. This replaces the "NotificationSetting" which was stored once per user against each type of notification. The new approach is more generic as any plugin can store per-user settings.

Enhancements

  • Improve query efficiency when loading and checking plugin registry
  • Fix for "stock required for build" event - pass to the notification systems now

Tasks

  • Split built-in notification methods into separate plugins
  • Documentation for new built-in plugin types
  • Reimplement solution for per-user-per-plugin settings
  • Fix unit testing
  • Check that email notifications still work
  • Check that UI notification still work
  • API unit tests for new plugin-user-setting
  • Playwright tests for configuring plugin user settings
  • Fix remaining "notification settings" user settings panel

Related Issues

@SchrodingersGat SchrodingersGat added this to the 1.0.0 milestone Jun 3, 2025
@SchrodingersGat SchrodingersGat requested a review from matmair as a code owner June 3, 2025 23:58
Copy link

netlify bot commented Jun 3, 2025

Deploy Preview for inventree-web-pui-preview ready!

Name Link
🔨 Latest commit f125dbb
🔍 Latest deploy log https://app.netlify.com/projects/inventree-web-pui-preview/deploys/688325edc7208c00083668d1
😎 Deploy Preview https://deploy-preview-9735--inventree-web-pui-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 97 (no change from production)
Accessibility: 81 (no change from production)
Best Practices: 100 (no change from production)
SEO: 78 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify project configuration.

@SchrodingersGat SchrodingersGat added plugin Plugin ecosystem refactor labels Jun 3, 2025
@SchrodingersGat SchrodingersGat marked this pull request as draft June 3, 2025 23:58
@SchrodingersGat SchrodingersGat added the breaking Indicates a major update or change which breaks compatibility label Jun 4, 2025
- Ensure plugin slug is correctly attached
- Consistent format
- Logic fixes
Copy link

codecov bot commented Jun 4, 2025

Codecov Report

❌ Patch coverage is 92.88136% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.78%. Comparing base (6e81fa2) to head (f125dbb).
⚠️ Report is 2 commits behind head on master.

❌ Your patch status has failed because the patch coverage (83.78%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.
❌ Your project status has failed because the head coverage (91.61%) is below the target coverage (92.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9735      +/-   ##
==========================================
- Coverage   86.82%   86.78%   -0.05%     
==========================================
  Files        1251     1253       +2     
  Lines       55398    55331      -67     
  Branches     2089     2089              
==========================================
- Hits        48102    48019      -83     
- Misses       6783     6799      +16     
  Partials      513      513              
Flag Coverage Δ
backend 88.48% <92.88%> (-0.06%) ⬇️
migrations 42.82% <42.71%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
Backend Apps 91.20% <94.97%> (+0.01%) ⬆️
Backend General 91.61% <83.78%> (-0.84%) ⬇️
Frontend 70.11% <ø> (+0.01%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@matmair matmair left a comment

Choose a reason for hiding this comment

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

TBH there are so many lost / fundamentally changed mechanisms that I can not compare this to the current implementation as it just shares some entry points and names.
This means a new write of every even slightly complex notification method plugin and refactors for all plugins that trigger notifications.

@SchrodingersGat
Copy link
Member Author

@matmair fair comments - I'll see if I can adjust this to reduce the number of changes required

@SchrodingersGat SchrodingersGat added the bug Identifies a bug which needs to be addressed label Jun 9, 2025
- Allows plugins to define per-user settings
@SchrodingersGat
Copy link
Member Author

I think the difference in code coverage here is due to deleted lines in the codebase....

In any case, @matmair this is ready for review now. The key points are:

  • Notification methods are now delivered by "normal" plugin approach
  • Yes, this is a breaking change (which is why I want to get it done now, before 1.0.0)
  • Plugins can now specify "user-specific settings". This replaces the concept of "notification setting" which was specific to a notificationmethod

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request refactors the notification system to treat notification methods as individual plugins instead of being part of a larger plugin. It implements a new user-specific settings system for plugins, changes the notification architecture to use the NotificationMixin, and replaces the NotificationUserSetting model with PluginUserSetting.

  • Splits core notification methods (UI, Email, Slack) into separate plugins
  • Introduces plugin-specific user settings through new PluginUserSetting model
  • Replaces the legacy notification collection system with plugin-based approach

Reviewed Changes

Copilot reviewed 57 out of 57 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/frontend/tests/pui_plugins.spec.ts Adds new Playwright test for plugin user settings functionality
src/frontend/src/states/SettingsStates.tsx Updates plugin settings state to accept endpoint parameter
src/frontend/src/pages/Index/Settings/UserSettings.tsx Adds Plugin Settings tab to user settings
src/frontend/src/pages/Index/Settings/UserPluginSettings.tsx New component for managing user-specific plugin settings
src/frontend/src/components/settings/SettingList.tsx Adds support for plugin user settings and onLoaded callback
src/frontend/lib/enums/ApiEndpoints.tsx Adds new API endpoint for plugin user settings
src/backend/InvenTree/users/ruleset.py Updates ruleset to reference new PluginUserSetting model
src/backend/InvenTree/templates/email/*.html New and updated email templates
src/backend/InvenTree/plugin/serializers.py Replaces NotificationUserSetting with PluginUserSetting serializer
src/backend/InvenTree/plugin/samples/integration/sample.py Adds example user settings to sample plugin
src/backend/InvenTree/plugin/registry.py Updates built-in plugin names for new notification structure
src/backend/InvenTree/plugin/plugin.py Adds NOTIFICATION mixin enum
src/backend/InvenTree/plugin/models.py Replaces NotificationUserSetting with PluginUserSetting model
src/backend/InvenTree/plugin/migrations/ Database migrations for model changes
src/backend/InvenTree/plugin/builtin/integration/core_notifications.py Refactors notification methods into separate plugin classes
src/backend/InvenTree/plugin/base/integration/SettingsMixin.py Adds user settings support to SettingsMixin
src/backend/InvenTree/plugin/base/integration/NotificationMixin.py New mixin for notification plugins
src/backend/InvenTree/plugin/api.py Adds API endpoints for plugin user settings
src/backend/InvenTree/plugin/admin.py Updates admin interface for new models
src/backend/InvenTree/common/notifications.py Refactors notification triggering to use plugin system
src/backend/InvenTree/build/tasks.py Updates build notification to use new system
Multiple test files Updates tests to work with new notification architecture
docs/ Documentation updates for new notification plugins and mixins

@matmair
Copy link
Contributor

matmair commented Jul 21, 2025

TBH I am still not a fan of this change, I feel like notification settings should be a first-party component just like other settings. Moving them to the plugins makes them far more obscure.
The rest (except coverage) looks fine.

@matmair
Copy link
Contributor

matmair commented Jul 21, 2025

The new plugin user settings seem to not be covered fully, there seems to be stuff missing on the APIs

@SchrodingersGat
Copy link
Member Author

SchrodingersGat commented Jul 23, 2025

TBH I am still not a fan of this change, I feel like notification settings should be a first-party component just like other settings.

I see what you mean, but the plugin specific notification settings are controlled exclusively by the plugins which implement them.

What about a compromise:

First-Party Settings

  • Add some specific "first party" user settings for notifications (such as "enable emails" / "enable UI notifications")
  • First party plugins reference these settings, rather than their own "plugin specific" settings
  • These settings are placed in the "user notification settings" panel
  • Any other "third party" plugins which provide notification support will have their settings appear in the "plugin settings" panel

Edit

I came up with a cleaner way - display all user settings for notification plugins in the "user notifications" tab:

image

This approach also works nicely for global plugin settings:

image

@SchrodingersGat
Copy link
Member Author

@matmair coverage has been increased across new features, and the notification settings now appear in their own panel

@SchrodingersGat SchrodingersGat merged commit 1085625 into inventree:master Jul 26, 2025
30 of 33 checks passed
@SchrodingersGat SchrodingersGat deleted the notification-plugins branch July 26, 2025 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Relates to the API breaking Indicates a major update or change which breaks compatibility bug Identifies a bug which needs to be addressed full-run Always do a full QC CI run migration Data or schema migrations plugin Plugin ecosystem refactor User Interface Related to the frontend / User Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants