Skip to content
Jump to bottom

Automated Test: topic-email-management #317

Closed
wants to merge 1 commit into from
+156 −69
Closed

Automated Test: topic-email-management #317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion app/assets/javascripts/discourse/components/dropdown-button.js.es6
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,11 @@ export default Ember.Component.extend(StringBuffer, {
}.on('willDestroyElement'),

renderString(buffer) {
const title = this.get('title');
if (title) {
buffer.push("<h4 class='title'>" + title + "</h4>");
}

buffer.push("<h4 class='title'>" + this.get('title') + "</h4>");
buffer.push("<button class='btn standard dropdown-toggle' data-toggle='dropdown'>");
buffer.push(this.get('text'));
buffer.push("</button>");
Expand Down
9 changes: 9 additions & 0 deletions app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import ObjectController from "discourse/controllers/object";

export default ObjectController.extend({

stopNotificiationsText: function() {
return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") });
}.property("model.fancyTitle"),
Comment on lines +5 to +7
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Typo: stopNotificiationsText → stopNotificationsText

Same typo flagged in the template — extra 'i' in "Notificiations". Fix here and in unsubscribe.hbs together.

✏️ Proposed fix 
-  stopNotificiationsText: function() {
+  stopNotificationsText: function() {
     return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") });
   }.property("model.fancyTitle"),
📝 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
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 - 7, Rename the misspelled controller property stopNotificiationsText to
stopNotificationsText in topic-unsubscribe.js.es6 (preserve the
.property("model.fancyTitle") observer) and update the corresponding reference
in unsubscribe.hbs (and any other code/tests referencing stopNotificiationsText)
to the corrected stopNotificationsText so bindings and templates continue to
work.


})
1 change: 1 addition & 0 deletions app/assets/javascripts/discourse/routes/app-route-map.js.es6
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export default function() {
this.route('fromParamsNear', { path: '/:nearPost' });
});
this.resource('topicBySlug', { path: '/t/:slug' });
this.route('topicUnsubscribe', { path: '/t/:slug/:id/unsubscribe' });

this.resource('discovery', { path: '/' }, function() {
// top
Expand Down
25 changes: 13 additions & 12 deletions app/assets/javascripts/discourse/routes/topic-from-params.js.es6
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,32 +2,32 @@
export default Discourse.Route.extend({

// Avoid default model hook
model: function(p) { return p; },
model(params) { return params; },

setupController: function(controller, params) {
setupController(controller, params) {
params = params || {};
params.track_visit = true;
var topic = this.modelFor('topic'),
postStream = topic.get('postStream');

var topicController = this.controllerFor('topic'),
topicProgressController = this.controllerFor('topic-progress'),
composerController = this.controllerFor('composer');
const self = this,
topic = this.modelFor('topic'),
postStream = topic.get('postStream'),
topicController = this.controllerFor('topic'),
topicProgressController = this.controllerFor('topic-progress'),
composerController = this.controllerFor('composer');

// I sincerely hope no topic gets this many posts
if (params.nearPost === "last") { params.nearPost = 999999999; }

var self = this;
params.forceLoad = true;
postStream.refresh(params).then(function () {

postStream.refresh(params).then(function () {
// TODO we are seeing errors where closest post is null and this is exploding
// we need better handling and logging for this condition.

// The post we requested might not exist. Let's find the closest post
var closestPost = postStream.closestPostForPostNumber(params.nearPost || 1),
closest = closestPost.get('post_number'),
progress = postStream.progressIndexOfPost(closestPost);
const closestPost = postStream.closestPostForPostNumber(params.nearPost || 1),
closest = closestPost.get('post_number'),
progress = postStream.progressIndexOfPost(closestPost);

topicController.setProperties({
'model.currentPost': closest,
Expand All @@ -43,6 +43,7 @@ export default Discourse.Route.extend({
Ember.run.scheduleOnce('afterRender', function() {
self.appEvents.trigger('post:highlight', closest);
});

Discourse.URL.jumpToPost(closest);

if (topic.present('draft')) {
Expand Down
23 changes: 23 additions & 0 deletions app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import PostStream from "discourse/models/post-stream";

export default Discourse.Route.extend({
model(params) {
const topic = this.store.createRecord("topic", { id: params.id });
return PostStream.loadTopicView(params.id).then(json => {
topic.updateFromJson(json);
return topic;
});
},

afterModel(topic) {
// hide the notification reason text
topic.set("details.notificationReasonText", null);
},

actions: {
didTransition() {
this.controllerFor("application").set("showFooter", true);
return true;
}
}
});
8 changes: 8 additions & 0 deletions app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<div class="container">
<p>
{{{stopNotificiationsText}}}
</p>
<p>
{{i18n "topic.unsubscribe.change_notification_state"}} {{topic-notifications-button topic=model}}
</p>
Comment on lines +1 to +7
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Typo in property name: stopNotificiationsText → stopNotificationsText

The property name has an extra 'i' — Notificiations instead of Notifications. While this is consistent between the template and controller (so it works), it will be confusing for future maintainers and should be corrected in both places.

✏️ Proposed fix 
 <div class="container">
   <p>
-    {{{stopNotificiationsText}}}
+    {{{stopNotificationsText}}}
   </p>
   <p>
     {{i18n "topic.unsubscribe.change_notification_state"}} {{topic-notifications-button topic=model}}
   </p>
 </div>
🤖 Prompt for AI Agents 
In `@app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs` around
lines 1 - 7, Rename the misspelled property stopNotificiationsText to
stopNotificationsText everywhere it’s defined and referenced: update the
template usage ({{{stopNotificiationsText}}}) and change the corresponding
property in the controller/component that provides it (where
stopNotificiationsText is declared/computed), then run tests/grep to update any
other occurrences (helpers, tests, or usages like bindings) to the corrected
stopNotificationsText to keep behavior identical but fix the typo.

</div>
3 changes: 3 additions & 0 deletions app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default Discourse.View.extend({
classNames: ["topic-unsubscribe"]
});
12 changes: 12 additions & 0 deletions app/assets/stylesheets/common/base/topic.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,15 @@
// Top of bullet aligns with top of line - adjust line height to vertically align bullet.
line-height: 0.8;
}

.topic-unsubscribe {
.notification-options {
display: inline-block;
.dropdown-toggle {
float: none;
}
.dropdown-menu {
bottom: initial;
}
}
}
26 changes: 24 additions & 2 deletions app/controllers/topics_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,12 @@ class TopicsController < ApplicationController
:bulk,
:reset_new,
:change_post_owners,
:bookmark]
:bookmark,
:unsubscribe]

before_filter :consider_user_for_promotion, only: :show

skip_before_filter :check_xhr, only: [:show, :feed]
skip_before_filter :check_xhr, only: [:show, :unsubscribe, :feed]

def id_for_slug
topic = Topic.find_by(slug: params[:slug].downcase)
Expand Down Expand Up @@ -94,6 +95,26 @@ def show
raise ex
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.notification_level > TopicUser.notification_levels[:regular]
tu.notification_level = TopicUser.notification_levels[:regular]
else
tu.notification_level = TopicUser.notification_levels[:muted]
end
Comment on lines +107 to +111
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Toggle semantics on a GET "unsubscribe" action are problematic.

Two concerns:

  1. GET mutating state: This action is routed via GET (for email link clickability), but it modifies TopicUser notification level. This violates HTTP semantics and can be triggered by prefetch/preload mechanisms in email clients, browsers, or link scanners. Consider rendering a confirmation page on GET and performing the actual mutation on POST.

  2. Toggle is unintuitive for "unsubscribe": Users clicking an "unsubscribe" link expect a one-way action (stop notifications). The current toggle means clicking the link a second time would re-subscribe, which is unexpected. Consider always setting the level to :muted (or :regular at minimum) instead of toggling.

🤖 Prompt for AI Agents 
In `@app/controllers/topics_controller.rb` around lines 107 - 111, The current GET
handler mutates TopicUser.notification_level (tu.notification_level) by toggling
between notification_levels[:regular] and [:muted]; change it so GET no longer
mutates state: have the controller render a confirmation page on GET (showing
the pending unsubscribe) and perform the actual update in a separate POST action
(e.g., unsubscribe_confirm or the same action constrained to POST). Also change
the mutation logic to be one-way: when performing the POST update, set
tu.notification_level = TopicUser.notification_levels[:muted] (do not toggle)
and persist the change; keep references to the existing tu variable and
TopicUser.notification_levels to locate the code.


tu.save!

perform_show_response
end
Comment on lines +98 to +116
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

TopicUser.find_by can return nil, causing NoMethodError on line 107.

If the current user has never interacted with the topic (no TopicUser record exists), find_by returns nil, and tu.notification_level on line 107 will raise NoMethodError. This is a realistic scenario — a user may receive an email notification but have no TopicUser row yet.

🐛 Proposed fix 
-    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!
+    tu = TopicUser.find_or_initialize_by(user_id: current_user.id, topic_id: params[:topic_id])
+    tu.notification_level = TopicUser.notification_levels[:muted]
+    tu.save!
🧰 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, In unsubscribe,
TopicUser.find_by may return nil causing a NoMethodError on tu; update the
unsubscribe action to handle a missing TopicUser: fetch with
TopicUser.find_or_initialize_by(user_id: current_user.id, topic_id:
params[:topic_id]) (or explicitly return/redirect if you prefer), then set
tu.notification_level using the same conditional logic and call tu.save!; keep
the rest of the flow (perform_show_response and redirect logic) unchanged and
reference the tu/local variable, TopicUser.find_by/find_or_initialize_by, and
the unsubscribe method.


def wordpress
params.require(:best)
params.require(:topic_id)
Expand Down Expand Up @@ -476,6 +497,7 @@ def perform_show_response
format.html do
@description_meta = @topic_view.topic.excerpt
store_preloaded("topic_#{@topic_view.topic.id}", MultiJson.dump(topic_view_serializer))
render :show
end

format.json do
Expand Down
5 changes: 2 additions & 3 deletions app/mailers/user_notifications.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,7 @@ def send_notification_email(opts)
context: context,
username: username,
add_unsubscribe_link: true,
unsubscribe_url: post.topic.unsubscribe_url,
allow_reply_by_email: allow_reply_by_email,
use_site_subject: use_site_subject,
add_re_to_subject: add_re_to_subject,
Expand All @@ -306,9 +307,7 @@ def send_notification_email(opts)
}

# If we have a display name, change the from address
if from_alias.present?
email_opts[:from_alias] = from_alias
end
email_opts[:from_alias] = from_alias if from_alias.present?

TopicUser.change(user.id, post.topic_id, last_emailed_post_number: post.post_number)

Expand Down
4 changes: 4 additions & 0 deletions app/models/topic.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,10 @@ def relative_url(post_number=nil)
url
end

def unsubscribe_url
"#{url}/unsubscribe"
end

def clear_pin_for(user)
return unless user.present?
TopicUser.change(user.id, id, cleared_pinned_at: Time.now)
Expand Down
21 changes: 8 additions & 13 deletions app/models/topic_user.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@ class TopicUser < ActiveRecord::Base

scope :tracking, lambda { |topic_id|
where(topic_id: topic_id)
.where("COALESCE(topic_users.notification_level, :regular) >= :tracking",
regular: TopicUser.notification_levels[:regular], tracking: TopicUser.notification_levels[:tracking])
.where("COALESCE(topic_users.notification_level, :regular) >= :tracking",
regular: TopicUser.notification_levels[:regular],
tracking: TopicUser.notification_levels[:tracking])
}

# Class methods
Expand Down Expand Up @@ -58,13 +59,9 @@ def lookup_for(user, topics)

def create_lookup(topic_users)
topic_users = topic_users.to_a

result = {}
return result if topic_users.blank?

topic_users.each do |ftu|
result[ftu.topic_id] = ftu
end
topic_users.each { |ftu| result[ftu.topic_id] = ftu }
result
end

Expand Down Expand Up @@ -113,11 +110,9 @@ def change(user_id, topic_id, attrs)
end

if attrs[:notification_level]
MessageBus.publish("/topic/#{topic_id}",
{notification_level_change: attrs[:notification_level]}, user_ids: [user_id])
MessageBus.publish("/topic/#{topic_id}", { notification_level_change: attrs[:notification_level] }, user_ids: [user_id])
end


rescue ActiveRecord::RecordNotUnique
# In case of a race condition to insert, do nothing
end
Expand All @@ -127,7 +122,7 @@ def track_visit!(topic,user)
user_id = user.is_a?(User) ? user.id : topic

now = DateTime.now
rows = TopicUser.where({topic_id: topic_id, user_id: user_id}).update_all({last_visited_at: now})
rows = TopicUser.where(topic_id: topic_id, user_id: user_id).update_all(last_visited_at: now)
if rows == 0
TopicUser.create(topic_id: topic_id, user_id: user_id, last_visited_at: now, first_visited_at: now)
else
Expand Down Expand Up @@ -196,7 +191,7 @@ def update_last_read(user, topic_id, post_number, msecs, opts={})
end

if before != after
MessageBus.publish("/topic/#{topic_id}", {notification_level_change: after}, user_ids: [user.id])
MessageBus.publish("/topic/#{topic_id}", { notification_level_change: after }, user_ids: [user.id])
end
end

Expand All @@ -220,7 +215,7 @@ def update_last_read(user, topic_id, post_number, msecs, opts={})
WHERE ftu.user_id = :user_id and ftu.topic_id = :topic_id)",
args)

MessageBus.publish("/topic/#{topic_id}", {notification_level_change: args[:new_status]}, user_ids: [user.id])
MessageBus.publish("/topic/#{topic_id}", { notification_level_change: args[:new_status] }, user_ids: [user.id])
end
end

Expand Down
40 changes: 19 additions & 21 deletions app/views/email/notification.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,29 @@
<div id='main' class=<%= classes %>>

<%= render :partial => 'email/post', :locals => {:post => post} %>
<%= render partial: 'email/post', locals: { post: post } %>
<% if context_posts.present? %>
<div class='footer'>
%{respond_instructions}
</div>
<hr>
<h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
<% 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>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

CSS selector dot in HTML class attribute.

class='.previous-discussion' includes a leading dot, which is CSS selector syntax and not valid as an HTML class name. The dot will be literally included in the class, so any CSS rule targeting .previous-discussion won't match.

🐛 Proposed fix 
-    <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.

Suggested change
<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 element is using a CSS selector dot (class='.previous-discussion') which
results in a literal dot in the HTML class and prevents selectors like
.previous-discussion from matching; edit the H4 tag in notification.html.erb
(the element with <%= t "user_notifications.previous_discussion" %>) to remove
the leading dot so the class is previous-discussion, and update any CSS if it
was accidentally written to target the dotted literal class.


<% context_posts.each do |p| %>
<%= render partial: 'email/post', locals: { post: p } %>
<% end %>
<% end %>
<% end %>

<hr>
<hr>

<div class='footer'>%{respond_instructions}</div>
<div class='footer'>%{unsubscribe_link}</div>
Comment on lines +5 to +20
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

%{respond_instructions} is rendered twice when context_posts are present.

Line 6 outputs %{respond_instructions} inside the if context_posts.present? block, and line 19 outputs it again unconditionally. When context posts exist, the respond instructions will appear twice in the email.

If the intent is to show respond instructions between the main post and the context posts (as a separator), and then again at the bottom, this is likely unintentional duplication. Otherwise, remove one occurrence.

🐛 Suggested fix — keep only the unconditional footer 
   <% if context_posts.present? %>
-    <div class='footer'>%{respond_instructions}</div>
-
     <hr>

     <h4 class='previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>

     <% context_posts.each do |p| %>
       <%= render partial: 'email/post', locals: { post: p } %>
     <% end %>
   <% end %>

   <hr>

   <div class='footer'>%{respond_instructions}</div>
   <div class='footer'>%{unsubscribe_link}</div>
📝 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
<% 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 %>
<% end %>
<% end %>
<hr>
<hr>
<div class='footer'>%{respond_instructions}</div>
<div class='footer'>%{unsubscribe_link}</div>
<% if context_posts.present? %>
<hr>
<h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
<% context_posts.each do |p| %>
<%= render partial: 'email/post', locals: { post: p } %>
<% end %>
<% end %>
<hr>
<div class='footer'>%{respond_instructions}</div>
<div class='footer'>%{unsubscribe_link}</div>
🤖 Prompt for AI Agents 
In `@app/views/email/notification.html.erb` around lines 5 - 20, The
respond_instructions footer is rendered twice when context_posts.present? is
true; remove the duplicated instance inside the conditional block so only the
unconditional "<div class='footer'>%{respond_instructions}</div>" remains at the
bottom, leaving the context_posts block (and its render partial: 'email/post')
intact and unchanged.


<div class='footer'>
%{respond_instructions}
</div>
<div class='footer'>
%{unsubscribe_link}
</div>
</div>

<div itemscope itemtype="http://schema.org/EmailMessage" style="display:none">
<div itemprop="action" itemscope itemtype="http://schema.org/ViewAction">
<link itemprop="url" href="<%= Discourse.base_url %><%= post.url %>" />
<meta itemprop="name" content="<%= t 'read_full_topic' %>"/>
</div>
<div itemprop="action" itemscope itemtype="http://schema.org/ViewAction">
<link itemprop="url" href="<%= Discourse.base_url %><%= post.url %>" />
<meta itemprop="name" content="<%= t 'read_full_topic' %>"/>
</div>
</div>
4 changes: 3 additions & 1 deletion config/locales/client.en.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -981,6 +981,9 @@ en:
search: "There are no more search results."

topic:
unsubscribe:
stop_notifications: "You will stop receiving notifications for <strong>{{title}}</strong>."
change_notification_state: "You can change your notification state"
filter_to: "{{post_count}} posts in topic"
create: 'New Topic'
create_long: 'Create a new Topic'
Expand Down Expand Up @@ -1014,7 +1017,6 @@ en:
new_posts:
one: "there is 1 new post in this topic since you last read it"
other: "there are {{count}} new posts in this topic since you last read it"

likes:
one: "there is 1 like in this topic"
other: "there are {{count}} likes in this topic"
Expand Down
5 changes: 4 additions & 1 deletion config/locales/server.en.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1849,7 +1849,10 @@ en:
subject_template: "Downloading remote images disabled"
text_body_template: "The `download_remote_images_to_local` setting was disabled because the disk space limit at `download_remote_images_threshold` was reached."

unsubscribe_link: "To unsubscribe from these emails, visit your [user preferences](%{user_preferences_url})."
unsubscribe_link: |
To unsubscribe from these emails, visit your [user preferences](%{user_preferences_url}).
To stop receiving notifications about this particular topic, [click here](%{unsubscribe_url}).
subject_re: "Re: "
subject_pm: "[PM] "
Expand Down
8 changes: 5 additions & 3 deletions config/routes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,10 +434,12 @@
# Topic routes
get "t/id_for/:slug" => "topics#id_for_slug"
get "t/:slug/:topic_id/wordpress" => "topics#wordpress", constraints: {topic_id: /\d+/}
get "t/:slug/:topic_id/moderator-liked" => "topics#moderator_liked", constraints: {topic_id: /\d+/}
get "t/:topic_id/wordpress" => "topics#wordpress", constraints: {topic_id: /\d+/}
get "t/:slug/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/, post_number: /\d+/}
get "t/:topic_id/summary" => "topics#show", constraints: {topic_id: /\d+/, post_number: /\d+/}
get "t/:slug/:topic_id/moderator-liked" => "topics#moderator_liked", constraints: {topic_id: /\d+/}
get "t/:slug/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Missing defaults: {summary: true} on the slug-less summary route.

Line 439 sets defaults: {summary: true} for t/:slug/:topic_id/summary, but line 442 (t/:topic_id/summary) omits it. Both routes point to topics#show, so the slug-less variant will not set params[:summary] to true, likely breaking the summary view when accessed without a slug.

🐛 Proposed fix 
-  get "t/:topic_id/summary" => "topics#show", constraints: {topic_id: /\d+/}
+  get "t/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/}

Also applies to: 442-442

🤖 Prompt for AI Agents 
In `@config/routes.rb` at line 439, The slug-less summary route (get
"t/:topic_id/summary") is missing defaults: {summary: true}, so params[:summary]
won't be set when hitting topics#show; update that route to include defaults:
{summary: true} like the slugged route (get "t/:slug/:topic_id/summary" =>
"topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/}) so
both routes set params[:summary] for TopicsController#show.

get "t/:slug/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/}
get "t/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/}
Comment on lines +440 to +441
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, find the topics controller file
find . -type f -name "*topics*controller*"

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 271

🏁 Script executed:

# Check the unsubscribe action in the topics controller
ast-grep --pattern $'def unsubscribe
  $$$
end'

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 2521

Refactor unsubscribe routes to use POST/DELETE or implement a GET confirmation flow.

The topics#unsubscribe action on lines 98-116 of app/controllers/topics_controller.rb directly mutates state by modifying the user's notification level and calling tu.save! on GET requests. This creates a serious security and usability issue: email link scanners, browser prefetching, and web crawlers can trigger the unsubscribe without user intent.

Consider either:

  1. Change the routes to use POST/DELETE instead of GET
  2. Or keep GET for a confirmation page but move the actual state mutation to a separate POST/DELETE action
🤖 Prompt for AI Agents 
In `@config/routes.rb` around lines 440 - 441, The unsubscribe route currently
maps GET "t/:slug/:topic_id/unsubscribe" and GET "t/:topic_id/unsubscribe" to
topics#unsubscribe which mutates state (calls tu.save!) and is unsafe; change
the routes to use a non-idempotent verb (POST or DELETE) for the state-changing
endpoint and/or implement a GET confirmation flow: keep a GET action (e.g.,
topics#unsubscribe_confirm) that renders a confirmation page and move the actual
mutation into a separate POST/DELETE action (e.g., topics#unsubscribe or
topics#destroy_subscription) which performs the tu.save! logic; update the
routes to point GET -> topics#unsubscribe_confirm and POST/DELETE ->
topics#unsubscribe (or new name), and adjust the controller
(topics_controller.rb) to split the confirmation and mutation accordingly and
guard the mutation behind the non-GET verb.

get "t/:topic_id/summary" => "topics#show", constraints: {topic_id: /\d+/}
put "t/:slug/:topic_id" => "topics#update", constraints: {topic_id: /\d+/}
put "t/:slug/:topic_id/star" => "topics#star", constraints: {topic_id: /\d+/}
put "t/:topic_id/star" => "topics#star", constraints: {topic_id: /\d+/}
Expand Down