FEATURE: per-topic unsubscribe option in emails#2
Conversation
WalkthroughThis change set introduces topic-level unsubscribe functionality in both the backend and frontend. It adds new routes, controllers, views, templates, and localization strings to support unsubscribing from topic notifications. Email notifications are updated to include a direct unsubscribe link, and related styles and tests are adjusted to accommodate these new features. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend (JS)
participant Backend (Rails)
participant Email
User->>Frontend (JS): Navigates to /t/:slug/:id/unsubscribe
Frontend (JS)->>Backend (Rails): GET /t/:slug/:id/unsubscribe
Backend (Rails)->>Backend (Rails): topics#unsubscribe action
Backend (Rails)->>Backend (Rails): Adjusts TopicUser notification level
Backend (Rails)-->>Frontend (JS): Renders unsubscribe page
Frontend (JS)-->>User: Shows unsubscribe confirmation and options
Backend (Rails)->>Email: Sends notification with unsubscribe_url
Email-->>User: Receives email with topic unsubscribe link
User->>Frontend (JS): Clicks unsubscribe link in email
Frontend (JS)->>Backend (Rails): GET /t/:slug/:id/unsubscribe
Backend (Rails)-->>Frontend (JS): Processes as above
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
app/models/topic_user.rb (1)
120-123:track_visit!assignsuser_idfromtopic– critical bug
Line 122 usestopicinstead ofuserfor the fallback branch, causinguser_idto be set to the topic id whenever a raw id is passed. This silently corrupts data intopic_usersand all downstream statistics.- user_id = user.is_a?(User) ? user.id : topic + user_id = user.is_a?(User) ? user.id : user
🧹 Nitpick comments (3)
app/models/topic_user.rb (1)
60-66: One-liner block is acceptable, but watch for future scalability
Using a single-line{ … }block fortopic_users.eachkeeps the file concise; just ensure it doesn’t grow into multi-statement logic later, as that would warrant the multi-linedo…endform.app/views/email/notification.html.erb (1)
3-3: Preferrender ... post: pcompact syntax for consistencyLines 3 and 13 mix the 1.9 hash style with positional arguments. Minor, but staying consistent aids readability.
<%= render partial: 'email/post', locals: { post: post } %> ... <%= render partial: 'email/post', locals: { post: p } %>Also applies to: 12-14
app/controllers/topics_controller.rb (1)
28-28: Add trailing comma for consistency.Consider adding a trailing comma after
unsubscribeto follow Ruby style conventions.- :unsubscribe] + :unsubscribe,]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/assets/javascripts/discourse/components/dropdown-button.js.es6(1 hunks)app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6(1 hunks)app/assets/javascripts/discourse/routes/app-route-map.js.es6(1 hunks)app/assets/javascripts/discourse/routes/topic-from-params.js.es6(2 hunks)app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6(1 hunks)app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs(1 hunks)app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6(1 hunks)app/assets/stylesheets/common/base/topic.scss(1 hunks)app/controllers/topics_controller.rb(3 hunks)app/mailers/user_notifications.rb(2 hunks)app/models/topic.rb(1 hunks)app/models/topic_user.rb(6 hunks)app/views/email/notification.html.erb(1 hunks)config/locales/client.en.yml(1 hunks)config/locales/server.en.yml(1 hunks)config/routes.rb(1 hunks)lib/email/message_builder.rb(2 hunks)spec/components/email/message_builder_spec.rb(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
app/models/topic.rb (4)
app/models/post.rb (2)
url(342-348)url(350-352)app/models/category.rb (1)
url(392-404)app/serializers/similar_topic_serializer.rb (1)
url(14-16)app/models/notification.rb (1)
url(90-94)
app/mailers/user_notifications.rb (2)
app/models/notification.rb (1)
post(96-100)app/models/topic.rb (1)
unsubscribe_url(719-721)
app/controllers/topics_controller.rb (2)
app/models/topic.rb (1)
unsubscribe_url(719-721)app/models/topic_user.rb (1)
notification_levels(19-21)
🪛 RuboCop (1.76.1)
lib/email/message_builder.rb
[warning] 62-62: Use == if you meant to do a comparison or wrap the expression in parentheses to indicate you meant to assign in a condition.
(Lint/AssignmentInCondition)
app/controllers/topics_controller.rb
[convention] 28-28: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
[convention] 98-116: Assignment Branch Condition size for unsubscribe is too high. [<4, 31, 6> 31.83/23]
(Metrics/AbcSize)
🔇 Additional comments (27)
app/assets/javascripts/discourse/routes/topic-from-params.js.es6 (2)
5-5: LGTM! Good ES6 modernization.The conversion to method shorthand syntax is correct and improves code readability.
7-46: Excellent ES6 modernization!The changes effectively modernize the code:
- Method shorthand syntax is cleaner and more readable
- Using
constinstead ofvarprovides better scoping- Consolidating variable declarations improves readability
- All functionality remains intact
app/models/topic.rb (1)
719-721: LGTM! Clean implementation following established patterns.The
unsubscribe_urlmethod follows the same pattern as other URL methods in the codebase (likeurl,relative_url,last_post_url) and provides a clear, consistent way to generate unsubscribe URLs for topics.app/assets/javascripts/discourse/routes/app-route-map.js.es6 (1)
13-13: LGTM! Well-placed route addition.The
topicUnsubscriberoute follows proper naming conventions and uses the correct path pattern that aligns with theunsubscribe_urlmethod added to the Topic model. The placement near other topic routes is logical and maintains good code organization.app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6 (1)
1-3: LGTM! Standard Discourse view implementation.The view component follows Discourse conventions by extending
Discourse.Viewand providing a semantic CSS class name for styling. The minimal implementation is appropriate for this use case.spec/components/email/message_builder_spec.rb (1)
171-172: LGTM! Proper test coverage for unsubscribe URL.The addition of the
unsubscribe_urlparameter ensures that the email message builder tests cover the new topic-level unsubscribe functionality. The URL format matches the pattern generated by the Topic model'sunsubscribe_urlmethod.app/assets/stylesheets/common/base/topic.scss (1)
67-77: LGTM! Clean CSS styling for the unsubscribe feature.The CSS rules properly override default dropdown behaviors to ensure the unsubscribe UI components are correctly positioned and aligned. The
inline-blockdisplay, float reset, and bottom position reset work together to create a clean layout for the topic unsubscribe interface.app/assets/javascripts/discourse/components/dropdown-button.js.es6 (1)
27-30: LGTM! Good defensive programming practice.The conditional rendering prevents empty
<h4 class='title'>elements from being created when no title is provided, resulting in cleaner HTML output while maintaining backward compatibility.app/mailers/user_notifications.rb (2)
295-295: LGTM! Proper integration of unsubscribe URL.The addition of
unsubscribe_urlto the email options correctly integrates with the new topic unsubscribe feature. The URL frompost.topic.unsubscribe_urlwill be available in email templates to render unsubscribe links.
310-310: LGTM! Clean code organization.The positioning of the
from_aliasassignment maintains the same functionality while improving code organization within the email options setup.app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs (1)
6-6: LGTM! Proper component integration.The template correctly uses internationalization and properly binds the
topic-notifications-buttoncomponent to the model, providing a clean unsubscribe interface.app/models/topic_user.rb (5)
8-13: Chained-wherereformat looks good
The line-break and indentation of the chainedwherecall improve readability while preserving semantics.
112-114: InlineMessageBus.publishhash is fine
Collapsing the hash onto the same line is a harmless stylistic tweak that keeps the method compact.
124-126:update_allinline hash is fine
No functional change; the tightened formatting is clear and Ruby-style compliant.
190-196: Same-lineMessageBus.publishremains clear
The refactor to a single line doesn’t affect behaviour and keeps the callback concise.
216-219: Consistent formatting here as well
Matches the earlier style changes; no issues.config/locales/server.en.yml (1)
1852-1855: Likely misplaced/indented locale key – please double-check path & scope
unsubscribe_linkis indented as a child ofdownload_remote_images_disabled, so its lookup key will be
en.download_remote_images_disabled.unsubscribe_link.
Given that the text is a generic unsubscribe paragraph (not specific to “download_remote_images_disabled” events) it probably belongs under a more global namespace (e.g.user_notificationsor a new top-levelAction items:
- Confirm the lookup path used in
user_notifications.rb/ templates.- If the code expects a global key, move the block accordingly and keep indentation consistent.
- Run
rake i18n:js:export(or equivalent) to catch missing keys before deploy.lib/email/message_builder.rb (3)
24-28: LGTM: Improved readability.The multi-line hash initialization improves code readability while maintaining identical functionality.
34-39: LGTM: Cleaner conditional logic.The ternary operator formatting makes the conditional assignment more readable while preserving the original logic.
61-68: LGTM: Formatting improvements with expected static analysis false positive.The whitespace adjustments improve readability. The static analysis warning about assignment in condition on line 62 is a false positive - this is idiomatic Ruby where
response_instructions = @template_args[:respond_instructions]intentionally assigns and checks truthiness.config/routes.rb (1)
438-442: LGTM: Well-structured unsubscribe routes addition.The new unsubscribe routes are properly implemented with:
- Consistent topic_id constraints (
/\d+/)- Both slug and non-slug variants for flexibility
- Proper mapping to
topics#unsubscribeaction- Logical route ordering and simplified summary route constraints
These routes effectively support the per-topic unsubscribe feature.
config/locales/client.en.yml (1)
984-986: Confirm placeholder spec & provide full guidance text
stop_notificationscorrectly interpolates{{title}}, butchange_notification_statelooks incomplete (“You can change your notification state”).
Consider adding an explicit call-to-action (e.g. “here” with a link placeholder) so translators know where the link will be injected and users get a clear affordance.Example:
change_notification_state: "You can change your notification state <a href='{{link}}'>here</a>."At minimum, double-check downstream templates expect a link placeholder.
app/controllers/topics_controller.rb (3)
27-28: LGTM: Security configuration is correct.Adding
unsubscribeto the login-required actions is appropriate since users need to be authenticated to manage their notification preferences.
32-32: LGTM: XHR exemption is necessary for email links.Exempting
unsubscribefrom the XHR check is correct since unsubscribe links will be accessed directly from emails, not through AJAX requests.
500-500: LGTM: Explicit template rendering improves clarity.Making the template rendering explicit is a good practice, especially since both
showandunsubscribeactions share this response logic.app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6 (2)
12-15: LGTM: Clear intent and proper implementation.Clearing the notification reason text on the unsubscribe page makes good UX sense, and the implementation uses proper Ember patterns.
17-22: LGTM: Proper Ember action implementation.The
didTransitionaction correctly ensures the footer is shown and follows Ember conventions by returningtrueto allow action bubbling.
| stopNotificiationsText: function() { | ||
| return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") }); | ||
| }.property("model.fancyTitle"), |
There was a problem hiding this comment.
Fix typo in method name.
There's a typo in the computed property name: stopNotificiationsText should be stopNotificationsText (missing 'a' in "Notifications").
Apply this diff to fix the typo:
- stopNotificiationsText: function() {
+ stopNotificationsText: function() {Note: This change will also require updating the template file that references this property.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| stopNotificiationsText: function() { | |
| return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") }); | |
| }.property("model.fancyTitle"), | |
| stopNotificationsText: function() { | |
| return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") }); | |
| }.property("model.fancyTitle"), |
🤖 Prompt for AI Agents
In app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6 around
lines 5 to 7, the computed property name stopNotificiationsText contains a typo
and should be renamed to stopNotificationsText. Rename the method to fix the
spelling error and ensure that any template files or other code referencing this
property are updated accordingly to use the corrected name.
| model(params) { | ||
| const topic = this.store.createRecord("topic", { id: params.id }); | ||
| return PostStream.loadTopicView(params.id).then(json => { | ||
| topic.updateFromJson(json); | ||
| return topic; | ||
| }); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for the async operation.
The model loading pattern is appropriate, but there's no error handling if PostStream.loadTopicView fails.
Consider adding error handling:
model(params) {
const topic = this.store.createRecord("topic", { id: params.id });
- return PostStream.loadTopicView(params.id).then(json => {
+ return PostStream.loadTopicView(params.id).then(json => {
topic.updateFromJson(json);
return topic;
+ }).catch(error => {
+ // Handle the error appropriately - maybe redirect to topic or show error
+ console.error("Failed to load topic for unsubscribe:", error);
+ throw error;
});
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| model(params) { | |
| const topic = this.store.createRecord("topic", { id: params.id }); | |
| return PostStream.loadTopicView(params.id).then(json => { | |
| topic.updateFromJson(json); | |
| return topic; | |
| }); | |
| }, | |
| model(params) { | |
| const topic = this.store.createRecord("topic", { id: params.id }); | |
| return PostStream.loadTopicView(params.id).then(json => { | |
| topic.updateFromJson(json); | |
| return topic; | |
| }).catch(error => { | |
| // Handle the error appropriately - maybe redirect to topic or show error | |
| console.error("Failed to load topic for unsubscribe:", error); | |
| throw error; | |
| }); | |
| }, |
🤖 Prompt for AI Agents
In app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6 around lines
4 to 10, the model method calls the async function PostStream.loadTopicView
without error handling. To fix this, add a catch block to handle any errors from
the promise returned by loadTopicView. Inside the catch, log the error or handle
it appropriately to prevent unhandled promise rejections and improve robustness.
| @@ -0,0 +1,8 @@ | |||
| <div class="container"> | |||
| <p> | |||
| {{{stopNotificiationsText}}} | |||
There was a problem hiding this comment.
Fix property name to match controller correction.
The template references stopNotificiationsText which has a typo (should be stopNotificationsText). This needs to be corrected to match the fix in the controller.
Apply this diff to fix the typo:
- {{{stopNotificiationsText}}}
+ {{{stopNotificationsText}}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{{stopNotificiationsText}}} | |
| {{{stopNotificationsText}}} |
🤖 Prompt for AI Agents
In app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs at line 3,
the property name `stopNotificiationsText` contains a typo. Change it to
`stopNotificationsText` to match the corrected property name in the controller
and ensure proper data binding.
| def unsubscribe | ||
| @topic_view = TopicView.new(params[:topic_id], current_user) | ||
|
|
||
| if slugs_do_not_match || (!request.format.json? && params[:slug].blank?) | ||
| return redirect_to @topic_view.topic.unsubscribe_url, status: 301 | ||
| end | ||
|
|
||
| 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] | ||
| end | ||
|
|
||
| tu.save! | ||
|
|
||
| perform_show_response | ||
| end |
There was a problem hiding this comment.
Handle nil TopicUser record and consider reducing complexity.
The unsubscribe logic is sound, but there are two concerns:
- Critical: If no
TopicUserrecord exists,find_byreturnsniland calling methods on it will raise aNoMethodError. - Optional: The method has high ABC complexity (31.83/23) which could be reduced by extracting the slug validation logic.
Apply this diff to handle the nil case:
tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id])
+
+if tu.nil?
+ tu = TopicUser.create!(user_id: current_user.id, topic_id: params[:topic_id], 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
-
tu.save!
+endFor complexity reduction, consider extracting the slug validation into a private method similar to the show action's approach.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def unsubscribe | |
| @topic_view = TopicView.new(params[:topic_id], current_user) | |
| if slugs_do_not_match || (!request.format.json? && params[:slug].blank?) | |
| return redirect_to @topic_view.topic.unsubscribe_url, status: 301 | |
| end | |
| 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] | |
| end | |
| tu.save! | |
| perform_show_response | |
| end | |
| def unsubscribe | |
| @topic_view = TopicView.new(params[:topic_id], current_user) | |
| if slugs_do_not_match || (!request.format.json? && params[:slug].blank?) | |
| return redirect_to @topic_view.topic.unsubscribe_url, status: 301 | |
| end | |
| tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id]) | |
| if tu.nil? | |
| tu = TopicUser.create!( | |
| user_id: current_user.id, | |
| topic_id: params[:topic_id], | |
| 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 | |
| tu.save! | |
| end | |
| perform_show_response | |
| end |
🧰 Tools
🪛 RuboCop (1.76.1)
[convention] 98-116: Assignment Branch Condition size for unsubscribe is too high. [<4, 31, 6> 31.83/23]
(Metrics/AbcSize)
🤖 Prompt for AI Agents
In app/controllers/topics_controller.rb around lines 98 to 116, the unsubscribe
method does not handle the case when TopicUser.find_by returns nil, causing a
potential NoMethodError when accessing tu.notification_level. Fix this by adding
a guard clause or conditional to handle nil tu before proceeding. Additionally,
reduce method complexity by extracting the slug validation logic (the if
condition checking slugs and request format) into a separate private method,
similar to the approach used in the show action.
| <% if context_posts.present? %> | ||
| <div class='footer'>%{respond_instructions}</div> | ||
|
|
There was a problem hiding this comment.
Duplicate “respond_instructions” footer will render twice
Lines 6 and 19 both emit %{respond_instructions}. When context_posts are present the footer appears twice, bloating the email and risking localisation mistakes. Remove one instance (prefer the unconditional footer after the <hr>).
- <% if context_posts.present? %>
- <div class='footer'>%{respond_instructions}</div>
+ <% if context_posts.present? %>Also applies to: 17-20
🤖 Prompt for AI Agents
In app/views/email/notification.html.erb around lines 5 to 7 and 17 to 20, the
footer containing %{respond_instructions} is rendered twice when context_posts
are present, causing duplication in the email. Remove the conditional footer
inside the if block at line 6 and keep only the unconditional footer after the
<hr> to avoid duplication and localization issues.
|
|
||
| <% context_posts.each do |p| %> | ||
| <%= render :partial => 'email/post', :locals => {:post => p} %> | ||
| <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix invalid CSS class attribute
<h4 class='.previous-discussion'> contains a leading dot; HTML parsers treat the entire string (including the dot) as the class name.
-<h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
+<h4 class='previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4> | |
| <h4 class='previous-discussion'><%= t "user_notifications.previous_discussion" %></h4> |
🤖 Prompt for AI Agents
In app/views/email/notification.html.erb at line 10, the class attribute on the
h4 tag incorrectly includes a leading dot in the class name. Remove the leading
dot from the class attribute value so it reads class="previous-discussion"
instead of class=".previous-discussion" to correctly assign the CSS class.
2 participants
Test 2
Summary by CodeRabbit
New Features
User Interface
Localization
Bug Fixes
Documentation
Refactor/Style
Tests