notifications: add user preferences filter #65
Conversation
rekt-hard
force-pushed
the
3-introduce-a-new-userpreferencesnotifications-configuration
branch
2 times, most recently
from
f854d4d to
f4c0d44
Compare
rekt-hard
force-pushed
the
3-introduce-a-new-userpreferencesnotifications-configuration
branch
from
f4c0d44 to
2d60ff5
Compare
There was a problem hiding this comment.
LGTM, some styling/alignment comments.
In general, I have a feeling though that we might need to flesh-out the Recipient schema fields a bit more... Right now there's this very fluffy data field, which makes it a bit hard for a Backend e.g. to know for sure where to fetch from email, full_name, etc. Definitely not to be addressed right now in this PR, but I'll open an issue in invenio-notifications so we can keep track of it there better.
| if user.get("preferences", {}).get("notifications", {}).get("enabled", True): | ||
| recipients[user["id"]] = Recipient(data=user) |
There was a problem hiding this comment.
| if user.get("preferences", {}).get("notifications", {}).get("enabled", True): | |
| recipients[user["id"]] = Recipient(data=user) | |
| recipients[user["id"]] = Recipient(data=user) |
minor: since we have also the equivalent separate filter above
There was a problem hiding this comment.
Do we want to apply some filtering here as well or leave it to the specified RecipientFilters of the NotificationBuilder completely? So should the filters be responsible for all the filtering or just extra filtering?
schemas: add notifications schema
notifications: update UserPreferencesFilter to work with dict tests: add test for UserPreferencesFilter
resolvers: refactor import
rekt-hard
force-pushed
the
3-introduce-a-new-userpreferencesnotifications-configuration
branch
from
2d60ff5 to
2dcc0c4
Compare
This class did not provide any additional implementation details and could be easily reconstructed.
tests/test_notifications.py
Outdated
| u = UserAggregate.from_user(user_pub.user).dumps() | ||
|
|
||
| user_notifications_enabled = deepcopy(u) | ||
| user_notifications_enabled["preferences"]["notifications"] = {"enabled": True} |
There was a problem hiding this comment.
minor: could we maybe override the ACCOUNTS_USER_PREFERENCES_SCHEMA in conftest.py to integrate NotificationPreferences to test this?
There was a problem hiding this comment.
adapted conftest.py to use a preferences schema with NotificationPreferences.
records: add default value for notifications when parsing user data
rekt-hard
force-pushed
the
3-introduce-a-new-userpreferencesnotifications-configuration
branch
from
29d1d91 to
d21fd1d
Compare
| @@ -85,6 +85,13 @@ | |||
| "timezone": { | |||
| "type": "keyword", | |||
| "index":"false" | |||
| }, | |||
| "notifications": { | |||
There was a problem hiding this comment.
could you add this index name to the changelog inveniosoftware/docs-invenio-rdm#536 ? we need to remember to add this to the migration recipe
schemas: add notifications schema
❤️ Thank you for your contribution!
Description
closes inveniosoftware/invenio-notifications#3
Add notification schema to be used in
invenio-app-rdmAdd notification recipient filter for user preferences
Checklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
Third-party code
If you've added third-party code (copy/pasted or new dependencies), please reach out to an architect.
Reminder
By using GitHub, you have already agreed to the GitHub’s Terms of Service including that: