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

fix(subscriptions): reply notifications not working #3445

Merged
merged 3 commits into from
Jun 3, 2022

Conversation

SychO9
Copy link
Member

@SychO9 SychO9 commented Jun 1, 2022

Fixes #3443

Changes proposed in this pull request:
As mentioned in the issue the reply notification job now receives an updated $lastPostNumber and because new post numbers are now guaranteed to be the max+1 (last post number before it + 1) then we can assume the job can use $lastPostNumber - 1.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

@davwheat
Copy link
Member

davwheat commented Jun 1, 2022

Could we run into issues if two people post simultaneously and the queue processes the jobs late? I'd assume that the last post number would then refer to a later post than what we want, wouldn't it?

@SychO9
Copy link
Member Author

SychO9 commented Jun 2, 2022

If I understand you correctly, the post number would still point to the correct value since it is provided to the job object at posting time. So no matter when the job processes it'll have the correct number because it isn't looking it up but using what it was given:

public function __construct(Post $post, $lastPostNumber)
{
$this->post = $post;
$this->lastPostNumber = $lastPostNumber;
}

@davwheat
Copy link
Member

davwheat commented Jun 2, 2022

Oh yes, sorry, I was being a total idiot.

@SychO9
Copy link
Member Author

SychO9 commented Jun 2, 2022

Never!

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

Successfully merging this pull request may close these issues.

Reply notifications not being sent out to users
3 participants