FEATURE: per-topic unsubscribe option in emails#9
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a per-topic unsubscribe feature that allows users to manage notification preferences for individual topics directly from email notifications.
Changes:
- Added new unsubscribe routes and controller action for topics
- Updated email templates to include topic-specific unsubscribe links
- Created JavaScript views, controllers, and routes for the unsubscribe UI
- Enhanced message builder to support custom unsubscribe URLs
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| config/routes.rb | Added topic unsubscribe routes with and without slug |
| app/controllers/topics_controller.rb | Implemented unsubscribe action to change notification levels |
| app/models/topic.rb | Added unsubscribe_url method |
| app/mailers/user_notifications.rb | Passed topic unsubscribe URL to email builder |
| lib/email/message_builder.rb | Formatting improvements for template args |
| config/locales/server.en.yml | Updated unsubscribe link text with topic-specific option |
| config/locales/client.en.yml | Added unsubscribe UI text |
| app/views/email/notification.html.erb | Code formatting improvements |
| app/assets/javascripts/discourse/routes/app-route-map.js.es6 | Registered topicUnsubscribe route |
| app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6 | Created route for unsubscribe page |
| app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6 | Created controller for unsubscribe page |
| app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6 | Created view for unsubscribe page |
| app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs | Created template for unsubscribe page |
| app/assets/javascripts/discourse/components/dropdown-button.js.es6 | Added title safety check |
| app/assets/stylesheets/common/base/topic.scss | Added styles for unsubscribe page |
| app/models/topic_user.rb | Code formatting improvements |
| app/assets/javascripts/discourse/routes/topic-from-params.js.es6 | Modernized ES6 syntax |
| spec/components/email/message_builder_spec.rb | Updated test with unsubscribe_url parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,8 @@ | |||
| <div class="container"> | |||
| <p> | |||
| {{{stopNotificiationsText}}} | |||
There was a problem hiding this comment.
Corrected spelling of 'stopNotificiations' to 'stopNotifications'.
| {{{stopNotificiationsText}}} | |
| {{{stopNotificationsText}}} |
| tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id]) | ||
|
|
||
| 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 record doesn't exist. The code accesses tu.notification_level without checking if tu is nil first. Add a nil check or handle the case where the user hasn't interacted with the topic yet.
| tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id]) | |
| if tu.notification_level > TopicUser.notification_levels[:regular] | |
| tu.notification_level = TopicUser.notification_levels[:regular] | |
| else | |
| tu.notification_level = TopicUser.notification_levels[:muted] | |
| tu = TopicUser.find_or_initialize_by(user_id: current_user.id, topic_id: params[:topic_id]) | |
| if tu.new_record? | |
| # User hasn't interacted with this topic yet; unsubscribe by muting it. | |
| tu.notification_level = TopicUser.notification_levels[:muted] | |
| 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 |
| return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") }); | ||
| }.property("model.fancyTitle"), | ||
|
|
||
| }) |
There was a problem hiding this comment.
Trailing comma missing after the last property definition. This is inconsistent with modern JavaScript best practices and can cause issues with certain tooling.
Test 2
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#2