fix: Unsubscribe link for collection subscriptions #8734
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the unsubscribe link for collection subscriptions by adding the missing subscriptionId functionality to notifications and related endpoints. Key changes include:
- Adding a migration to add the subscriptionId column to notifications.
- Updating the notification recipient helper functions and tests to handle a Recipient type that includes an optional subscription.
- Propagating subscriptionId to the unsubscribe URL, email templates, and various notification tasks.
Reviewed Changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/migrations/20250321021710-add-subscription-to-notifications.js | Migration to add subscriptionId column |
| server/models/helpers/NotificationHelper.ts | Updated recipient types and mapping logic for notifications |
| server/routes/api/subscriptions/subscriptions.ts | Adjusted unsubscribe handling to include subscriptionId |
| server/models/Notification.ts | Added subscription association and foreign key |
| server/models/Subscription.ts | Added isCollectionSubscription getter |
| server/routes/api/subscriptions/schema.ts | Added optional subscriptionId to schema |
| app/hooks/useQueryNotices.ts | Added handling for the UnsubscribeCollection notice |
| server/emails/templates/DocumentPublishedOrUpdatedEmail.tsx | Passed subscriptionId to the unsubscribe URL |
| shared/types.ts | Added UnsubscribeCollection enum |
| server/queues/processors/EmailsProcessor.ts | Included subscriptionId in notification payload |
| server/models/helpers/SubscriptionHelper.ts | Updated unsubscribeUrl and unsubscribeToken to include subscriptionId |
| server/models/helpers/NotificationHelper.test.ts | Updated tests to check for recipient.user.id and subscription linkage |
| server/queues/tasks/* | Updated notification tasks to propagate subscriptionId to notifications |
Files not reviewed (1)
- shared/i18n/locales/en_US/translation.json: Language not supported
|
oof terrible miss! thanks for fixing it 🙏 will test now. |
|
Passing Should be fine right? 🤔 |
|
I believe the intent is OK, should be very rare. Interesting though |
|
Alternatively I could grab the |
I think you meant using the latest
Same, this edge-case must be veryyy rare - think we can leave it as-is? Just wanted to bring it up to cover the bases. |
well to fix the issue you raised it would need to be passed into the email -> unsubscribe url -> deletion handler yep |
8ae178f to
d85f445
Compare
Hopefully all the existing tests make this easier to review – TLDR: We include a link to unsubscribe in document notification emails, when #8392 was added this wasn't updated to account for collection subscriptions.