Automated Test: topic-email-management #325
Closed
+156
−69
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughWalkthroughThis PR introduces a topic unsubscribe feature enabling users to manage notification preferences. Changes span JavaScript components, routes, templates, backend controller actions, email integration, and localization to support a new unsubscribe workflow accessible via both route and email links. Changes
Sequence DiagramsequenceDiagram
actor User
participant Browser
participant TopicsController
participant TopicModel
participant PostStream
participant RouteHandler
participant Template
User->>Browser: Click unsubscribe link
Browser->>TopicsController: GET /t/:slug/:id/unsubscribe
TopicsController->>TopicModel: Create/load topic
TopicsController->>PostStream: loadTopicView(params.id)
PostStream-->>TopicModel: Return topic data with JSON update
TopicsController->>TopicModel: Clear notificationReasonText
TopicsController->>RouteHandler: Trigger didTransition
RouteHandler->>Browser: Enable footer visibility
TopicsController->>Template: Render unsubscribe template
Template-->>Browser: Display notification options
Browser-->>User: Show confirmation UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6`:
- Around line 4-9: The model(params) currently uses params.id only, causing
failures when routes supply topic_id; update model in topic-unsubscribe.js.es6
to derive an id value (e.g. const topicId = params.id || params.topic_id), use
topicId when calling this.store.createRecord("topic", { id: topicId }) and when
calling PostStream.loadTopicView(topicId), then proceed to updateFromJson and
return the topic as before.
In `@app/controllers/topics_controller.rb`:
- Around line 98-116: The unsubscribe action can nil-deref tu because
TopicUser.find_by may return nil; update the logic in unsubscribe to ensure a
TopicUser exists before accessing notification_level — use
TopicUser.find_or_initialize_by(user_id: current_user.id, topic_id:
params[:topic_id]) (or TopicUser.change/create) to get a record (assign
user_id/topic_id on the initializer), then set tu.notification_level to
TopicUser.notification_levels[:regular] or [:muted] as before, save! and call
perform_show_response; ensure any new record has sensible defaults so
tu.notification_level reads/writes safely.
In `@app/views/email/notification.html.erb`:
- Around line 5-14: Remove the duplicate respond instructions and fix the header
class: ensure %{respond_instructions} is rendered only once when
context_posts.present? (remove the extra occurrence under the posts loop or the
footer div so it appears a single time), and change the h4 class from
'.previous-discussion' to 'previous-discussion' so the selector matches; update
the template around the context_posts block and the h4 element (referencing
context_posts, the footer div containing %{respond_instructions}, and the h4
with class '.previous-discussion') accordingly.
In `@config/routes.rb`:
- Around line 440-441: Change the two GET routes for the unsubscribe action to
use POST so state changes go through non-idempotent HTTP methods: replace get
"t/:slug/:topic_id/unsubscribe" and get "t/:topic_id/unsubscribe" with post
equivalents routing to topics#unsubscribe; then update any links that call
TopicsController#unsubscribe to use method: :post or button_to (or submit a
form) so the request includes the proper verb; alternatively, if you prefer GET
links, require and verify a signed token in TopicsController#unsubscribe (the
code path that calls tu.save! on the TopicUser) and reject unsigned requests—do
one of these fixes to ensure the unsubscribe mutation is protected against CSRF
and accidental GET-triggering.
In `@lib/email/message_builder.rb`:
- Around line 61-68: RuboCop flags the assignment-in-condition in the block
guarded by `@opts`[:add_unsubscribe_link]; pull the assignment out or parenthesize
it to make intent explicit: assign response_instructions =
`@template_args`[:respond_instructions] on its own line (or wrap the assignment in
parentheses if you prefer) before the if that checks it, then proceed to call
PrettyText.cook(response_instructions).html_safe and
html_override.gsub!("%{respond_instructions}", respond_instructions); keep the
rest of the unsubscribe_link logic unchanged.
🧹 Nitpick comments (3)
app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs (1)
1-7: Consider fixing the typo instopNotificiationsTextfor readability.
Renaming tostopNotificationsTextwill reduce confusion; update the controller/computed property accordingly.♻️ Suggested template rename
- {{{stopNotificiationsText}}} + {{{stopNotificationsText}}}spec/components/email/message_builder_spec.rb (1)
169-172: Add a direct assertion forunsubscribe_urlinterpolation.Right now the new argument is passed but not verified. A simple expectation will prevent regressions.
🧪 Proposed test addition
it "has the user preferences url in the body" do expect(message_with_unsubscribe.body).to match(builder.template_args[:user_preferences_url]) end + + it "includes the unsubscribe_url in the body" do + expect(message_with_unsubscribe.body).to include("/t/1234/unsubscribe") + endapp/controllers/topics_controller.rb (1)
8-28: Add trailing comma for multiline array style.RuboCop expects a trailing comma after the last element in a multiline array.
🎨 Suggested style fix
- :bookmark, - :unsubscribe] + :bookmark, + :unsubscribe, + ]
Comment on lines
+4
to
+9
| 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.
Handle both id and topic_id route params.
model(params) uses params.id exclusively (Line 5-6). If the non-slug route passes topic_id, this will be undefined and loadTopicView will fail. Add a fallback to params.topic_id.
🛠️ Proposed fix
model(params) {
- const topic = this.store.createRecord("topic", { id: params.id });
- return PostStream.loadTopicView(params.id).then(json => {
+ const topicId = params.id || params.topic_id;
+ const topic = this.store.createRecord("topic", { id: topicId });
+ return PostStream.loadTopicView(topicId).then(json => {
topic.updateFromJson(json);
return topic;
});
},📝 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.
Suggested change
| 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 topicId = params.id || params.topic_id; | |
| const topic = this.store.createRecord("topic", { id: topicId }); | |
| return PostStream.loadTopicView(topicId).then(json => { | |
| topic.updateFromJson(json); | |
| return topic; | |
| }); | |
| }, |
🤖 Prompt for AI Agents
In `@app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6` around
lines 4 - 9, The model(params) currently uses params.id only, causing failures
when routes supply topic_id; update model in topic-unsubscribe.js.es6 to derive
an id value (e.g. const topicId = params.id || params.topic_id), use topicId
when calling this.store.createRecord("topic", { id: topicId }) and when calling
PostStream.loadTopicView(topicId), then proceed to updateFromJson and return the
topic as before.
Comment on lines
+98
to
+116
| 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 missing TopicUser to avoid nil dereference.
TopicUser.find_by can return nil, which will raise on tu.notification_level. Use find_or_initialize_by or TopicUser.change to ensure a record exists.
🛠️ Suggested fix
- tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id])
+ tu = TopicUser.find_or_initialize_by(user_id: current_user.id, topic_id: params[:topic_id])
+ tu.notification_level ||= TopicUser.notification_levels[:regular]
if tu.notification_level > TopicUser.notification_levels[:regular]
tu.notification_level = TopicUser.notification_levels[:regular]
else
tu.notification_level = TopicUser.notification_levels[:muted]
end🧰 Tools
🪛 RuboCop (1.84.0)
[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 - 116, The unsubscribe
action can nil-deref tu because TopicUser.find_by may return nil; update the
logic in unsubscribe to ensure a TopicUser exists before accessing
notification_level — use TopicUser.find_or_initialize_by(user_id:
current_user.id, topic_id: params[:topic_id]) (or TopicUser.change/create) to
get a record (assign user_id/topic_id on the initializer), then set
tu.notification_level to TopicUser.notification_levels[:regular] or [:muted] as
before, save! and call perform_show_response; ensure any new record has sensible
defaults so tu.notification_level reads/writes safely.
Comment on lines
+5
to
+14
| <% if context_posts.present? %> | ||
| <div class='footer'>%{respond_instructions}</div> | ||
|
|
||
| <hr> | ||
|
|
||
| <% context_posts.each do |p| %> | ||
| <%= render :partial => 'email/post', :locals => {:post => p} %> | ||
| <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4> | ||
|
|
||
| <% context_posts.each do |p| %> | ||
| <%= render partial: 'email/post', locals: { post: p } %> | ||
| <% end %> |
There was a problem hiding this comment.
Avoid duplicate respond instructions and fix the header class.
When context_posts is present, %{respond_instructions} appears twice (Line 6 and Line 19). Also, the class attribute includes a leading dot (Line 10), which won’t match the intended CSS selector.
✅ Suggested fix
- <% if context_posts.present? %>
- <div class='footer'>%{respond_instructions}</div>
+ <% if context_posts.present? %>
<hr>
- <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
+ <h4 class='previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
<% context_posts.each do |p| %>
<%= render partial: 'email/post', locals: { post: p } %>
<% end %>
<% end %>Also applies to: 17-20
🤖 Prompt for AI Agents
In `@app/views/email/notification.html.erb` around lines 5 - 14, Remove the
duplicate respond instructions and fix the header class: ensure
%{respond_instructions} is rendered only once when context_posts.present?
(remove the extra occurrence under the posts loop or the footer div so it
appears a single time), and change the h4 class from '.previous-discussion' to
'previous-discussion' so the selector matches; update the template around the
context_posts block and the h4 element (referencing context_posts, the footer
div containing %{respond_instructions}, and the h4 with class
'.previous-discussion') accordingly.
Comment on lines
+440
to
+441
| get "t/:slug/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/} | ||
| get "t/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the unsubscribe action and related CSRF/authenticity checks
rg -n --type=rb -C5 'def unsubscribe' app/controllers/topics_controller.rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Find the topics controller file
fd -n "topics_controller" .
# Search for unsubscribe action
rg -n 'def unsubscribe' --glob='*controller*'
# Search for CSRF/authenticity checks
rg -n 'verify_authenticity_token|skip_before_action|protect_from_forgery' --glob='*topics*controller*' -C3Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 450
🏁 Script executed:
#!/bin/bash
# Read the unsubscribe method in topics_controller.rb
cat -n app/controllers/topics_controller.rb | sed -n '90,110p'
# Check for controller-level CSRF protection settings
rg 'protect_from_forgery|skip_before_action' app/controllers/topics_controller.rb -B2 -A2Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 964
🏁 Script executed:
#!/bin/bash
# Continue reading the unsubscribe method to see the full implementation
cat -n app/controllers/topics_controller.rb | sed -n '105,125p'
# Check for controller-level CSRF protection and skip_before_action for unsubscribe
rg 'protect_from_forgery|skip_before_action.*unsubscribe' app/controllers/topics_controller.rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 942
🏁 Script executed:
#!/bin/bash
# Check the top of topics_controller.rb for protect_from_forgery and class definition
head -50 app/controllers/topics_controller.rb | cat -n
# Also search for any skip_before_action at controller level
rg 'skip_before_action' app/controllers/topics_controller.rb -B1 -A1Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 2503
🏁 Script executed:
#!/bin/bash
# Check ApplicationController for protect_from_forgery configuration
rg 'protect_from_forgery|class ApplicationController' app/controllers/application_controller.rb -B2 -A5
# Also check if unsubscribe explicitly skips CSRF
rg 'skip_before_action|skip_before_filter' app/controllers/topics_controller.rb | grep -i unsubscribeRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 619
Use POST instead of GET for the unsubscribe action.
The unsubscribe action mutates user notification state (tu.save! on line 113) but is exposed via GET routes (lines 440-441). Rails' protect_from_forgery does not protect GET requests by default—only POST/PUT/PATCH/DELETE. This allows browsers to trigger unsubscribes via link prefetch, image preloading, or CSRF attacks without requiring a CSRF token.
Convert these routes to POST, or require a signed token in the URL to authorize the state change.
🤖 Prompt for AI Agents
In `@config/routes.rb` around lines 440 - 441, Change the two GET routes for the
unsubscribe action to use POST so state changes go through non-idempotent HTTP
methods: replace get "t/:slug/:topic_id/unsubscribe" and get
"t/:topic_id/unsubscribe" with post equivalents routing to topics#unsubscribe;
then update any links that call TopicsController#unsubscribe to use method:
:post or button_to (or submit a form) so the request includes the proper verb;
alternatively, if you prefer GET links, require and verify a signed token in
TopicsController#unsubscribe (the code path that calls tu.save! on the
TopicUser) and reject unsigned requests—do one of these fixes to ensure the
unsubscribe mutation is protected against CSRF and accidental GET-triggering.
Comment on lines
+61
to
+68
| if @opts[:add_unsubscribe_link] | ||
| if response_instructions = @template_args[:respond_instructions] | ||
| respond_instructions = PrettyText.cook(response_instructions).html_safe | ||
| html_override.gsub!("%{respond_instructions}", respond_instructions) | ||
| end | ||
|
|
||
| unsubscribe_link = PrettyText.cook(I18n.t('unsubscribe_link', template_args)).html_safe | ||
| html_override.gsub!("%{unsubscribe_link}",unsubscribe_link) | ||
| html_override.gsub!("%{unsubscribe_link}", unsubscribe_link) |
There was a problem hiding this comment.
Wrap assignment in condition to satisfy lint and avoid ambiguity.
RuboCop flags the assignment-in-condition pattern here. Parenthesizing makes the intent explicit.
🔧 Proposed tweak
- if response_instructions = `@template_args`[:respond_instructions]
+ if (response_instructions = `@template_args`[:respond_instructions])
respond_instructions = PrettyText.cook(response_instructions).html_safe
html_override.gsub!("%{respond_instructions}", respond_instructions)
end🧰 Tools
🪛 RuboCop (1.84.0)
[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)
🤖 Prompt for AI Agents
In `@lib/email/message_builder.rb` around lines 61 - 68, RuboCop flags the
assignment-in-condition in the block guarded by `@opts`[:add_unsubscribe_link];
pull the assignment out or parenthesize it to make intent explicit: assign
response_instructions = `@template_args`[:respond_instructions] on its own line
(or wrap the assignment in parentheses if you prefer) before the if that checks
it, then proceed to call PrettyText.cook(response_instructions).html_safe and
html_override.gsub!("%{respond_instructions}", respond_instructions); keep the
rest of the unsubscribe_link logic unchanged.
Sign in
to join this conversation on GitHub.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes