-
Notifications
You must be signed in to change notification settings - Fork 80
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
FEATURE: Prevents assign notification & change status on solved #285
Conversation
Relates to this [topic](https://meta.discourse.org/t/assign-plugin-for-informatica/256974/94) Add an event listener to `accepted_solution` event Add `assigns_reminder_assigned_topics_query` modifier to not notify if `prevent_assign_notification` setting is on. Add settings to prevent assign notification and change status on solved
app/lib/plugin_initializer.rb
Outdated
class AssignsRemainderForTopicsQuery < PluginInitializer | ||
def apply_plugin_api | ||
plugin.register_modifier(:assigns_reminder_assigned_topics_query) do |query| | ||
next query if !SiteSetting.prevent_assign_notification |
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.
We will need this site setting to be more specific, something like ignore_solved_topics_in_assigned_reminder
config/locales/server.en.yml
Outdated
@@ -11,6 +11,9 @@ en: | |||
solved_topics_auto_close_hours: "Auto close topic (n) hours after the last reply once the topic has been marked as solved. Set to 0 to disable auto closing." | |||
show_filter_by_solved_status: "Show a dropdown to filter a topic list by solved status." | |||
notify_on_staff_accept_solved: "Send notification to the topic creator when a post is marked as solution by a staff." | |||
prevent_assign_notification: "Prevent notification from discourse-assign for solved topics." |
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.
Same here maybe "Prevent assigned reminder from including solved topics. only relevant when discourse-assign"
config/locales/server.en.yml
Outdated
@@ -11,6 +11,9 @@ en: | |||
solved_topics_auto_close_hours: "Auto close topic (n) hours after the last reply once the topic has been marked as solved. Set to 0 to disable auto closing." | |||
show_filter_by_solved_status: "Show a dropdown to filter a topic list by solved status." | |||
notify_on_staff_accept_solved: "Send notification to the topic creator when a post is marked as solution by a staff." | |||
prevent_assign_notification: "Prevent notification from discourse-assign for solved topics." | |||
assign_to_status_on_solved: "Enable assigning a status when a topic is solved. Requires discourse-assign plugin." |
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.
Maybe - "When a topic is solved update all assignments to this status"
config/settings.yml
Outdated
@@ -32,6 +32,13 @@ discourse_solved: | |||
client: true | |||
notify_on_staff_accept_solved: | |||
default: false | |||
prevent_assign_notification: | |||
default: false | |||
assign_to_status_on_solved: |
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.
I actually think we don't need this site setting. You can default status_to_assign_on_solved
to an empty string rather than Done
and use a present?
or blank?
check in plugin.rb. If a string is present we basically know that this is enabled.
config/settings.yml
Outdated
default: false | ||
assign_to_status_on_solved: | ||
default: false | ||
status_to_assign_on_solved: |
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.
assignment_status_on_solve
maybe? Since it's not the status to assign, it's the status that assignments get updated to 🤷
Update SiteSettings names.
Add test for integration with discourse-assign plugin checks if the assignment status is moved to `Done`
Change `on(:accepted_solution)` is defined Update test to use acting_user instead of admin
app/lib/plugin_initializer.rb
Outdated
end | ||
end | ||
|
||
class AssignsRemainderForTopicsQuery < PluginInitializer |
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.
Is this remainder or reminder ?
app/lib/plugin_initializer.rb
Outdated
plugin.register_modifier(:assigns_reminder_assigned_topics_query) do |query| | ||
next query if !SiteSetting.ignore_solved_topics_in_assigned_reminder | ||
query.where( | ||
"topics.id NOT IN ( | ||
SELECT topic_id | ||
FROM topic_custom_fields | ||
WHERE name = 'accepted_answer_post_id' | ||
)", | ||
) | ||
end |
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.
I wonder if that doesn't deserve a better API than a plugin modifier. This create a very strong relationship between assign code and this code.
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.
Ideally a good API doesn't require changes if the core code implementation changes, which is very unlikely here.
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.
I personally think this isn't an issue. The modifier is generic enough that other plugins may use it, it seems like a good API that may be re-used. Gabriel is adding tests in the application of the modifier so we will detect any breaking changes on the discourse-assign side. This is how I would implement it as well
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.
It's not about if it can be used by other plugins or not. My point is that if we change how the query is implemented in discourse-assign every plugin will have to change, which goes against the idea of an API. You build API so you don't expose core, here you expose an internal AR query directly. So my suggestion is to reverse the API, so you can declare in a nice way what kind of filter you want to create and let discourse-assign decide how it will filter: sql, ruby, ...
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.
how would modifiers even work then if this is a problem? Every instance of apply_modifier is a version of this problem then, correct?
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.
To a certain extend yes. But 1) it's not like apply_modifier is our only solution
2) this case is kinda extreme because it's a query, if you were returning a list of results and filtering them that would be less annoying
This being said, Im not here to block your PR, Im just here to note that maybe we could do better here. If you want to go with this go with this 👍
Linted and added tests for `assigns_reminder_assigned_topics_query` modifier.
plugin.rb
Outdated
require_relative "app/lib/plugin_initializer" | ||
DiscourseSolved::AssignsReminderForTopicsQuery.new(self).apply_plugin_api |
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.
Is there a better way to do this?
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.
When there is only one... probably not. If there were multiple files you would loop over each at require, apply each one. This is probably fine. Although the name of the file should be more specific. Like app/lib/plugin_initializer.rb
-> lib/plugin_initializers/assigned_reminder_exclude_solved.rb
spec/integration/solved_spec.rb
Outdated
end | ||
end | ||
|
||
context "with using assigns_reminder_assigned_topics_query modifier" do |
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.
I don't know on this looking at it again. Since the test above relies directly on the discourse-assign plugin, maybe calling the AssignReminder class directly for testing is best? @jjaffeux want to weigh in here as well?
change plugin_initializer location update spec with new tests to test integration with discourse-assign
Screen.Recording.2024-04-23.at.18.09.02.mov
Todo: