-
-
Notifications
You must be signed in to change notification settings - Fork 528
Nextcloud group notification implementation #1440
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 PR adds support for targeting groups and all users in Nextcloud notifications, expanding the plugin's functionality to resolve special target patterns into individual user notifications.
Key changes:
- Added
_resolve_targets()method to expandgroup:<name>andallkeywords into individual user targets using Nextcloud's OCS provisioning API - Enhanced documentation to explain the new targeting features
- Added comprehensive test coverage for group and all-user expansion
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| apprise/plugins/nextcloud.py | Implements group/all-user expansion via OCS API with JSON/XML fallback parsing |
| tests/test_plugin_nextcloud.py | Adds test cases for group and all-user target resolution with mocked API responses |
| README.md | Documents new targeting syntax and requirements for using groups and all-user expansion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1440 +/- ##
==========================================
- Coverage 99.65% 99.59% -0.06%
==========================================
Files 174 174
Lines 22561 22648 +87
Branches 3590 3604 +14
==========================================
+ Hits 22483 22557 +74
- Misses 70 79 +9
- Partials 8 12 +4
🚀 New features to boost your workflow:
|
caronc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean... Is this working for you right now?
The code tries multiple endpoints and trys to parse in XML and falls gracefully...
I don't use nextcloud personally, so I can't make a good judgement call. But it just seems very odd... Is each version of the tool so different then the next, that so many workarounds need to be tested to get the list of users?
|
Yeah, I used local NextCloud for testing and it worked well. The concern here is complexity due to several fallbacks. |
|
Thanks for the review — I simplified the resolver as suggested. It now uses only the v1 OCS endpoints with JSON:
|
|
@caronc _resolve_targets simplified a lot and you can easily get the gist out of it now. |
|
I did a little refactoring to your code (not much); still need to add more test coverage; but i can help with this. I introduced the use of caching so the user lists do not have to fetch over and over again once already known |
Description:
Related issue (if applicable): #1111
Add group and everyone targeting to the Nextcloud plugin:
Nextcloud Notifications (Enhancement)
This enhancement allows sending notifications to:
Internally, Apprise expands group/everyone to user IDs via the OCS provisioning API (over HTTP or HTTPS, depending on ncloud/nclouds) and sends one notification per unique user.
🛠️ Setup Instructions
📦 Examples
Users:
Group:
Everyone:
Mixed (deduplicated):
Sub-path:
Checklist
Contribution by Gittensor, learn more at https://gittensor.io/