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

[Feature] Custom notification settings #1272

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

[Feature] Custom notification settings #1272

wants to merge 5 commits into from

Conversation

BentiGorlich
Copy link
Member

You can now set custom notification settings for users, magazine, entries and posts: Loud, Default or Muted.

The heart of the logic is in the method NotificationSettingsRepository::findNotificationSubscribersByTarget which replaced some logic in the notification managers.

The logic works in tiers:

  1. user settings
  2. entry/post settings
  3. magazine settings
  4. default settings

So if you muted the magazine and thread, but the author is set to "Loud" then you will still get notifications for threads created by them.
If you muted the user, but set the magazine to "Loud" then you will get notifications for threads that are not created by that muted user.

UI
Magazine Box Magazine Box
Thread Info Thread Info
Entry Entry
Post Post
User Popover User Popover

@BentiGorlich BentiGorlich added enhancement New feature or request frontend Visual issues, improvements, bugs or other aspects relating mostly to the front end backend Backend related issues and pull requests labels Dec 8, 2024
@BentiGorlich BentiGorlich added this to the v1.8.0 milestone Dec 8, 2024
@BentiGorlich BentiGorlich self-assigned this Dec 8, 2024
@BentiGorlich BentiGorlich linked an issue Dec 8, 2024 that may be closed by this pull request
Notification Settings per User/Magazine/Thread/Post #1063
Open
@BentiGorlich BentiGorlich force-pushed the new/custom-notification-settings branch from 074b8c7 to a2e5b9b Compare December 8, 2024 18:14
@BentiGorlich BentiGorlich force-pushed the new/custom-notification-settings branch from a2e5b9b to 65f9bbe Compare December 8, 2024 23:33
@BentiGorlich BentiGorlich force-pushed the new/custom-notification-settings branch from 65f9bbe to 07863d2 Compare December 10, 2024 11:06
@BentiGorlich BentiGorlich force-pushed the new/custom-notification-settings branch from 77ba641 to e88753b Compare January 7, 2025 12:02
@BentiGorlich BentiGorlich force-pushed the new/custom-notification-settings branch from e88753b to 1d6efed Compare January 7, 2025 12:49
@BentiGorlich BentiGorlich force-pushed the new/custom-notification-settings branch from 1d6efed to fecc14a Compare January 7, 2025 13:05
@BentiGorlich BentiGorlich force-pushed the new/custom-notification-settings branch from fecc14a to 905071b Compare January 7, 2025 14:32
@BentiGorlich BentiGorlich requested a review from melroy89 January 13, 2025 17:22
@BentiGorlich BentiGorlich force-pushed the new/custom-notification-settings branch from 1debff7 to 8e10dec Compare January 13, 2025 20:06
@melroy89
Copy link
Member

Depending on the order of merging. Do you want the branch to be merged to main. Or in the dev branch. I leave the merge order up to you.

melroy89
melroy89 previously approved these changes Jan 14, 2025
View reviewed changes
@BentiGorlich
Implement custom notifications
6d1b2ff 
You can now set custom notification settings for users, magazine, entries and posts: `Loud`, `Default` or `Muted`.

Move the logic of "fetch an endpoint and get html. Replace html node with selector x with the result of the fetch" to another controller, so it can be used outside of subjects (`Entry`, `EntryComment`, `Post` and `PostComment`)
@BentiGorlich
Fix not getting notified on replies
8071409 
- if the magazine was not on default (e.g.: on loud) and you had "notify on replies" enabled it didn't work -> fix that
- rename some variables for readability
- add log statement with all the variables
@BentiGorlich
Fix the tests and also a bug
b388f9e 
By fixing the tests I discovered that nested comments don't trigger a notification for the author of the entry anymore. Fix that bug and the tests
@BentiGorlich
Fix bookmark list callback not working anymore
00fc43e 
- the bookmark_list component was forgotten when moving the linkCallback code into the new html-refresh controller
@BentiGorlich BentiGorlich force-pushed the new/custom-notification-settings branch from 6a1fe25 to 00fc43e Compare January 14, 2025 15:29
@BentiGorlich BentiGorlich changed the base branch from dev/new_features to main January 14, 2025 15:31
@BentiGorlich BentiGorlich dismissed melroy89’s stale review January 14, 2025 15:31

The base branch was changed.

@BentiGorlich BentiGorlich requested a review from melroy89 January 15, 2025 10:17
melroy89
melroy89 reviewed Jan 15, 2025
View reviewed changes
src/Repository/NotificationSettingsRepository.php
$dontNeedSubscriptionString = $dontNeedSubscription ? 'true' : 'false';
$dontNeedToBeAuthorString = $dontNeedToBeAuthor ? 'true' : 'false';

$sql = "SELECT u.id FROM \"user\" u
Copy link
Member

Choose a reason for hiding this comment

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

This query looks very heavy. Did you perform benchmarks on this new query? We already have too many long duration queries, so hopefully this query is not taking long to execute?

And I'm talking about a server with many users and magazines of course (a real world scenario).

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope I didn't test or benchmark this query, but imo it only looks heavy, but is actually not for a database server. It has just a big where statement which means it will not go through that many table rows.
I would love to benchmark it, but I have no idea how I would do that

Copy link
Member

Choose a reason for hiding this comment

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

I would love to have some performance tracing lines in Mbin, but I digress..

Regarding PostgreSQL queries, try to get real life performance duration by setting log_min_duration_statement in postgresql. And check the postgresql logs. Like in this issue: #1230 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I activated it with a 1 second log_min_duration_statement. But I doubt that my server is a good benchmark

Copy link
Member

Choose a reason for hiding this comment

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

maybe set it to 500ms instead ;)? Which is also very long (too long) for a query.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melroy89 melroy89 melroy89 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@BentiGorlich BentiGorlich

Labels
backend Backend related issues and pull requests enhancement New feature or request frontend Visual issues, improvements, bugs or other aspects relating mostly to the front end
Projects
None yet
Milestone
v1.8.0
Development

Successfully merging this pull request may close these issues.

Notification Settings per User/Magazine/Thread/Post
2 participants
@BentiGorlich @melroy89