Skip to content
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

Merged
merged 12 commits into from Apr 26, 2024
Merged
7 changes: 7 additions & 0 deletions about.json
@@ -0,0 +1,7 @@
{
"tests": {
"requiredPlugins": [
"https://github.com/discourse/discourse-assign"
]
}
}
31 changes: 31 additions & 0 deletions app/lib/plugin_initializer.rb
@@ -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
Copy link
Contributor

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 ?

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
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor

@jjaffeux jjaffeux Apr 25, 2024

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, ...

Copy link
Contributor

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?

Copy link
Contributor

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 👍

end
end
end
2 changes: 2 additions & 0 deletions config/locales/server.en.yml
Expand Up @@ -11,6 +11,8 @@ 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."
ignore_solved_topics_in_assigned_reminder: "Prevent assigned reminder from including solved topics. only relevant when discourse-assign."
assignment_status_on_solve: "When a topic is solved update all assignments to this status"
disable_solved_education_message: "Disable education message for solved topics."
accept_solutions_topic_author: "Allow the topic author to accept a solution."
solved_add_schema_markup: "Add QAPage schema markup to HTML."
Expand Down
5 changes: 5 additions & 0 deletions config/settings.yml
Expand Up @@ -32,6 +32,11 @@ discourse_solved:
client: true
notify_on_staff_accept_solved:
default: false
ignore_solved_topics_in_assigned_reminder:
default: false
assignment_status_on_solve:
type: string
default: ""
disable_solved_education_message:
default: false
accept_solutions_topic_author:
Expand Down
10 changes: 10 additions & 0 deletions plugin.rb
Expand Up @@ -622,4 +622,14 @@ def self.skip_db?
end
end
end

if defined?(DiscourseAssign)
on(:accepted_solution) do |post|
next if SiteSetting.assignment_status_on_solve.blank?
Assigner.new(post.topic, post.acting_user).assign(
post.acting_user,
status: SiteSetting.assignment_status_on_solve,
)
end
end
end
31 changes: 31 additions & 0 deletions spec/integration/solved_spec.rb
Expand Up @@ -366,4 +366,35 @@
expect(response.status).to eq(200)
end
end

context "with discourse-assign installed", if: defined?(DiscourseAssign) do
let(:admin) { Fabricate(:admin) }
fab!(:group)

before do
SiteSetting.solved_enabled = true
SiteSetting.assign_enabled = true
SiteSetting.enable_assign_status = true
SiteSetting.assign_allowed_on_groups = "#{group.id}"
SiteSetting.assigns_public = true
SiteSetting.assignment_status_on_solve = "Done"

group.add(p1.acting_user)
group.add(user)

sign_in(user)
end

it "update all assignments to this status when a post is accepted" do
assigner = Assigner.new(p1.topic, user)
result = assigner.assign(user)
expect(result[:success]).to eq(true)

expect(p1.topic.assignment.status).to eq("New")
DiscourseSolved.accept_answer!(p1, user)

expect(p1.reload.custom_fields["is_accepted_answer"]).to eq("true")
expect(p1.topic.assignment.reload.status).to eq("Done")
end
end
end