FEATURE: per-topic unsubscribe option in emails#9
Conversation
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6 (2)
5-7: Consider modern computed property syntax.The
.property()syntax is from older Ember versions. Modern Ember usesEmber.computed()or decorators for better readability and maintainability.Example using
Ember.computed():stopNotificationsText: Ember.computed("model.fancyTitle", function() { return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") }); })
1-1: Replace ObjectController with modern Ember Controller pattern.Ember.ObjectController was deprecated in Ember 1.11 and removed in Ember 2.0. The codebase uses Ember 1.11.3.1, where this pattern still works but generates deprecation warnings. However, 19+ files in the codebase import from
discourse/controllers/object, making this a systematic modernization task.For
topic-unsubscribe.js.es6, replace the ObjectController import and migrate the computed property to modern Ember syntax:
- Remove
import ObjectController from "discourse/controllers/object"- Use regular
Controllerand access the model explicitly viamodel.propertyName- Convert
.property()computed properties to@computedor native getters
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/assets/javascripts/discourse/components/dropdown-button.js.es6(1 hunks)app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6(1 hunks)app/assets/javascripts/discourse/routes/app-route-map.js.es6(1 hunks)app/assets/javascripts/discourse/routes/topic-from-params.js.es6(2 hunks)app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6(1 hunks)app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs(1 hunks)app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6(1 hunks)app/assets/stylesheets/common/base/topic.scss(1 hunks)app/controllers/topics_controller.rb(3 hunks)app/mailers/user_notifications.rb(2 hunks)app/models/topic.rb(1 hunks)app/models/topic_user.rb(6 hunks)app/views/email/notification.html.erb(1 hunks)config/locales/client.en.yml(1 hunks)config/locales/server.en.yml(1 hunks)config/routes.rb(1 hunks)lib/email/message_builder.rb(2 hunks)spec/components/email/message_builder_spec.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
app/controllers/topics_controller.rb (2)
app/models/topic.rb (1)
unsubscribe_url(719-721)app/models/topic_user.rb (1)
notification_levels(19-21)
app/models/topic.rb (4)
app/models/post.rb (2)
url(342-348)url(350-352)app/models/category.rb (1)
url(392-404)app/serializers/similar_topic_serializer.rb (1)
url(14-16)app/models/notification.rb (1)
url(90-94)
lib/email/message_builder.rb (3)
lib/validators/post_validator.rb (1)
presence(19-29)app/models/topic.rb (1)
title(691-695)lib/discourse.rb (1)
base_url(211-213)
app/mailers/user_notifications.rb (2)
app/models/notification.rb (1)
post(96-100)app/models/topic.rb (1)
unsubscribe_url(719-721)
🪛 RuboCop (1.81.7)
app/controllers/topics_controller.rb
[convention] 28-28: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
[convention] 98-116: Assignment Branch Condition size for unsubscribe is too high. [<4, 31, 6> 31.83/23]
(Metrics/AbcSize)
lib/email/message_builder.rb
[warning] 62-62: Use == if you meant to do a comparison or wrap the expression in parentheses to indicate you meant to assign in a condition.
(Lint/AssignmentInCondition)
🔇 Additional comments (27)
app/mailers/user_notifications.rb (2)
310-310: LGTM! Clean refactor to single-line conditional.The refactoring of the
from_aliasassignment from a multi-line if/end block to a single-line guard clause improves readability without changing behavior.
295-295: Verify nil safety forpost.topicon lines 283, 295, and 300.The code accesses
post.topicwithout nil checks on multiple lines:
- Line 283:
template << "_pm" if post.topic.private_message?- Line 295:
unsubscribe_url: post.topic.unsubscribe_url,- Line 300:
private_reply: post.topic.private_message?,While posts typically have topics, the codebase demonstrates nil-guarding for
post.topicelsewhere (e.g.,app/models/topic_link.rb:169,app/models/post_action.rb:200). Verify thatpost.topicis guaranteed to be present at this point in the notification flow, or add defensive checks (e.g.,post.topic&.unsubscribe_url).app/models/topic.rb (1)
719-721: LGTM! Clean implementation following existing patterns.The
unsubscribe_urlmethod follows the same pattern as other URL methods in the Topic model (e.g.,last_post_url,relative_url). The implementation correctly appends "/unsubscribe" to the topic's URL.app/assets/javascripts/discourse/components/dropdown-button.js.es6 (1)
27-30: LGTM! Good defensive programming to prevent empty title elements.The conditional rendering of the title prevents outputting an empty or undefined
<h4 class='title'>element when no title is provided. This is a clean improvement that keeps the HTML output tidy.lib/email/message_builder.rb (3)
24-28: LGTM! Formatting improvement with trailing comma.The reformatting of the
template_argshash initialization improves readability and follows Ruby best practices by adding a trailing comma, which makes future diffs cleaner when adding new keys.
34-38: LGTM! Clean refactor to ternary operator.The refactoring from a multi-line if/else block to a ternary operator improves conciseness while maintaining the same behavior. The conditional logic is clear and readable.
59-69: Static analysis warning is a false positive.The RuboCop warning about assignment in condition at line 62 can be safely ignored. The pattern
unless html_override = @opts[:html_override]is idiomatic Ruby that combines assignment with a guard clause. This is intentional and widely used in Ruby codebases. The code correctly returnsnilwhen@opts[:html_override]is falsy, which is the expected behavior.spec/components/email/message_builder_spec.rb (1)
169-172: LGTM! Test properly updated for new unsubscribe_url parameter.The test correctly adds the
unsubscribe_urlparameter to align with the updatedEmail::MessageBuilderinitialization. The URL format matches the expected pattern from the Topic model'sunsubscribe_urlmethod.config/locales/server.en.yml (1)
1852-1855: LGTM! Clear messaging for both global and topic-specific unsubscribe options.The locale update correctly adds topic-specific unsubscribe functionality while preserving the existing global unsubscribe option. The YAML block scalar format is appropriate for multi-line content, and the message clearly distinguishes between the two unsubscribe options available to users.
app/models/topic_user.rb (1)
8-13: LGTM! Formatting improvements throughout the file.The changes consist entirely of formatting and style improvements:
- Improved alignment in the
trackingscope's where clause- Simplified the
create_lookupiteration to a single-line block- Consistent formatting of
MessageBus.publishcalls- Better readability in
update_allstatementsNo behavioral changes detected. These improvements enhance code consistency and maintainability.
Also applies to: 64-64, 113-113, 125-125, 194-194, 218-218
app/views/email/notification.html.erb (4)
3-3: LGTM! Updated to modern Ruby syntax.The partial render call has been updated from hash-rocket style to modern Ruby 1.9+ syntax (
post: post), which is the current convention.
5-15: LGTM! Improved content organization in context posts block.The reorganization improves the email layout by:
- Placing respond instructions in a footer before the context section
- Adding clear visual separation with
<hr>and a heading- Updating partial render syntax to modern Ruby style
The structure provides better visual hierarchy in the email.
17-20: LGTM! Clear footer structure with both instruction types.The consolidated footer section properly separates:
- Respond instructions (how to reply)
- Unsubscribe link (how to opt out)
Both are wrapped in separate footer divs, which allows for consistent styling and clear visual separation from the main content.
24-28: LGTM! Improved indentation in schema.org metadata block.The indentation changes improve readability of the schema.org EmailMessage metadata block without altering its functionality.
app/assets/javascripts/discourse/routes/topic-from-params.js.es6 (2)
5-16: LGTM: ES6 modernization improves code quality.The conversion to ES6 method shorthand and
constdeclarations improves readability and scoping. These changes align with modern JavaScript best practices.
58-60: Good addition: Error handling for promise rejection.The
.catch()handler provides graceful error handling for topic loading failures, improving robustness.app/assets/javascripts/discourse/routes/app-route-map.js.es6 (1)
13-13: LGTM: Route declaration follows established conventions.The new
topicUnsubscriberoute is properly declared with appropriate path parameters, consistent with existing topic routes.app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6 (1)
1-3: LGTM: Simple view implementation.The view correctly extends
Discourse.Viewand applies the appropriate CSS class for styling.app/assets/stylesheets/common/base/topic.scss (1)
67-77: LGTM: Styling supports inline notification controls.The CSS properly positions the notification dropdown within the unsubscribe page by removing floats and adjusting layout properties.
config/routes.rb (1)
438-442: LGTM: Routes properly constrained and organized.The unsubscribe routes are correctly added with appropriate constraints (
topic_id: /\d+/) and support both slug-based and ID-only paths, consistent with other topic routes.app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6 (3)
12-15: LGTM: Appropriately clears notification reason.Clearing the
notificationReasonTexton the unsubscribe page makes sense, as the user is explicitly managing their notification preferences.
17-22: LGTM: Footer visibility handled correctly.Setting
showFootertotrueensures the application footer displays on the unsubscribe page, providing consistent navigation.
4-10: The suggested error handling approach has a critical issue: the "exception-unknown" route doesn't exist.The review comment proposes adding
.catch(error => this.transitionTo("exception-unknown")), but onlyexception.js.es6exists in the codebase. More importantly, the application route already defines a globalerror(err, transition)action that handles promise rejections from the model hook:
- 404 errors transition to
'unknown'- Other errors transition to
'exception'This global handler catches rejected promises from route models automatically. The lack of explicit error handling in
topic-unsubscribe.js.es6follows the pattern used throughout the codebase—most routes rely on this centralized error handling rather than adding.catch()in individual model hooks.If you want to add route-specific error handling, use the pattern from
preferences-about.js.es6(which uses.then(success, failure)with a user alert), but the current design already prevents blank error screens.Likely an incorrect or invalid review comment.
app/controllers/topics_controller.rb (4)
27-28: LGTM!The addition of
:unsubscribeto theensure_logged_infilter correctly ensures users must be authenticated to access the unsubscribe action.
32-32: LGTM!Skipping the XHR check for the unsubscribe action is appropriate, allowing users to access the unsubscribe page directly via URL (similar to
showandfeed).
107-111: Verify toggle behavior for unsubscribe action.The current logic toggles between notification levels rather than simply unsubscribing:
- If tracking or watching → downgrade to regular
- If regular or muted → mute
This differs from typical "unsubscribe" behavior, which would unconditionally mute notifications. Please confirm this toggle behavior is intentional for the unsubscribe feature.
500-500: LGTM!The explicit
render :showimproves code clarity by making the response behavior explicit for HTML format requests.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis change introduces a topic unsubscribe feature across the frontend and backend. It adds a new route, controller, and view for unsubscribe pages; updates the backend controller to handle unsubscribe requests and manage notification levels; adds unsubscribe URLs to email notifications; includes localization strings; and modernizes some frontend code to ES6 syntax. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Router as Ember Router
participant Controller as Topic-Unsubscribe Controller
participant Route as Topic-Unsubscribe Route
participant API as Backend API
participant TopicsCtrl as Topics Controller
participant Model as Topic Model
User->>Browser: Navigate to /t/topic-slug/123/unsubscribe
Browser->>Router: Route transition
Router->>Route: Enter topicUnsubscribe route
Route->>API: GET /t/topic-slug/123 (load topic view)
API->>TopicsCtrl: unsubscribe action
TopicsCtrl->>Model: Get/create topic with id
TopicsCtrl->>Model: Update from fetched JSON
TopicsCtrl->>API: Update TopicUser notification_level
API-->>TopicsCtrl: Success
TopicsCtrl-->>API: Render show response
API-->>Route: Topic JSON + HTML response
Route->>Model: Create topic record
Route->>Controller: setupController with model
Controller->>Controller: Compute stopNotificiationsText
Controller-->>Browser: Render unsubscribe template
Browser->>User: Display unsubscribe confirmation page
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/assets/javascripts/discourse/routes/topic-from-params.js.es6 (1)
32-35: Remove the unusedenteredAtproperty or document its intended use.Verification confirms that
enteredAt: new Date().getTime().toString()is set ontopicControllerin app/assets/javascripts/discourse/lib/url.js:230 but is never read or consumed anywhere in the codebase. This unused property should either be removed or accompanied by documentation and tests explaining its purpose.app/models/topic_user.rb (1)
120-131: Bug intrack_visit!usestopicinstead ofuserwhen derivinguser_id
user_id = user.is_a?(User) ? user.id : topicwill assign the topic identifier asuser_idwheneveruseris not aUserinstance (e.g., when an ID is passed), corrupting the(topic_id, user_id)pair:def track_visit!(topic,user) topic_id = topic.is_a?(Topic) ? topic.id : topic user_id = user.is_a?(User) ? user.id : topic # <- should use `user`This predates the formatting change but is a correctness issue worth fixing now.
Suggested fix:
def track_visit!(topic,user) topic_id = topic.is_a?(Topic) ? topic.id : topic - user_id = user.is_a?(User) ? user.id : topic + user_id = user.is_a?(User) ? user.id : user
🧹 Nitpick comments (5)
app/assets/javascripts/discourse/routes/topic-from-params.js.es6 (1)
7-16: Complete the ES6 modernization by using arrow functions.The ES6 method shorthand and
constdeclarations are good improvements. However, theconst self = thisbinding on line 11 is unnecessary if you use arrow functions for the callbacks on lines 23 and 58.Apply this diff to eliminate the
selfbinding and complete the ES6 modernization:- const self = this, - topic = this.modelFor('topic'), + const 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; } params.forceLoad = true; - postStream.refresh(params).then(function () { + postStream.refresh(params).then(() => { // 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 const closestPost = postStream.closestPostForPostNumber(params.nearPost || 1), closest = closestPost.get('post_number'), progress = postStream.progressIndexOfPost(closestPost); topicController.setProperties({ 'model.currentPost': closest, enteredAt: new Date().getTime().toString(), }); topicProgressController.setProperties({ progressPosition: progress, expanded: false }); // Highlight our post after the next render - Ember.run.scheduleOnce('afterRender', function() { - self.appEvents.trigger('post:highlight', closest); + Ember.run.scheduleOnce('afterRender', () => { + this.appEvents.trigger('post:highlight', closest); }); Discourse.URL.jumpToPost(closest); if (topic.present('draft')) { composerController.open({ draft: Discourse.Draft.getLocal(topic.get('draft_key'), topic.get('draft')), draftKey: topic.get('draft_key'), draftSequence: topic.get('draft_sequence'), topic: topic, ignoreIfChanged: true }); } - }).catch(function(e) { + }).catch((e) => { Ember.warn('Could not view topic', e); });app/models/topic_user.rb (1)
112-114: MessageBus publish calls are consistent; consider DRYing topic channelAll three
MessageBus.publish("/topic/#{topic_id}", { notification_level_change: ... }, user_ids: [...])calls are consistent and correct. If you touch this again, factoring out the channel string or a tiny helper would reduce duplication, but it’s not urgent.Also applies to: 193-195, 218-219
lib/email/message_builder.rb (1)
61-64: Clean up assignment in condition inhtml_partto satisfy RuboCop
if response_instructions = @template_args[:respond_instructions]is valid Ruby but triggersLint/AssignmentInCondition. To keep the intent clear and silence the warning, you can split it:- if response_instructions = @template_args[:respond_instructions] - respond_instructions = PrettyText.cook(response_instructions).html_safe - html_override.gsub!("%{respond_instructions}", respond_instructions) - end + response_instructions = @template_args[:respond_instructions] + if response_instructions + cooked = PrettyText.cook(response_instructions).html_safe + html_override.gsub!("%{respond_instructions}", cooked) + endapp/controllers/topics_controller.rb (1)
27-28: Consider adding a trailing comma.For consistency with Ruby style conventions, consider adding a trailing comma after
:unsubscribeon line 28. This makes future additions cleaner and reduces diff noise.Based on learnings: This aligns with RuboCop's TrailingCommaInArrayLiteral convention.
Apply this diff:
:bookmark, - :unsubscribe] + :unsubscribe,]app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6 (1)
1-3: Consider migrating from deprecated ObjectController.
ObjectControllerwas deprecated in Ember 1.11. Consider using a standardControllerand accessing model properties directly (e.g.,this.get('model.fancyTitle')instead of relying on ObjectController's proxying behavior).If this Discourse version still actively uses ObjectController elsewhere, this refactor can be deferred. However, verify the Ember version in use to confirm compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/assets/javascripts/discourse/components/dropdown-button.js.es6(1 hunks)app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6(1 hunks)app/assets/javascripts/discourse/routes/app-route-map.js.es6(1 hunks)app/assets/javascripts/discourse/routes/topic-from-params.js.es6(2 hunks)app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6(1 hunks)app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs(1 hunks)app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6(1 hunks)app/assets/stylesheets/common/base/topic.scss(1 hunks)app/controllers/topics_controller.rb(3 hunks)app/mailers/user_notifications.rb(2 hunks)app/models/topic.rb(1 hunks)app/models/topic_user.rb(6 hunks)app/views/email/notification.html.erb(1 hunks)config/locales/client.en.yml(1 hunks)config/locales/server.en.yml(1 hunks)config/routes.rb(1 hunks)lib/email/message_builder.rb(2 hunks)spec/components/email/message_builder_spec.rb(1 hunks)
🧰 Additional context used
🪛 RuboCop (1.81.7)
app/controllers/topics_controller.rb
[convention] 28-28: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
[convention] 98-116: Assignment Branch Condition size for unsubscribe is too high. [<4, 31, 6> 31.83/23]
(Metrics/AbcSize)
lib/email/message_builder.rb
[warning] 62-62: Use == if you meant to do a comparison or wrap the expression in parentheses to indicate you meant to assign in a condition.
(Lint/AssignmentInCondition)
🔇 Additional comments (18)
app/assets/javascripts/discourse/routes/topic-from-params.js.es6 (2)
5-5: LGTM: Clean ES6 method shorthand conversion.The conversion to ES6 method shorthand is correct and improves code readability.
28-30: LGTM: Good use of const for immutable bindings.Converting these declarations from
vartoconstis a good practice that prevents accidental reassignment.app/assets/javascripts/discourse/components/dropdown-button.js.es6 (1)
26-30: Guarded title rendering inrenderStringlooks goodUsing a local
titleand only emitting the<h4>when truthy avoids empty headings and matches the existing raw-HTML rendering style in this component. Assumingtitleremains trusted/escaped upstream, this is fine.app/models/topic_user.rb (2)
8-13:trackingscope chaining is clearer and behavior-preservingThe split
wherecalls with named params keep the SQL condition readable without changing semantics.
60-66: Lookup creation loop style change is fineSwitching to a single-line block for
topic_users.eachis purely stylistic; behavior is unchanged and still straightforward.config/locales/client.en.yml (1)
983-987: Newjs.topic.unsubscribestrings look correctThe added
stop_notificationsandchange_notification_statekeys use the expected{{title}}interpolation style and HTML markup pattern used elsewhere in this file, so they should plug into the unsubscribe UI without issues.app/models/topic.rb (1)
719-721:unsubscribe_urlhelper is simple and correctly based on canonical topic URLUsing
url(theDiscourse.base_url-based canonical topic URL) and appending/unsubscribegives you a stable per-topic unsubscribe endpoint ideal for emails and links. Callers should prefer this helper over hand-assembling the path.config/locales/server.en.yml (1)
1852-1855: LGTM! Unsubscribe messaging is clear and well-structured.The multi-line unsubscribe block provides users with both general unsubscribe options and topic-specific unsubscribe functionality. The YAML syntax and interpolation variables are correctly formatted.
app/assets/stylesheets/common/base/topic.scss (1)
67-77: LGTM! Clean styling for the unsubscribe UI.The layout adjustments for
.topic-unsubscribelook good. The use ofinitialfor the bottom property has broad browser support and should work fine in this context.config/routes.rb (1)
438-442: LGTM! Routes are well-structured and consistent.The new unsubscribe routes follow existing patterns in the codebase, providing both slug-based and numeric topic ID variants with proper constraints. The routing setup aligns well with the new topic unsubscribe feature.
app/mailers/user_notifications.rb (2)
295-295: Good addition of topic-specific unsubscribe URL.Adding
unsubscribe_urlto email options enables per-topic unsubscribe functionality in notification emails. This integrates cleanly with the new Topic#unsubscribe_url method.
310-310: Cleaner conditional assignment.The refactor from multi-line to single-line conditional is more idiomatic Ruby and improves readability.
app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6 (1)
1-3: LGTM! Simple and correct view definition.The view properly extends Discourse.View and sets the class name that matches the CSS styling in
topic.scss. This follows the standard Ember/Discourse pattern.app/assets/javascripts/discourse/routes/app-route-map.js.es6 (1)
13-13: LGTM! Route properly registered.The
topicUnsubscriberoute is correctly defined with a path that matches the server-side routes inconfig/routes.rb. Good placement alongside other topic-related routes.spec/components/email/message_builder_spec.rb (1)
171-172: LGTM! Test properly updated for new functionality.The test now passes
unsubscribe_urlparameter alongsideadd_unsubscribe_link: true, correctly verifying the new per-topic unsubscribe feature. The URL format matches the expected route pattern.app/views/email/notification.html.erb (1)
6-6: Verify intentional duplication of respond_instructions.The
%{respond_instructions}placeholder appears twice: once inside thecontext_postsblock (line 6) and once in the footer (line 19). This means when context posts are present, the respond instructions will be displayed twice in the email. Please confirm this duplication is intentional.Also applies to: 19-19
app/controllers/topics_controller.rb (1)
493-507: LGTM: Explicit render improves clarity.The explicit
render :showon line 500 clarifies the template being rendered for HTML responses, which is especially helpful now thatperform_show_responseis called from bothshowandunsubscribeactions.app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6 (1)
12-15: LGTM: Notification reason cleared appropriately.Clearing the
notificationReasonTextinafterModelis appropriate for the unsubscribe flow, as the user is explicitly managing their subscription and doesn't need to see why they were previously notified.
| stopNotificiationsText: function() { | ||
| return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") }); | ||
| }.property("model.fancyTitle"), |
There was a problem hiding this comment.
Fix typo in method name.
The method name stopNotificiationsText has a typo: "Notificiations" should be "Notifications". This will cause the template to fail when trying to access this property.
Apply this diff to fix the typo:
- stopNotificiationsText: function() {
+ stopNotificationsText: function() {
return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") });
}.property("model.fancyTitle"),📝 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.
| stopNotificiationsText: function() { | |
| return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") }); | |
| }.property("model.fancyTitle"), | |
| stopNotificationsText: function() { | |
| return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") }); | |
| }.property("model.fancyTitle"), |
🤖 Prompt for AI Agents
In app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6 around
lines 5 to 7, the method name stopNotificiationsText contains a typo; rename it
to stopNotificationsText (fix the spelling) while preserving its body and the
.property("model.fancyTitle") dependency so templates can access
model.fancyTitle via stopNotificationsText; also search and update any template
or JS references that used the old misspelled name to the corrected
stopNotificationsText.
| model(params) { | ||
| const topic = this.store.createRecord("topic", { id: params.id }); | ||
| return PostStream.loadTopicView(params.id).then(json => { | ||
| topic.updateFromJson(json); | ||
| return topic; | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Add error handling for topic loading.
The PostStream.loadTopicView promise has no error handling. If the topic doesn't exist or the user lacks access, the promise rejection will bubble up unhandled, potentially leaving the user on a broken page.
Apply this diff to add error handling:
model(params) {
const topic = this.store.createRecord("topic", { id: params.id });
- return PostStream.loadTopicView(params.id).then(json => {
- topic.updateFromJson(json);
- return topic;
- });
+ return PostStream.loadTopicView(params.id)
+ .then(json => {
+ topic.updateFromJson(json);
+ return topic;
+ })
+ .catch(() => {
+ this.transitionTo("discovery.latest");
+ });
},📝 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.
| 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 topic = this.store.createRecord("topic", { id: params.id }); | |
| return PostStream.loadTopicView(params.id) | |
| .then(json => { | |
| topic.updateFromJson(json); | |
| return topic; | |
| }) | |
| .catch(() => { | |
| this.transitionTo("discovery.latest"); | |
| }); | |
| }, |
🤖 Prompt for AI Agents
In app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6 around lines
4–10, PostStream.loadTopicView is missing error handling; wrap the promise with
a .catch handler that handles load failures by logging the error, rolling
back/deleting the created topic record (topic.deleteRecord() or
topic.rollbackAttributes()), and transitioning away (e.g.
this.transitionTo('not-found') or another appropriate route) so the user is not
left on a broken page; ensure the handler returns a value or transition to stop
the route from rendering a broken model.
| @@ -0,0 +1,8 @@ | |||
| <div class="container"> | |||
| <p> | |||
| {{{stopNotificiationsText}}} | |||
There was a problem hiding this comment.
🧩 Analysis chain
Typo in variable name: "stopNotificiationsText".
The variable is misspelled as stopNotificiationsText (missing 't' in "Notifications"). This should be stopNotificationsText for consistency and readability. Please verify this typo is consistently used in the controller, or correct it throughout.
Additionally, the triple braces {{{ }}} render unescaped HTML. Ensure the stopNotificationsText value is properly sanitized to prevent XSS vulnerabilities.
Apply this diff if the typo needs correction:
- {{{stopNotificiationsText}}}
+ {{{stopNotificationsText}}}🏁 Script executed:
#!/bin/bash
# Verify the exact content of the template file
cat app/assets/javascripts/discourse/templates/topic/unsubscribe.hbsLength of output: 191
🏁 Script executed:
#!/bin/bash
# Search for stopNotifications variable in controller and helper files
rg -n "stopNotif" --type=ts --type=js -C3 app/Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for this specific template being rendered or referenced
rg -n "unsubscribe" --type=ts --type=js app/ | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for stopNotifications (correct spelling) anywhere in codebase
rg -n "stopNotifications" --type-list | head -5
rg -n "stopNotifications" -C3Length of output: 389
🏁 Script executed:
#!/bin/bash
# Search for stopNotificiations (exact typo) everywhere
rg -n "stopNotificiations" -C3Length of output: 1303
🏁 Script executed:
#!/bin/bash
# Find controller/route files related to unsubscribe
fd -e "js" -e "ts" -e "rb" | xargs grep -l "unsubscribe" 2>/dev/null | head -20Length of output: 557
🏁 Script executed:
#!/bin/bash
# Look for Ember controller files
fd -name "*controller*" -o -name "*route*" | head -20Length of output: 230
Typo confirmed in variable name across multiple files: "stopNotificiationsText" should be "stopNotificationsText".
The misspelling exists consistently in both the template and controller, so the code functionally works. However, this spelling error should be corrected for maintainability:
app/assets/javascripts/discourse/templates/topic/unsubscribe.hbsline 3app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6line 5
The triple braces in the template render unescaped HTML; verify the controller's computed property sanitizes the content appropriately.
🤖 Prompt for AI Agents
In app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs around line
3 and in app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6
around line 5, rename the misspelled property stopNotificiationsText to
stopNotificationsText in both the template and the controller so names match;
update all references in these files (and any other files importing/using it) to
the corrected spelling, and in the controller ensure the computed property
returns sanitized/HTML-safe content before the template's triple-brace render
(or change to double braces if HTML is not needed).
| 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! |
There was a problem hiding this comment.
Handle nil TopicUser record.
The code calls TopicUser.find_by which can return nil if no record exists for the user-topic pair. The subsequent lines access tu.notification_level and call tu.save! without checking for nil, which will raise a NoMethodError if the record doesn't exist.
Apply this diff to handle the nil case:
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!
+ if tu
+ 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!
+ else
+ TopicUser.create!(
+ user_id: current_user.id,
+ topic_id: params[:topic_id],
+ notification_level: TopicUser.notification_levels[:muted]
+ )
+ end📝 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.
| 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 | |
| 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! | |
| else | |
| TopicUser.create!( | |
| user_id: current_user.id, | |
| topic_id: params[:topic_id], | |
| notification_level: TopicUser.notification_levels[:muted] | |
| ) | |
| end |
🤖 Prompt for AI Agents
In app/controllers/topics_controller.rb around lines 105 to 113, the code
assumes TopicUser.find_by always returns a record; first check if tu is nil and
handle it (e.g., render head :not_found or render json error with appropriate
status and return) before accessing tu.notification_level or calling tu.save!;
if you prefer to create a default TopicUser instead, instantiate one with the
appropriate attributes and initial notification_level, then proceed to update
and save—either way ensure the nil branch returns or creates so no NoMethodError
occurs.
|
|
||
| <% 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.
Remove the leading dot from the class name.
The class attribute contains .previous-discussion with a leading dot, which is invalid HTML syntax. The dot is CSS selector syntax and should not appear in the HTML class attribute.
Apply this diff to fix the class name:
- <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 around line 10, the class attribute
value incorrectly includes a leading dot ('.previous-discussion'); remove the
leading dot so the class is "previous-discussion" to produce valid HTML, i.e.,
replace ".previous-discussion" with "previous-discussion" in that h4 tag.
| @template_args = { | ||
| site_name: SiteSetting.email_prefix.presence || SiteSetting.title, | ||
| base_url: Discourse.base_url, | ||
| user_preferences_url: "#{Discourse.base_url}/my/preferences", | ||
| }.merge!(@opts) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify per-topic unsubscribe_url is actually wired through template and headers
You’re now:
- Building
@template_argswithsite_name,base_url, anduser_preferences_url. - When
add_unsubscribe_linkis set, injecting anunsubscribe_linksnippet viaI18n.t('unsubscribe_link', template_args)into both HTML and text bodies. - Still setting the
List-Unsubscribeheader to<#{template_args[:user_preferences_url]}>.
Given the new Topic#unsubscribe_url and mailer changes mentioned in the PR description, double‑check:
- That
:unsubscribe_urlis included in@optsand propagated into@template_args(e.g., via the merge), and - That either the
unsubscribe_linkI18n string or theList-Unsubscribeheader (or both) actually use that per‑topic URL rather than always pointing at the global preferences page.
If the intent is truly per‑topic unsubscribe, you probably want something along the lines of:
- @template_args = {
- site_name: ...,
- base_url: ...,
- user_preferences_url: "#{Discourse.base_url}/my/preferences",
- }.merge!(@opts)
+ @template_args = {
+ site_name: ...,
+ base_url: Discourse.base_url,
+ user_preferences_url: "#{Discourse.base_url}/my/preferences",
+ }.merge!(@opts)
+
+ unsubscribe_url = @template_args[:unsubscribe_url] || @template_args[:user_preferences_url]and then in header_args:
- result['List-Unsubscribe'] = "<#{template_args[:user_preferences_url]}>" if @opts[:add_unsubscribe_link]
+ if @opts[:add_unsubscribe_link]
+ url = template_args[:unsubscribe_url] || template_args[:user_preferences_url]
+ result['List-Unsubscribe'] = "<#{url}>"
+ endPlease also ensure the unsubscribe_link translation only references interpolation keys that exist in template_args, otherwise I18n.t will raise at runtime.
Also applies to: 61-69, 104-108
🏁 Script executed:
#!/bin/bash
# Examine the full message_builder.rb file to understand the structure
cat lib/email/message_builder.rbLength of output: 5929
🏁 Script executed:
#!/bin/bash
# Search for unsubscribe_url assignments or usage across the codebase
rg -n "unsubscribe_url" --type=rbLength of output: 26
🏁 Script executed:
#!/bin/bash
# Search for List-Unsubscribe header setup
rg -n "List-Unsubscribe" --type=rbLength of output: 26
🏁 Script executed:
#!/bin/bash
# Find I18n translations related to unsubscribe_link
fd -e yml -e yaml | xargs rg -l "unsubscribe_link" 2>/dev/null | head -20Length of output: 597
🏁 Script executed:
#!/bin/bash
# Find the English locale file with unsubscribe_link definition
grep -n "unsubscribe_link" config/locales/server.en.yml | head -20Length of output: 163
🏁 Script executed:
#!/bin/bash
# Search for unsubscribe_url usage (without type restriction)
rg -n "unsubscribe_url" --max-count=50Length of output: 952
🏁 Script executed:
#!/bin/bash
# Search for add_unsubscribe_link usage in mailers and related code
rg -n "add_unsubscribe_link" --max-count=50Length of output: 980
🏁 Script executed:
#!/bin/bash
# Look for MessageBuilder initialization with opts to see what gets passed
rg -n "MessageBuilder.new" -C 2 --max-count=30Length of output: 5304
🏁 Script executed:
#!/bin/bash
# Get full context around user_notifications.rb line 295 to see the complete opts being passed
sed -n '290,310p' app/mailers/user_notifications.rbLength of output: 786
🏁 Script executed:
#!/bin/bash
# Get the full unsubscribe_link translation to see exactly what it uses
sed -n '1852,1857p' config/locales/server.en.ymlLength of output: 237
🏁 Script executed:
#!/bin/bash
# Look at the test for unsubscribe to see what's expected
grep -A 10 "with add_unsubscribe_link true" spec/components/email/message_builder_spec.rbLength of output: 552
Fix List-Unsubscribe header to use per-topic unsubscribe_url and remove redundant condition
The review comment is correct. Your code partially implements per-topic unsubscribe:
- The
unsubscribe_urlfrompost.topic.unsubscribe_urlis passed in opts at line 295 - Lines 24-28 merge this into
@template_argsvia.merge!(@opts) - The
unsubscribe_linktranslation uses both%{user_preferences_url}and%{unsubscribe_url}, both of which are available intemplate_args
However, lines 106-107 have two issues:
-
List-Unsubscribe header ignores the per-topic URL: It hardcodes
user_preferences_urlinstead of checking forunsubscribe_urlfirst, making it inconsistent with the email body which includes the topic-specific unsubscribe link. -
Redundant if statement: The outer
if @opts[:add_unsubscribe_link]at line 106 makes the inner condition at line 107 redundant.
Update lines 106-108 to:
if @opts[:add_unsubscribe_link]
url = @template_args[:unsubscribe_url] || @template_args[:user_preferences_url]
result['List-Unsubscribe'] = "<#{url}>"
endThis ensures the RFC 8058 header uses the per-topic URL when available, matching the email body behavior.
🤖 Prompt for AI Agents
In lib/email/message_builder.rb around lines 106 to 108, the List-Unsubscribe
header currently ignores the per-topic unsubscribe_url and has a redundant
conditional; replace the block so that when @opts[:add_unsubscribe_link] is true
it sets url = @template_args[:unsubscribe_url] ||
@template_args[:user_preferences_url] and then assigns
result['List-Unsubscribe'] = "<#{url}>", thereby preferring the per-topic
unsubscribe link and removing the redundant inner check.
2 participants
Test 2
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#2