FEATURE: per-topic unsubscribe option in emails#10
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a per-topic unsubscribe feature to email notifications. Users can now unsubscribe from individual topics directly from notification emails, in addition to the existing global email preference settings.
Key Changes:
- Added
/t/:topic_id/unsubscribeand/t/:slug/:topic_id/unsubscriberoutes for topic-specific unsubscribe functionality - Updated email templates to include a topic-specific unsubscribe link alongside the general preferences link
- Created new frontend routes, controllers, views, and templates to handle the unsubscribe UI
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| config/routes.rb | Added new unsubscribe routes for topics |
| config/locales/server.en.yml | Updated unsubscribe text to include topic-specific option |
| config/locales/client.en.yml | Added client-side translations for unsubscribe UI |
| app/models/topic.rb | Added unsubscribe_url method to Topic model |
| app/mailers/user_notifications.rb | Passed unsubscribe_url to email builder |
| lib/email/message_builder.rb | Formatting improvements for template args |
| app/controllers/topics_controller.rb | Added unsubscribe action to handle notification level changes |
| app/views/email/notification.html.erb | Formatting improvements to email template |
| app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6 | New route for unsubscribe page |
| app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6 | New controller for unsubscribe functionality |
| app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs | New template for unsubscribe UI |
| app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6 | New view for unsubscribe page |
| app/assets/javascripts/discourse/routes/app-route-map.js.es6 | Registered new unsubscribe route |
| app/assets/javascripts/discourse/components/dropdown-button.js.es6 | Added null check for title property |
| app/assets/stylesheets/common/base/topic.scss | Added styles for unsubscribe page |
| app/models/topic_user.rb | Code style improvements |
| app/assets/javascripts/discourse/routes/topic-from-params.js.es6 | Refactored to use ES6 syntax |
| spec/components/email/message_builder_spec.rb | Updated test to include unsubscribe_url parameter |
| @@ -0,0 +1,8 @@ | |||
| <div class="container"> | |||
| <p> | |||
| {{{stopNotificiationsText}}} | |||
There was a problem hiding this comment.
Corrected spelling of 'stopNotificiations' to 'stopNotifications'.
| {{{stopNotificiationsText}}} | |
| {{{stopNotificationsText}}} |
|
|
||
| export default ObjectController.extend({ | ||
|
|
||
| stopNotificiationsText: function() { |
There was a problem hiding this comment.
Corrected spelling of 'stopNotificiations' to 'stopNotifications'.
| stopNotificiationsText: function() { | |
| stopNotificationsText: function() { |
| if tu.notification_level > TopicUser.notification_levels[:regular] | ||
| tu.notification_level = TopicUser.notification_levels[:regular] | ||
| else | ||
| tu.notification_level = TopicUser.notification_levels[:muted] |
There was a problem hiding this comment.
Potential nil dereference if TopicUser.find_by returns nil. The code should handle the case where no TopicUser record exists before accessing tu.notification_level.
| if tu.notification_level > TopicUser.notification_levels[:regular] | |
| tu.notification_level = TopicUser.notification_levels[:regular] | |
| else | |
| tu.notification_level = TopicUser.notification_levels[:muted] | |
| if tu.nil? | |
| # If no TopicUser exists, create one with the correct notification_level | |
| notification_level = TopicUser.notification_levels[:muted] | |
| tu = TopicUser.new(user_id: current_user.id, topic_id: params[:topic_id], notification_level: notification_level) | |
| else | |
| if tu.notification_level > TopicUser.notification_levels[:regular] | |
| tu.notification_level = TopicUser.notification_levels[:regular] | |
| else | |
| tu.notification_level = TopicUser.notification_levels[:muted] | |
| end |
| get "t/:slug/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/, post_number: /\d+/} | ||
| get "t/:topic_id/summary" => "topics#show", constraints: {topic_id: /\d+/, post_number: /\d+/} | ||
| get "t/:slug/:topic_id/moderator-liked" => "topics#moderator_liked", constraints: {topic_id: /\d+/} | ||
| get "t/:slug/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/} |
There was a problem hiding this comment.
The constraint post_number: /\d+/ was removed from this route but it was present in the original. This could potentially allow invalid requests that were previously filtered out.
|
|
||
| stopNotificiationsText: function() { | ||
| return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") }); | ||
| }.property("model.fancyTitle"), |
There was a problem hiding this comment.
[nitpick] Trailing comma should be removed after the last property in the object.
| }.property("model.fancyTitle"), | |
| }.property("model.fancyTitle") |
Test 2
Replicated from ai-code-review-evaluation/discourse-copilot#2