-
Notifications
You must be signed in to change notification settings - Fork 83
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
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
c6f8794
FEATURE: Prevents assign notification & change status on solved
Grubba27 0487888
DEV: Address review comments
Grubba27 e63af67
DEV(WIP): Add tests for integration with discourse-assign
Grubba27 c1d37ac
DEV: lint solved_spec.rb
Grubba27 e102d82
DEV: Update test where it updates all assignments
Grubba27 3e0a596
DEV: lint & add tests for assigns_reminder_assigned_topics_query
Grubba27 6603d84
DEV: Update tests based on review feedback
Grubba27 9d0e602
DEV: Add describe to spec for discourse-assign integration tests
Grubba27 f398843
DEV: update describe name for discourse-assing spec integration
Grubba27 ab17e39
DEV: Add more tests to spec for discourse-assign integration
Grubba27 1a3a77d
DEV: Lint solved_spec
Grubba27 8584bf6
DEV: Lint and update spec to not have `p1` topic inside
Grubba27 File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"tests": { | ||
"requiredPlugins": [ | ||
"https://github.com/discourse/discourse-assign" | ||
] | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
# 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 AssignsReminderForTopicsQuery < 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.not( | ||
id: | ||
TopicCustomField.where( | ||
name: ::DiscourseSolved::ACCEPTED_ANSWER_POST_ID_CUSTOM_FIELD, | ||
).pluck(:topic_id), | ||
) | ||
end | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -366,4 +366,61 @@ | |
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 | ||
|
||
context "with using assigns_reminder_assigned_topics_query modifier" do | ||
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 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? |
||
class DummyClass | ||
def test | ||
DiscoursePluginRegistry.apply_modifier(:assigns_reminder_assigned_topics_query, Topic.all) | ||
end | ||
end | ||
|
||
before { SiteSetting.ignore_solved_topics_in_assigned_reminder = true } | ||
|
||
it "should not include solved topics in the query" do | ||
topic = Fabricate(:topic) | ||
topic2 = Fabricate(:topic) | ||
topic3 = Fabricate(:topic) | ||
post = Fabricate(:post, topic: topic) | ||
|
||
DiscourseSolved.accept_answer!(post, Discourse.system_user) | ||
|
||
topics = DummyClass.new.test.to_a | ||
|
||
expect(topics).not_to include(topic) | ||
|
||
expect(topics).to include(topic2) | ||
expect(topics).to include(topic3) | ||
end | ||
end | ||
end |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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