FEATURE: per-topic unsubscribe option in emails#2
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a per-topic unsubscribe feature that allows users to unsubscribe from notifications for specific topics directly from email notifications, rather than needing to visit their general preferences page.
Key Changes:
- Added topic-specific unsubscribe URLs to email notifications
- Created new controller action and routes for topic unsubscribe functionality
- Added frontend interface for managing topic notification preferences
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
lib/email/message_builder.rb |
Adds support for topic-specific unsubscribe URLs in email templates |
app/mailers/user_notifications.rb |
Passes topic unsubscribe URL to email builder |
config/routes.rb |
Adds new routes for topic unsubscribe functionality |
app/controllers/topics_controller.rb |
Implements unsubscribe action to modify notification levels |
app/models/topic.rb |
Adds method to generate topic unsubscribe URLs |
config/locales/server.en.yml |
Updates email unsubscribe text to include topic-specific option |
config/locales/client.en.yml |
Adds localization strings for unsubscribe interface |
| Frontend files | Implements JavaScript routes, controllers, and templates for unsubscribe UI |
| Test files | Updates tests to include new unsubscribe URL parameter |
| end | ||
|
|
||
| tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id]) | ||
|
|
There was a problem hiding this comment.
The code doesn't handle the case where TopicUser record doesn't exist. If find_by returns nil, the subsequent lines will raise a NoMethodError when trying to access tu.notification_level.
| raise Discourse::NotFound unless tu |
| return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") }); | ||
| }.property("model.fancyTitle"), | ||
|
|
||
| }) |
There was a problem hiding this comment.
There's a typo in the function name stopNotificiationsText - it should be stopNotificationsText (missing 'a' in 'notifications').
| @@ -0,0 +1,8 @@ | |||
| <div class="container"> | |||
| <p> | |||
| {{{stopNotificiationsText}}} | |||
There was a problem hiding this comment.
There's a typo in the property name stopNotificiationsText - it should be stopNotificationsText (missing 'a' in 'notifications'). This should match the corrected property name in the controller.
| {{{stopNotificiationsText}}} | |
| {{{stopNotificationsText}}} |
| end | ||
|
|
||
| def unsubscribe | ||
| @topic_view = TopicView.new(params[:topic_id], current_user) |
There was a problem hiding this comment.
The unsubscribe action doesn't authenticate the user. Since it's accessing current_user, there should be authentication requirements to ensure only logged-in users can access this functionality.
|
This pull request has been automatically marked as stale because it has been open for 60 days with no activity. To keep it open, remove the stale tag, push code, or add a comment. Otherwise, it will be closed in 14 days. |
Test 2