Skip to content
Jump to bottom

Automated Test: topic-email-management #309

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

Automated Test: topic-email-management #309

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"),

})
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"). This is consistent with the controller so it works, but it should be fixed in both places to avoid confusion for future maintainers.

Also, the triple-stache {{{ }}} renders unescaped HTML. This is needed because the i18n string contains <strong> tags, and fancyTitle is sanitized server-side, so the XSS risk is mitigated. Worth noting for future reference.

Proposed fix 
-  {{{stopNotificiationsText}}}
+  {{{stopNotificationsText}}}

Also fix in the controller (topic-unsubscribe.js.es6):

-  stopNotificiationsText: function() {
+  stopNotificationsText: function() {
🤖 Prompt for AI Agents 
In `@app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs` around
lines 1 - 7, Rename the misspelled property stopNotificiationsText to
stopNotificationsText in the template and the corresponding controller
(topic-unsubscribe controller) so both places use the correct identifier; update
the template's {{{stopNotificiationsText}}} to {{{stopNotificationsText}}} and
the controller export/property that supplies it, keeping the triple-stache
unchanged because the i18n string intentionally contains HTML and is sanitized
server-side.

</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

tu.save!
Comment on lines +105 to +113
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.

If the user has never interacted with this topic, no TopicUser record exists. find_by returns nil, and line 107 will raise NoMethodError: undefined method 'notification_level' for nil:NilClass.

🐛 Proposed fix: handle missing TopicUser 
-    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_by(user_id: current_user.id, topic_id: params[:topic_id])
+
+    if tu.nil?
+      TopicUser.change(current_user.id, params[:topic_id],
+        notification_level: TopicUser.notification_levels[:muted])
+    else
+      new_level = if tu.notification_level > TopicUser.notification_levels[:regular]
+                    TopicUser.notification_levels[:regular]
+                  else
+                    TopicUser.notification_levels[:muted]
+                  end
+      tu.update!(notification_level: new_level)
+    end
🤖 Prompt for AI Agents 
In `@app/controllers/topics_controller.rb` around lines 105 - 113,
TopicUser.find_by can return nil, so guard the code that reads/sets
tu.notification_level: after calling TopicUser.find_by(user_id: current_user.id,
topic_id: params[:topic_id]) check whether tu is nil and handle it (either
create a new TopicUser for current_user and params[:topic_id] before toggling
notification_level, or return a suitable response like head :not_found or
redirect), then perform the existing toggle logic using
TopicUser.notification_levels and call tu.save! only when tu is present.


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 | 🟠 Major

State mutation via GET request violates HTTP semantics.

The unsubscribe action modifies TopicUser.notification_level but is routed as a GET (see config/routes.rb lines 440–441). GET requests must be safe/idempotent per HTTP spec. This also exposes the action to CSRF via link prefetching, browser preloading, and crawler visits — any of which could silently toggle a user's notification state.

Consider changing the route to PUT or POST, and rendering a confirmation page on GET that submits via a form.

🧰 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 currently mutates TopicUser.notification_level in the controller method
unsubscribe (using TopicView and TopicUser.find_by) while being routed as a GET;
change the route to a non-safe verb (POST or PUT) and split the controller so
GET unsubscribe renders a confirmation page (no state changes) and the actual
mutation happens only in a new POST/PUT handler (e.g., unsubscribe_confirm or
the same unsubscribe action responding to POST/PUT). Move the logic that looks
up tu (TopicUser.find_by(...)) and updates tu.notification_level and tu.save!
into the POST/PUT branch, ensure CSRF protection is enabled for the form
submission, and keep perform_show_response only in the mutating handler.


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

Erroneous dot in HTML class attribute value.

class='.previous-discussion' should be class='previous-discussion'. The leading dot is CSS selector syntax and shouldn't appear in the class attribute itself — any CSS rule targeting .previous-discussion won't match .previous-discussion as a literal class name.

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, In the H4 element in
app/views/email/notification.html.erb (the line rendering t
"user_notifications.previous_discussion"), remove the erroneous leading dot from
the class attribute so the element uses the literal class name
previous-discussion (i.e., change class='.previous-discussion' to
class='previous-discussion') so CSS rules targeting the .previous-discussion
selector will match correctly.


<% 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 rendered twice when context_posts is present.

When context_posts.present? is true, %{respond_instructions} appears on both line 6 (inside the conditional) and line 19 (unconditional footer). The email recipient will see the same response instructions duplicated.

Either remove the respond_instructions div on line 6, or wrap the footer instance (line 19) in an unless context_posts.present? block.

Proposed fix: remove the duplicate inside the conditional 
   <% if context_posts.present? %>
-    <div class='footer'>%{respond_instructions}</div>
-
     <hr>

     <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
<% 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 template
renders %{respond_instructions} twice when context_posts.present? is true; fix
by removing the inner duplicate (the <div
class='footer'>%{respond_instructions}</div> inside the if block) or
alternatively make the bottom footer conditional with unless
context_posts.present? so %{respond_instructions} only appears once; update the
view where context_posts, respond_instructions, and unsubscribe_link are used to
ensure only a single respond_instructions footer is rendered.


<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+/}
get "t/:slug/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/}
get "t/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/}
get "t/:topic_id/summary" => "topics#show", constraints: {topic_id: /\d+/}
Comment on lines +439 to +442
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

Slugless summary route (line 442) is missing defaults: { summary: true }.

The slug-based summary route on line 439 sets defaults: { summary: true }, but the slugless variant on line 442 does not. This means visiting t/:topic_id/summary will invoke topics#show without params[:summary], so the summary behavior won't activate.

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+/}
📝 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
get "t/:slug/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/}
get "t/:slug/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/}
get "t/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/}
get "t/:topic_id/summary" => "topics#show", constraints: {topic_id: /\d+/}
get "t/:slug/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/}
get "t/:slug/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/}
get "t/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/}
get "t/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/}
🤖 Prompt for AI Agents 
In `@config/routes.rb` around lines 439 - 442, The slugless summary route "get
\"t/:topic_id/summary\" => \"topics#show\"" is missing defaults so
params[:summary] won't be set; update that route to include defaults: { summary:
true } (preserving the existing constraints: {topic_id: /\d+/}) so topics#show
receives the same summary flag as the slug-based route.

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