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

Reply notifications not being sent out to users #3443

Closed
SychO9 opened this issue May 31, 2022 · 4 comments · Fixed by #3445
Closed

Reply notifications not being sent out to users #3443

SychO9 opened this issue May 31, 2022 · 4 comments · Fixed by #3445
Labels

Comments

@SychO9
Copy link
Member

SychO9 commented May 31, 2022

Bug Report

Current Behavior
When someone replies to a discussion you follow, you do not receive a notification.

Steps to Reproduce

  1. Follow a discussion you have read.
  2. Reply with a different user
  3. No notification is sent out.

Expected Behavior
A notification should be sent out.

Environment

  • Flarum version: Introduced in 1.3.0

Possible Solution
Context: If the actor has read the post number 6 which at the time was the last, and someone replies, the subscriptions extension sends out notifications to those following who have last read post number 6. It doesn't send notifications to all those who have last read less than post number 7 (newest reply) to avoid repetitive notifications.

Additional Context
It does this by listening to the Posted event and using the $event->post->discussion->last_post_number (should be 6), it expects to run before DiscussionMetadataUpdater subscriber (updates last_post_number to 7), however in 1.3 this PR (#3373) changes the behavior and leads to extension listeners to run after all core listeners. So what's happening now is that the last_post_number used in the subscriptions listener has already been updated to 7 in this example.

v1.2 v1.3
Screenshot from 2022-05-31 14-23-56 Screenshot from 2022-05-31 14-22-04

Potential Solutions

  • We could introduce a listener priority system to make sure the listener runs before the discussion metadata updater listener.
  • We could go for a last_post_number - 1 change in the subscriptions listener. This wouldn't be a solution if we were still using post_number_index, but since we now count new post numbers based on previous existing post number, it might be fine to do.
@luceos
Copy link
Member

luceos commented May 31, 2022

As the post has already been saved when the listener is dispatched, why don't we use $post->number -1 instead? Relying on a relationship inside that listener seems unnecessary and the number should already have been updated.

@SychO9
Copy link
Member Author

SychO9 commented May 31, 2022

Yes I believe we could just do that, I would just like to make sure that is the case first, I'll investigate further sometime later today.

@matteocontrini
Copy link
Contributor

Is there a chance for this fix to be released as a patch? I would personally skip 1.3.0 altogether otherwise...

@luceos
Copy link
Member

luceos commented Jun 4, 2022

@matteocontrini yes we'll do a patch release for this soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants