-
Notifications
You must be signed in to change notification settings - Fork 9
FEATURE: per-topic unsubscribe option in emails #2
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?
Conversation
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.
Greptile Summary
This PR implements a per-topic email unsubscribe feature that allows users to unsubscribe from notifications for specific topics rather than all email notifications. The implementation spans both frontend and backend components:
Backend Changes:
- Adds a new
unsubscribeaction toTopicsControllerthat toggles notification levels between regular and muted states - Introduces new routes
/t/:slug/:topic_id/unsubscribeand/t/:topic_id/unsubscribefor handling unsubscribe requests - Extends the
Topicmodel with anunsubscribe_urlmethod that generates topic-specific unsubscribe URLs - Updates
UserNotificationsmailer to include the per-topic unsubscribe URL in email notifications - Modifies email templates to ensure unsubscribe links are consistently displayed
Frontend Changes:
- Creates new Ember.js components including a route (
topic-unsubscribe.js.es6), controller (topic-unsubscribe.js.es6), view, and template for the unsubscribe interface - Adds client-side routing support for the unsubscribe functionality
- Implements defensive programming in the dropdown button component to handle optional titles
- Updates CSS styles to support the new unsubscribe interface components
Localization and Configuration:
- Adds new localization strings for both server and client-side unsubscribe messaging
- Updates email notification templates to include per-topic unsubscribe options alongside existing preference links
- Includes comprehensive test updates for the
Email::MessageBuilderto handle the new unsubscribe URL parameter
The feature integrates seamlessly with Discourse's existing notification system while providing users more granular control over their email subscriptions. Users can now unsubscribe from individual topic notifications directly from email links without affecting their global notification preferences.
Confidence score: 2/5
- This PR has significant implementation issues that make it unsafe to merge in its current state
- Multiple critical bugs including typos, potential nil pointer exceptions, missing error handling, and deprecated API usage could cause immediate production failures
- All files need attention, particularly the controllers, JavaScript components, templates, and CSS files which contain syntax errors and logic flaws
18 files reviewed, 4 comments
|
|
||
| export default ObjectController.extend({ | ||
|
|
||
| stopNotificiationsText: function() { |
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.
syntax: Typo: 'stopNotificiationsText' should be 'stopNotificationsText' (missing 'a' in 'Notifications')
| stopNotificiationsText: function() { | |
| stopNotificationsText: function() { |
| @@ -0,0 +1,8 @@ | |||
| <div class="container"> | |||
| <p> | |||
| {{{stopNotificiationsText}}} | |||
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.
syntax: Typo in property name: 'stopNotificiationsText' should be 'stopNotificationsText' (missing 'n' in 'Notifications')
| {{{stopNotificiationsText}}} | |
| {{{stopNotificationsText}}} |
| 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]) |
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.
logic: Potential nil pointer exception - if no TopicUser record exists, tu will be nil and calling methods on it will crash
| tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id]) | |
| tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id]) | |
| return render json: {error: "Topic user not found"}, status: 404 unless tu |
| :change_post_owners, | ||
| :bookmark] | ||
| :bookmark, | ||
| :unsubscribe] |
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.
logic: Authentication concern - email unsubscribe links typically work without login, but this requires authentication
|
|
||
| <% 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: CSS selector syntax error: '.previous-discussion' should not have a leading dot in the class attribute
| <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4> | |
| <h4 class='previous-discussion'><%= t "user_notifications.previous_discussion" %></h4> |
Test 2