Skip to content

Automated Test: topic-email-management #325

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
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"),

})
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
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
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;
});
Comment on lines +4 to +9

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

},

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

actions: {
didTransition() {
this.controllerFor("application").set("showFooter", true);
return true;
}
}
});
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>
</div>
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
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
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!

perform_show_response
end
Comment on lines +98 to +116

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.


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

<% context_posts.each do |p| %>
<%= render partial: 'email/post', locals: { post: p } %>
<% end %>
Comment on lines +5 to +14

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

<% end %>
<% end %>

<hr>
<hr>

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

<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
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
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
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+/}
Comment on lines +440 to +441

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 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.rb

Repository: 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*' -C3

Repository: 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 -A2

Repository: 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.rb

Repository: 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 -A1

Repository: 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 unsubscribe

Repository: 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.

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
Loading