-
Notifications
You must be signed in to change notification settings - Fork 0
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?
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; | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+4
to
+10
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. 🛠️ Refactor suggestion Consider adding error handling for the async operation. The model loading pattern is appropriate, but there's no error handling if Consider adding error handling: model(params) {
const topic = this.store.createRecord("topic", { id: params.id });
- return PostStream.loadTopicView(params.id).then(json => {
+ return PostStream.loadTopicView(params.id).then(json => {
topic.updateFromJson(json);
return topic;
+ }).catch(error => {
+ // Handle the error appropriately - maybe redirect to topic or show error
+ console.error("Failed to load topic for unsubscribe:", error);
+ throw error;
});
},📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| 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}}} | ||||||
|
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. Fix property name to match controller correction. The template references Apply this diff to fix the typo: - {{{stopNotificiationsText}}}
+ {{{stopNotificationsText}}}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| </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 nil TopicUser record and consider reducing complexity. The unsubscribe logic is sound, but there are two concerns:
Apply this diff to handle the nil case: tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id])
+
+if tu.nil?
+ tu = TopicUser.create!(user_id: current_user.id, topic_id: params[:topic_id], notification_level: TopicUser.notification_levels[:muted])
+else
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!
+endFor complexity reduction, consider extracting the slug validation into a private method similar to the 📝 Committable suggestion
Suggested change
🧰 Tools🪛 RuboCop (1.76.1)[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> | ||||||
|
|
||||||
|
Comment on lines
+5
to
+7
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. Duplicate “respond_instructions” footer will render twice Lines 6 and 19 both emit - <% if context_posts.present? %>
- <div class='footer'>%{respond_instructions}</div>
+ <% if context_posts.present? %>Also applies to: 17-20 🤖 Prompt for AI Agents |
||||||
| <hr> | ||||||
|
|
||||||
| <% 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 commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Fix invalid CSS class attribute
-<h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
+<h4 class='previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
|
|
||||||
| <% 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.
Fix typo in method name.
There's a typo in the computed property name:
stopNotificiationsTextshould bestopNotificationsText(missing 'a' in "Notifications").Apply this diff to fix the typo:
Note: This change will also require updating the template file that references this property.
📝 Committable suggestion
🤖 Prompt for AI Agents