Skip to content

Fix notification card links to use redirect URL#7665

Open
jacksbox-cassandra wants to merge 3 commits intonextcloud:mainfrom
jacksbox-cassandra:fix-notification-card-links
Open

Fix notification card links to use redirect URL#7665
jacksbox-cassandra wants to merge 3 commits intonextcloud:mainfrom
jacksbox-cassandra:fix-notification-card-links

Conversation

@jacksbox-cassandra
Copy link

Summary

This PR fixes issue #2469 where notifications for cards (due dates, assignments, comments) do not properly open the card when clicked.

Problem

Currently, card notifications use direct board/card URLs, which may not properly open the card modal if the user doesn't have the board loaded in their session or if there are routing issues.

Solution

Changed card notifications to use the redirect URL instead, which:

  • Uses the existing redirectToCard route that handles edge cases
  • Properly redirects to the correct board and opens the card modal
  • Is more resilient to different user states and routing scenarios
  • Follows the same pattern used elsewhere in the codebase

Changes

  • Added getCardRedirectUrl() method to Notifier class
  • Updated all card notification types to use redirect URLs:
    • card-assigned
    • card-overdue
    • card-comment-mentioned
  • Enhanced test coverage to verify redirect URL functionality

Testing

  • Added unit tests to verify notifications generate correct redirect URLs
  • Tested notification link generation for all card notification types
  • Verified backward compatibility with existing notification system

Fixes #2469

$this->assertEquals($notification, $actualNotification);
}

public function testPrepareCardAssigned() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is duplicated.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed! Renamed to testPrepareCardAssignedUsesRedirectUrl in commit b56448b. Thanks for the review!

@jacksbox-cassandra
Copy link
Author

Thanks for catching that @luka-nextcloud! 🙏

I've renamed the duplicate test method to testPrepareCardAssignedUsesRedirectUrl to clarify its purpose (verifying that card-assigned notifications use the redirect URL pattern) and avoid conflicting with the existing data-provider test.

Commit: b56448b

@jacksbox-cassandra jacksbox-cassandra force-pushed the fix-notification-card-links branch from b56448b to 0ad1255 Compare February 25, 2026 21:49
Copy link
Author

@jacksbox-cassandra jacksbox-cassandra left a comment

Choose a reason for hiding this comment

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

Hi @luka-nextcloud, thank you for the review! I've fixed the duplicate function issue. The second 'testPrepareCardAssigned' function has been renamed to 'testPrepareCardAssignedUsesRedirectUrl' to clarify its purpose (it specifically tests that card assignment notifications use the redirect URL pattern). The duplicate function names that were causing the issue have been resolved.

- Change card notifications to use redirectToCard route instead of direct board/card URL
- This ensures notifications properly open the card modal regardless of current state
- Add getCardRedirectUrl method to Notifier class
- Update tests to verify redirect URL functionality
- Fixes issue nextcloud#2469: Notifications should open card

Signed-off-by: jacksbox_cassandra <jacksbot2026@gmail.com>
…ectUrl

Addresses review feedback: the added test method had the same name
as an existing method with a data provider, causing a duplicate
function declaration.

Renamed to testPrepareCardAssignedUsesRedirectUrl to clarify its
purpose (verifying that card-assigned notifications use the
redirect URL pattern).

Signed-off-by: jacksbox_cassandra <jacksbot2026@gmail.com>
@jacksbox-cassandra jacksbox-cassandra force-pushed the fix-notification-card-links branch from 0ad1255 to b62e6b3 Compare February 26, 2026 01:12
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.

Notifications should open card

2 participants