FEATURE: per-topic unsubscribe option in emails#9
Open
ShashankFC wants to merge 1 commit into
Open
Conversation
Author
|
@cubic-dev-ai review this pull request |
@ShashankFC I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 18 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="app/views/email/notification.html.erb">
<violation number="1" location="app/views/email/notification.html.erb:10">
P2: The HTML class attribute contains a leading dot: `class='.previous-discussion'`. The dot prefix is CSS selector syntax and should not be in the HTML `class` attribute. This means any CSS rule like `.previous-discussion { ... }` won't match this element (the browser sees the class name as literally `.previous-discussion` including the dot).</violation>
</file>
<file name="app/models/topic_user.rb">
<violation number="1" location="app/models/topic_user.rb:125">
P1: Bug (pre-existing, surfaced by this change): `user_id` is incorrectly assigned `topic` instead of `user` in the else branch. On line 124, `user_id = user.is_a?(User) ? user.id : topic` should be `user_id = user.is_a?(User) ? user.id : user`. When `user` is passed as a raw integer ID, `user_id` will be set to the `topic` argument instead, causing the query on this line to look up / create records with the wrong user.</violation>
</file>
<file name="app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6">
<violation number="1" location="app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6:1">
P2: `ObjectController` is deprecated (removed in Ember 2.0). New code should extend from `Controller` instead. Import from `discourse/controllers/controller`.</violation>
</file>
<file name="app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs">
<violation number="1" location="app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs:3">
P2: Typo in property name: `stopNotificiationsText` should be `stopNotificationsText` (extra 'i' in "Notificiations"). The controller (`topic-unsubscribe.js.es6`) has the same misspelling, so it works at runtime, but this should be fixed in both files for readability and maintainability.</violation>
</file>
<file name="app/assets/javascripts/discourse/routes/app-route-map.js.es6">
<violation number="1" location="app/assets/javascripts/discourse/routes/app-route-map.js.es6:13">
P1: Route `topicUnsubscribe` should be nested inside the `topic` resource rather than defined at the top level. The template at `templates/topic/unsubscribe.hbs` follows nested route naming (`topic.unsubscribe`), not top-level naming (`topicUnsubscribe`). Additionally, nesting it avoids potential route conflict with the `fromParamsNear` catch-all route at `/:nearPost`, and grants proper access to the parent topic model.</violation>
</file>
<file name="app/controllers/topics_controller.rb">
<violation number="1" location="app/controllers/topics_controller.rb:98">
P1: **Security/Design: State-modifying action on a GET route.** The `unsubscribe` action changes notification level and saves to the database, but it's mapped as `GET` in routes. This means:
1. CSRF protection is not enforced for GET in Rails, so any page can trigger this via `<img>` tags or link prefetching.
2. Browser prefetchers and crawlers can trigger unsubscribes just by visiting the URL.
This should be a two-step process: GET renders a confirmation page, POST/PUT performs the actual unsubscribe.</violation>
<violation number="2" location="app/controllers/topics_controller.rb:105">
P1: **Bug: `NoMethodError` when `TopicUser` record doesn't exist.** `find_by` returns `nil` if the user has no `TopicUser` record for this topic (e.g., they received a notification but never visited it). The subsequent `tu.notification_level` call will crash with `NoMethodError`. You need a nil guard or use `find_or_create_by`.</violation>
</file>
<file name="app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6">
<violation number="1" location="app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6:1">
P1: `Discourse.View` is not defined anywhere in this codebase. All other views in this directory use `Ember.View.extend()` (e.g., `application.js.es6`). This will throw a runtime error (`Cannot read properties of undefined`) when the module is loaded. Use `Ember.View.extend()` to match the existing convention.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| 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) |
There was a problem hiding this comment.
P1: Bug (pre-existing, surfaced by this change): user_id is incorrectly assigned topic instead of user in the else branch. On line 124, user_id = user.is_a?(User) ? user.id : topic should be user_id = user.is_a?(User) ? user.id : user. When user is passed as a raw integer ID, user_id will be set to the topic argument instead, causing the query on this line to look up / create records with the wrong user.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/models/topic_user.rb, line 125:
<comment>Bug (pre-existing, surfaced by this change): `user_id` is incorrectly assigned `topic` instead of `user` in the else branch. On line 124, `user_id = user.is_a?(User) ? user.id : topic` should be `user_id = user.is_a?(User) ? user.id : user`. When `user` is passed as a raw integer ID, `user_id` will be set to the `topic` argument instead, causing the query on this line to look up / create records with the wrong user.</comment>
<file context>
@@ -127,7 +122,7 @@ def track_visit!(topic,user)
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)
</file context>
| this.route('fromParamsNear', { path: '/:nearPost' }); | ||
| }); | ||
| this.resource('topicBySlug', { path: '/t/:slug' }); | ||
| this.route('topicUnsubscribe', { path: '/t/:slug/:id/unsubscribe' }); |
There was a problem hiding this comment.
P1: Route topicUnsubscribe should be nested inside the topic resource rather than defined at the top level. The template at templates/topic/unsubscribe.hbs follows nested route naming (topic.unsubscribe), not top-level naming (topicUnsubscribe). Additionally, nesting it avoids potential route conflict with the fromParamsNear catch-all route at /:nearPost, and grants proper access to the parent topic model.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/assets/javascripts/discourse/routes/app-route-map.js.es6, line 13:
<comment>Route `topicUnsubscribe` should be nested inside the `topic` resource rather than defined at the top level. The template at `templates/topic/unsubscribe.hbs` follows nested route naming (`topic.unsubscribe`), not top-level naming (`topicUnsubscribe`). Additionally, nesting it avoids potential route conflict with the `fromParamsNear` catch-all route at `/:nearPost`, and grants proper access to the parent topic model.</comment>
<file context>
@@ -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() {
</file context>
| raise ex | ||
| end | ||
|
|
||
| def unsubscribe |
There was a problem hiding this comment.
P1: Security/Design: State-modifying action on a GET route. The unsubscribe action changes notification level and saves to the database, but it's mapped as GET in routes. This means:
- CSRF protection is not enforced for GET in Rails, so any page can trigger this via
<img>tags or link prefetching. - Browser prefetchers and crawlers can trigger unsubscribes just by visiting the URL.
This should be a two-step process: GET renders a confirmation page, POST/PUT performs the actual unsubscribe.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/controllers/topics_controller.rb, line 98:
<comment>**Security/Design: State-modifying action on a GET route.** The `unsubscribe` action changes notification level and saves to the database, but it's mapped as `GET` in routes. This means:
1. CSRF protection is not enforced for GET in Rails, so any page can trigger this via `<img>` tags or link prefetching.
2. Browser prefetchers and crawlers can trigger unsubscribes just by visiting the URL.
This should be a two-step process: GET renders a confirmation page, POST/PUT performs the actual unsubscribe.</comment>
<file context>
@@ -94,6 +95,26 @@ def show
raise ex
end
+ def unsubscribe
+ @topic_view = TopicView.new(params[:topic_id], current_user)
+
</file context>
Comment on lines
+105
to
+110
| 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] |
There was a problem hiding this comment.
P1: Bug: NoMethodError when TopicUser record doesn't exist. find_by returns nil if the user has no TopicUser record for this topic (e.g., they received a notification but never visited it). The subsequent tu.notification_level call will crash with NoMethodError. You need a nil guard or use find_or_create_by.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/controllers/topics_controller.rb, line 105:
<comment>**Bug: `NoMethodError` when `TopicUser` record doesn't exist.** `find_by` returns `nil` if the user has no `TopicUser` record for this topic (e.g., they received a notification but never visited it). The subsequent `tu.notification_level` call will crash with `NoMethodError`. You need a nil guard or use `find_or_create_by`.</comment>
<file context>
@@ -94,6 +95,26 @@ def show
+ 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]
</file context>
Suggested change
| 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] | |
| tu = TopicUser.find_or_initialize_by(user_id: current_user.id, topic_id: params[:topic_id]) | |
| if tu.persisted? && tu.notification_level > TopicUser.notification_levels[:regular] | |
| tu.notification_level = TopicUser.notification_levels[:regular] | |
| else | |
| tu.notification_level = TopicUser.notification_levels[:muted] | |
| end |
| @@ -0,0 +1,3 @@ | |||
| export default Discourse.View.extend({ | |||
There was a problem hiding this comment.
P1: Discourse.View is not defined anywhere in this codebase. All other views in this directory use Ember.View.extend() (e.g., application.js.es6). This will throw a runtime error (Cannot read properties of undefined) when the module is loaded. Use Ember.View.extend() to match the existing convention.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6, line 1:
<comment>`Discourse.View` is not defined anywhere in this codebase. All other views in this directory use `Ember.View.extend()` (e.g., `application.js.es6`). This will throw a runtime error (`Cannot read properties of undefined`) when the module is loaded. Use `Ember.View.extend()` to match the existing convention.</comment>
<file context>
@@ -0,0 +1,3 @@
+export default Discourse.View.extend({
+ classNames: ["topic-unsubscribe"]
+});
</file context>
|
|
||
| <% context_posts.each do |p| %> | ||
| <%= render :partial => 'email/post', :locals => {:post => p} %> | ||
| <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4> |
There was a problem hiding this comment.
P2: The HTML class attribute contains a leading dot: class='.previous-discussion'. The dot prefix is CSS selector syntax and should not be in the HTML class attribute. This means any CSS rule like .previous-discussion { ... } won't match this element (the browser sees the class name as literally .previous-discussion including the dot).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/views/email/notification.html.erb, line 10:
<comment>The HTML class attribute contains a leading dot: `class='.previous-discussion'`. The dot prefix is CSS selector syntax and should not be in the HTML `class` attribute. This means any CSS rule like `.previous-discussion { ... }` won't match this element (the browser sees the class name as literally `.previous-discussion` including the dot).</comment>
<file context>
@@ -1,31 +1,29 @@
- <% 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| %>
</file context>
Suggested change
| <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4> | |
| <h4 class='previous-discussion'><%= t "user_notifications.previous_discussion" %></h4> |
| @@ -0,0 +1,9 @@ | |||
| import ObjectController from "discourse/controllers/object"; | |||
There was a problem hiding this comment.
P2: ObjectController is deprecated (removed in Ember 2.0). New code should extend from Controller instead. Import from discourse/controllers/controller.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6, line 1:
<comment>`ObjectController` is deprecated (removed in Ember 2.0). New code should extend from `Controller` instead. Import from `discourse/controllers/controller`.</comment>
<file context>
@@ -0,0 +1,9 @@
+import ObjectController from "discourse/controllers/object";
+
+export default ObjectController.extend({
</file context>
| @@ -0,0 +1,8 @@ | |||
| <div class="container"> | |||
| <p> | |||
| {{{stopNotificiationsText}}} | |||
There was a problem hiding this comment.
P2: Typo in property name: stopNotificiationsText should be stopNotificationsText (extra 'i' in "Notificiations"). The controller (topic-unsubscribe.js.es6) has the same misspelling, so it works at runtime, but this should be fixed in both files for readability and maintainability.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs, line 3:
<comment>Typo in property name: `stopNotificiationsText` should be `stopNotificationsText` (extra 'i' in "Notificiations"). The controller (`topic-unsubscribe.js.es6`) has the same misspelling, so it works at runtime, but this should be fixed in both files for readability and maintainability.</comment>
<file context>
@@ -0,0 +1,8 @@
+<div class="container">
+ <p>
+ {{{stopNotificiationsText}}}
+ </p>
+ <p>
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test 2
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#2