-
Notifications
You must be signed in to change notification settings - Fork 0
FEATURE: per-topic unsubscribe option in emails #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: email-notifications-enhancement
Are you sure you want to change the base?
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] | ||||||||||||||||
|
Comment on lines
+105
to
+107
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. Suggestion: When a logged-in user hits the unsubscribe URL for a topic they have no existing Severity Level: Major
|
||||||||||||||||
| tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id]) | |
| if tu.notification_level > TopicUser.notification_levels[:regular] | |
| tu = TopicUser.find_or_initialize_by(user_id: current_user.id, topic_id: params[:topic_id]) | |
| current_level = tu.notification_level || TopicUser.notification_levels[:regular] | |
| if current_level > TopicUser.notification_levels[:regular] |
Steps of Reproduction ✅
1. A notification email is sent which includes a per-topic unsubscribe link built from
`Topic#unsubscribe_url` at `app/models/topic.rb:20-22`, e.g. `/t/:slug/:id/unsubscribe`,
and referenced from the mailer at `app/mailers/user_notifications.rb:295` (via
`post.topic.unsubscribe_url` per Grep).
2. The user clicks that link, which routes to `TopicsController#unsubscribe` via the
routes defined in `config/routes.rb:440-441` (`get "t/:slug/:topic_id/unsubscribe" =>
"topics#unsubscribe"` and `get "t/:topic_id/unsubscribe" => "topics#unsubscribe"`).
3. After passing `ensure_logged_in` (configured in `TopicsController` at
`app/controllers/topics_controller.rb:8-28`), the request executes
`TopicsController#unsubscribe` at `app/controllers/topics_controller.rb:98-116`, where `tu
= TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id])` (line 105) is
called.
4. For any topic/user pair where no `topic_users` row exists (which is allowed and
expected, as shown by `TopicUser.get` returning `find_by` in
`app/models/topic_user.rb:68-72` and `TopicUser.change` explicitly handling the no-row
case at `app/models/topic_user.rb:74-107`), `tu` is `nil`, so `tu.notification_level` at
`app/controllers/topics_controller.rb:107` raises `NoMethodError: undefined method
'notification_level' for nil:NilClass`, returning HTTP 500 instead of the unsubscribe
view.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** app/controllers/topics_controller.rb
**Line:** 105:107
**Comment:**
*Null Pointer: When a logged-in user hits the unsubscribe URL for a topic they have no existing `TopicUser` record for, `find_by` will return `nil` and the subsequent access to `tu.notification_level` will raise a `NoMethodError`; initializing a record or defaulting the level avoids this nil dereference.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| 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 %> | ||
| <% 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> |
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.
Suggestion: The template uses triple-stash (
{{{}}}) when renderingstopNotificiationsText, which disables HTML escaping; because this text is built from a translation with an interpolated topic title (user-controlled content), any HTML in the title would be rendered unescaped and could lead to XSS. Use normal Handlebars interpolation here so the output is properly escaped while still showing the expected text. [security]Severity Level: Critical 🚨
Steps of Reproduction ✅
Prompt for AI Agent 🤖