FEATURE: per-topic unsubscribe option in emails

[ShashankFC]

Test 2nn---nReplicated from ai-code-review-evaluation/discourse-greptile#2

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 2m 26s —— View job

---

Code Review Complete

I've reviewed this PR which adds a per-topic unsubscribe option to email notifications. Here's my analysis:

---

Overview

This feature allows users to unsubscribe from notifications for specific topics directly from email links, rather than having to visit their user preferences page. The implementation includes both frontend (Ember.js) and backend (Ruby) changes.

---

Issues Found

1. Typo in Controller Property Name - app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6:5

stopNotificiationsText: function() {

Issue: "Notificiations" should be "Notifications"
Severity: Low - Typo in property name

Fix this →

---

2. Missing Nil Check in Unsubscribe Action - app/controllers/topics_controller.rb:105-114

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!

Issue: If no TopicUser record exists (user hasn't interacted with the topic yet), tu will be nil, causing a NoMethodError when trying to access tu.notification_level.
Severity: High - Will cause 500 errors for users who haven't visited the topic

Recommendation: Handle the case where no TopicUser record exists:

tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id])

if tu
  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!
else
  # Create a new TopicUser record with muted notifications
  TopicUser.create!(
    user_id: current_user.id,
    topic_id: params[:topic_id],
    notification_level: TopicUser.notification_levels[:muted]
  )
end

Fix this →

---

3. Missing First/Last Visited Timestamps - app/controllers/topics_controller.rb:105-114

Even if we add the nil check above, the manual creation doesn't set first_visited_at and last_visited_at timestamps. The existing TopicUser.change method handles this properly. Consider using it instead:

tu = TopicUser.get(post.topic_id, user)

if tu && tu.notification_level > TopicUser.notification_levels[:regular]
  TopicUser.change(current_user.id, params[:topic_id], 
    notification_level: TopicUser.notification_levels[:regular])
else
  TopicUser.change(current_user.id, params[:topic_id], 
    notification_level: TopicUser.notification_levels[:muted])
end

The TopicUser.change method already handles creation when no record exists (lines 85-110 in topic_user.rb).

---

4. Inconsistent List-Unsubscribe Header - lib/email/message_builder.rb:107

result['List-Unsubscribe'] = "<#{template_args[:user_preferences_url]}>" if @opts[:add_unsubscribe_link]

Issue: The List-Unsubscribe header points to user preferences, but now we have a per-topic unsubscribe URL available via @opts[:unsubscribe_url]. The header should include both URLs according to RFC 2369.

Recommendation:

if @opts[:add_unsubscribe_link]
  unsubscribe_urls = ["<#{template_args[:user_preferences_url]}>"]
  unsubscribe_urls << "<#{template_args[:unsubscribe_url]}>" if template_args[:unsubscribe_url].present?
  result['List-Unsubscribe'] = unsubscribe_urls.join(', ')
end

Fix this →

---

Suggestions for Improvement

5. Route Definition Redundancy - config/routes.rb

The routes include both slug and non-slug versions:

get "t/:slug/:topic_id/unsubscribe" => "topics#unsubscribe"
get "t/:topic_id/unsubscribe" => "topics#unsubscribe"

This is consistent with existing patterns in the codebase, but the controller already handles redirecting to the correct URL with slug (lines 101-103). Good pattern!

---

6. Consider Flash Message After Unsubscribe - app/controllers/topics_controller.rb:98-116

The unsubscribe action modifies notification settings but doesn't provide explicit feedback. Consider adding a flash message:

flash[:notice] = I18n.t('topic.unsubscribed_successfully')

---

7. Template Variable Escaping - app/views/email/notification.html.erb:20

The template uses %{unsubscribe_link} as a placeholder that gets replaced later. While this works, it's unconventional for ERB templates. The replacement happens in lib/email/message_builder.rb:68. This approach is already used for respond_instructions, so it's consistent with the existing codebase pattern.

---

Positive Aspects

✅ Good security: The unsubscribe action requires authentication (before_filter :ensure_logged_in)

✅ Proper slug handling: Redirects to canonical URL if slug is missing or doesn't match (lines 101-103)

✅ Internationalization: All user-facing strings use I18n

✅ Consistent patterns: Follows existing Discourse conventions for routes, controllers, and Ember.js structure

✅ Database efficiency: Uses existing TopicUser table and notification level system

✅ Email template updates: Properly adds the unsubscribe link to both text and HTML email formats

---

Test Coverage

The spec file spec/components/email/message_builder_spec.rb was updated but I should note that there don't appear to be tests specifically for:

• The new unsubscribe controller action
• The Topic#unsubscribe_url method
• The unsubscribe link appearing in emails with the correct URL

Consider adding integration tests for the full unsubscribe flow.

---

Summary

This is a well-structured feature implementation that follows Discourse conventions. The main concerns are:

1. Critical: Missing nil check for TopicUser (will cause errors)
2. Important: Use TopicUser.change method instead of direct manipulation
3. Enhancement: Update List-Unsubscribe email header to include topic URL
4. Minor: Fix typo in controller property name

The security model is sound, and the feature integrates well with the existing notification system.

---