Skip to content

Always register the Notifier - #14744

Merged
ChristophWurst merged 1 commit into
masterfrom
bugfix/noid/fix-push-notifications-for-updatenotifications
Mar 19, 2019
Merged

Always register the Notifier#14744
ChristophWurst merged 1 commit into
masterfrom
bugfix/noid/fix-push-notifications-for-updatenotifications

Conversation

@nickvergessen

Copy link
Copy Markdown
Member

... so background jobs without a user can prepare push notifications

... so background jobs without a user can prepare push notifications
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen

Copy link
Copy Markdown
MemberAuthor

/backport to stable15

@nickvergessen

Copy link
Copy Markdown
MemberAuthor

/backport to stable14

@rullzerrullzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Whoops...

// Since notifications also work for non-admins we don't check this here.
$this->registerNotifier();
} else if ($server->getGroupManager()->isAdmin($user->getUID())) {
if (!$server->getAppManager()->isEnabledForUser('notifications') &&

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is updateChecker only checked if the notifications is not enabled for Users?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Because if you receive a notification, we do not show the JS popup. It is basically the fallback for when you dont have notifications enabled.

@tacructacruc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can't find any problems

@ChristophWurst
ChristophWurst merged commit af03d3a into masterMar 19, 2019
@ChristophWurst
ChristophWurst deleted the bugfix/noid/fix-push-notifications-for-updatenotifications branch March 19, 2019 15:42
@backportbot-nextcloud

Copy link
Copy Markdown

backport to stable15 in #14749

@MorrisJobkeMorrisJobke mentioned this pull request Mar 20, 2019
9 tasks
@tobiasKaminsky

Copy link
Copy Markdown
Member

I applied this manually on NC15 and got yesterday evening a push notification with an info about update of collabora 🎉

Thank you for finding the problem @nickvergessen

@nickvergessen

Copy link
Copy Markdown
MemberAuthor

/backport to stable14

@backportbot-nextcloud

Copy link
Copy Markdown

backport to stable14 in #14783

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@nickvergessen@tobiasKaminsky@rullzer@tacruc@ChristophWurst