FEATURE: per-topic unsubscribe option in emails

[akshayutture-augment]

Test 2

---

Replicated from ai-code-review-evaluation/discourse-greptile#2

[akshayutture-augment]

@claude review this PR

[akshayutture-augment]

@claude review this PR

[claude]

Claude finished @akshayutture-augment's task —— View job

---

Code Review: Per-Topic Unsubscribe Option in Emails

I've completed a thorough review of this PR. Here are my findings:

Overview

This PR adds a per-topic unsubscribe feature that allows users to change their notification settings for specific topics directly from email notifications. The implementation spans backend (Rails), frontend (Ember.js), and email templates.

---

🔴 Critical Issues

1. Missing Authentication/Authorization in Unsubscribe Route (Security)

Location: app/controllers/topics_controller.rb:98-116

The unsubscribe action requires authentication (before_filter :ensure_logged_in on line 28), but there's no token-based authentication mechanism for users clicking unsubscribe links in emails. This means:

• Users must be logged in to unsubscribe
• If a user clicks the unsubscribe link from their email client while not logged in, they'll be redirected to login first
• This creates a poor UX and may violate email unsubscribe best practices (RFC 2369, CAN-SPAM)

Recommendation: Implement a secure token-based unsubscribe mechanism similar to the digest unsubscribe feature (see DigestUnsubscribeKey usage in user_notifications.rb:84).

2. Potential Nil Pointer Exception

Location: app/controllers/topics_controller.rb:105-113

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!

If no TopicUser record exists, tu will be nil, causing a NoMethodError on line 107. This can happen if a user receives an email about a topic they haven't interacted with yet.

Recommendation: Add nil check or use find_or_initialize_by:

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

---

⚠️ Major Issues

3. Inconsistent Unsubscribe Link Header

Location: lib/email/message_builder.rb:106-108

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

The List-Unsubscribe header always points to user preferences, not the per-topic unsubscribe URL. This is inconsistent with the feature's purpose.

Recommendation: Update to use topic-specific unsubscribe URL when available:

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

4. Missing Error Handling in Unsubscribe Action

Location: app/controllers/topics_controller.rb:113

The tu.save! call will raise an exception if validation fails, but there's no error handling. A better approach would be to handle errors gracefully and provide user feedback.

Recommendation: Add error handling with proper response:

if tu.save
  perform_show_response
else
  flash[:error] = I18n.t('topic.unsubscribe.error')
  redirect_to @topic_view.topic.url
end

5. Simplified TopicUser Logic Removed Context

Location: app/models/topic_user.rb:77-118

The refactoring of the change method appears to have removed some nuanced logic around notification levels. The old implementation (based on git history) had more sophisticated handling. Need to verify this doesn't break existing functionality.

---

💡 Minor Issues & Suggestions

6. Typo in Controller

Location: app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6:5

stopNotificiationsText: function() {

Typo: "Notificiations" should be "Notifications"

7. Missing Localization Strings

Location: config/locales/client.en.yml and config/locales/server.en.yml

The PR adds references to topic.unsubscribe.* strings, but I don't see the actual string definitions in the locale files shown. Please verify these exist:

• topic.unsubscribe.stop_notifications
• topic.unsubscribe.change_notification_state

8. HTML Override Mutation

Location: lib/email/message_builder.rb:64-68

html_override.gsub!("%{respond_instructions}", respond_instructions)
html_override.gsub!("%{unsubscribe_link}", unsubscribe_link)

Mutating the html_override parameter with gsub! is a side effect that could cause issues if the same string is reused. Consider using gsub (non-mutating) or making a copy first.

9. Frontend Route Complexity

Location: app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6:4-9

The route loads the entire topic just to show an unsubscribe page. This seems heavy-handed. Consider a lighter endpoint that only returns the topic ID and title.

10. Empty View File

Location: app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6

This file is nearly empty (only 3 lines). Is it necessary? Ember.js conventions suggest you might not need a separate view file if there's no custom view logic.

---

✅ Positives

1. Good URL Structure: The unsubscribe URL follows REST conventions: /t/:slug/:topic_id/unsubscribe
2. Template Reuse: Good reuse of existing email template infrastructure
3. Proper Routing: Routes are properly configured in both frontend and backend
4. Maintains Backwards Compatibility: Changes to existing code are minimal and backwards compatible

---

🔍 Testing Recommendations

1. Test nil TopicUser scenario: Verify behavior when user has no TopicUser record
2. Test authentication flow: Ensure logged-out users can still unsubscribe (once token system is added)
3. Test notification level transitions: Verify all state transitions work correctly (watching → regular, tracking → muted, etc.)
4. Test email rendering: Ensure unsubscribe links render correctly in all email clients
5. Test List-Unsubscribe header: Verify email clients respect the header

---

📋 Summary

This PR implements a useful feature but has some critical security and reliability issues that should be addressed before merging:

Must Fix:

• Add token-based authentication for unsubscribe action
• Handle nil TopicUser case
• Fix List-Unsubscribe header

Should Fix:

• Add error handling
• Fix typo in controller
• Verify localization strings exist

Nice to Have:

• Optimize frontend route loading
• Remove unnecessary view file
• Make HTML override non-mutating

---

[github-actions]

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.