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
Changes from 5 commits
c6f8794
0487888
e63af67
c1d37ac
e102d82
3e0a596
6603d84
9d0e602
f398843
ab17e39
1a3a77d
8584bf6
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,7 @@ | ||
{ | ||
"tests": { | ||
"requiredPlugins": [ | ||
"https://github.com/discourse/discourse-assign" | ||
] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
# frozen_string_literal: true | ||
|
||
module DiscourseSolved | ||
class PluginInitializer | ||
attr_reader :plugin | ||
|
||
def initialize(plugin) | ||
@plugin = plugin | ||
end | ||
|
||
def apply_plugin_api | ||
# this method should be overridden by subclasses | ||
raise NotImplementedError | ||
end | ||
end | ||
|
||
class AssignsRemainderForTopicsQuery < PluginInitializer | ||
def apply_plugin_api | ||
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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 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 👍 |
||
end | ||
end | ||
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.
Is this remainder or reminder ?