-
Notifications
You must be signed in to change notification settings - Fork 0
Automated Test: topic-email-management #325
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 |
|---|---|---|
| @@ -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; | ||
| } | ||
| } | ||
| }); | ||
| 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"] | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle missing
🛠️ 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 (Metrics/AbcSize) 🤖 Prompt for AI Agents |
||
|
|
||
| def wordpress | ||
| params.require(:best) | ||
| params.require(:topic_id) | ||
|
|
@@ -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 | ||
|
|
||
| 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid duplicate respond instructions and fix the header class. When ✅ 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 |
||
| <% 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> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 Convert these routes to POST, or require a signed token in the URL to authorize the state change. 🤖 Prompt for AI Agents |
||
| 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+/} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle both
idandtopic_idroute params.model(params)usesparams.idexclusively (Line 5-6). If the non-slug route passestopic_id, this will be undefined andloadTopicViewwill fail. Add a fallback toparams.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
🤖 Prompt for AI Agents