Skip to content

fix(notification): missing notification - #2135

Closed
luka-nextcloud wants to merge 1 commit into
masterfrom
fix-missing-push-notifications
Closed

fix(notification): missing notification#2135
luka-nextcloud wants to merge 1 commit into
masterfrom
fix-missing-push-notifications

Conversation

@luka-nextcloud

Copy link
Copy Markdown
Contributor

Signed-off-by: Luka Trovic <luka@nextcloud.com>
@github-project-automation github-project-automation Bot moved this to 🧭 Planning evaluation (don't pick) in 📝 Productivity team Sep 18, 2025
@luka-nextcloud luka-nextcloud moved this from 🧭 Planning evaluation (don't pick) to 👀 In review in 📝 Productivity team Sep 18, 2025
@cypress

cypress Bot commented Sep 18, 2025

Copy link
Copy Markdown

Activity    Run #2853

Run Properties:  status check failed Failed #2853  •  git commit 55e39447b7: fix(notification): missing notification
Project Activity
Branch Review fix-missing-push-notifications
Run status status check failed Failed #2853
Run duration 02m 13s
Commit git commit 55e39447b7: fix(notification): missing notification
Committer Luka Trovic
View all properties for this run ↗︎

Test results
Tests that failed  Failures 1
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 9
View all changes introduced in this branch ↗︎

Tests for review

Failed  cypress/e2e/sidebar.cy.ts • 1 failed test • Run E2E

View Output

Test Artifacts
Check activity listing in the sidebar > Has share activity Test Replay Screenshots

@nickvergessen

Copy link
Copy Markdown
Member

This feels wrong/unexpected to change like that after 5 years of working like that?
The activity app takes care of parsing those notifications. If you don't want that, disable the option to receive activities as notifications for your type and handle the notifications in your app directly?

For now a similar result should be achievable by changing:
https://github.com/nextcloud/deck/blob/main/lib/Notification/Notifier.php#L80

if ($notification->getApp() !== 'deck' || $notification->getObjectType() === 'activity_notification') {

@luka-nextcloud

luka-nextcloud commented Sep 19, 2025

Copy link
Copy Markdown
Contributor Author

@nickvergessen I do want the Activity app to handle parsing those notifications. However, for now, it won’t parse notifications where $notification->getApp() returns deck (different from activity)

@nickvergessen

Copy link
Copy Markdown
Member

The activity app already handles them, it's not caring about the app id in the notification, only about the object type: https://github.com/nextcloud/activity/blob/master/lib/NotificationGenerator.php#L107
It helps a bit to be able to see which app's activity is causing notifications 🙈

@luka-nextcloud

Copy link
Copy Markdown
Contributor Author

Now I understand what you mean, thank you.

@github-project-automation github-project-automation Bot moved this from 👀 In review to ☑️ Done in 📝 Productivity team Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

No push notifications for Deck

2 participants