FEATURE: Prevents assign notification & change status on solved #285

Grubba27 · 2024-04-23T21:16:30Z

Screen.Recording.2024-04-23.at.18.09.02.mov

Todo:

Create tests
Tests for this pr: DEV: add modifier to reminder assigned topics query discourse-assign#565

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

markvanlan · 2024-04-24T13:02:59Z

app/lib/plugin_initializer.rb

+  class AssignsRemainderForTopicsQuery < PluginInitializer
+    def apply_plugin_api
+      plugin.register_modifier(:assigns_reminder_assigned_topics_query) do |query|
+        next query if !SiteSetting.prevent_assign_notification


We will need this site setting to be more specific, something like ignore_solved_topics_in_assigned_reminder

markvanlan · 2024-04-24T13:05:07Z

config/locales/server.en.yml

@@ -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."


Same here maybe "Prevent assigned reminder from including solved topics. only relevant when discourse-assign"

markvanlan · 2024-04-24T13:06:04Z

config/locales/server.en.yml

@@ -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."


Maybe - "When a topic is solved update all assignments to this status"

markvanlan · 2024-04-24T13:08:33Z

config/settings.yml

@@ -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:


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.

markvanlan · 2024-04-24T13:09:42Z

config/settings.yml

+    default: false
+  assign_to_status_on_solved:
+    default: false
+  status_to_assign_on_solved:


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

jjaffeux · 2024-04-24T20:48:02Z

app/lib/plugin_initializer.rb

+    end
+  end
+
+  class AssignsRemainderForTopicsQuery < PluginInitializer


Is this remainder or reminder ?

jjaffeux · 2024-04-24T20:52:11Z

app/lib/plugin_initializer.rb

+      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


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.

Ideally a good API doesn't require changes if the core code implementation changes, which is very unlikely here.

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

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

how would modifiers even work then if this is a problem? Every instance of apply_modifier is a version of this problem then, correct?

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.

Grubba27 · 2024-04-25T21:27:02Z

plugin.rb

+  require_relative "app/lib/plugin_initializer"
+  DiscourseSolved::AssignsReminderForTopicsQuery.new(self).apply_plugin_api


Is there a better way to do this?

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

markvanlan · 2024-04-25T22:06:26Z

spec/integration/solved_spec.rb

+    end
+  end
+
+  context "with using assigns_reminder_assigned_topics_query modifier" do


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

Grubba27 mentioned this pull request Apr 23, 2024

DEV: add modifier to reminder assigned topics query discourse/discourse-assign#565

Merged

markvanlan reviewed Apr 24, 2024

View reviewed changes

Grubba27 added 4 commits April 24, 2024 11:06

DEV: Address review comments

0487888

Update SiteSettings names.

DEV(WIP): Add tests for integration with discourse-assign

e63af67

Add test for integration with discourse-assign plugin checks if the assignment status is moved to `Done`

DEV: lint solved_spec.rb

c1d37ac

DEV: Update test where it updates all assignments

e102d82

Change `on(:accepted_solution)` is defined Update test to use acting_user instead of admin

jjaffeux reviewed Apr 24, 2024

View reviewed changes

DEV: lint & add tests for assigns_reminder_assigned_topics_query

3e0a596

Linted and added tests for `assigns_reminder_assigned_topics_query` modifier.

Grubba27 commented Apr 25, 2024

View reviewed changes

Grubba27 marked this pull request as ready for review April 25, 2024 21:34

markvanlan reviewed Apr 25, 2024

View reviewed changes

DEV: Update tests based on review feedback

6603d84

change plugin_initializer location update spec with new tests to test integration with discourse-assign

markvanlan approved these changes Apr 26, 2024

View reviewed changes

Grubba27 added 5 commits April 26, 2024 11:14

DEV: Add describe to spec for discourse-assign integration tests

9d0e602

DEV: update describe name for discourse-assing spec integration

f398843

DEV: Add more tests to spec for discourse-assign integration

ab17e39

DEV: Lint solved_spec

1a3a77d

DEV: Lint and update spec to not have p1 topic inside

8584bf6

Grubba27 merged commit 2b6e17d into main Apr 26, 2024
4 checks passed

Grubba27 deleted the feature/assigns-reminder-assigned-topics branch April 26, 2024 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEATURE: Prevents assign notification & change status on solved #285

FEATURE: Prevents assign notification & change status on solved #285

Grubba27 commented Apr 23, 2024 •

edited

markvanlan Apr 24, 2024

markvanlan Apr 24, 2024

markvanlan Apr 24, 2024

markvanlan Apr 24, 2024

markvanlan Apr 24, 2024

jjaffeux Apr 24, 2024

jjaffeux Apr 24, 2024

jjaffeux Apr 24, 2024

markvanlan Apr 25, 2024

jjaffeux Apr 25, 2024 •

edited

markvanlan Apr 25, 2024

jjaffeux Apr 25, 2024

Grubba27 Apr 25, 2024

markvanlan Apr 25, 2024

markvanlan Apr 25, 2024

		require_relative "app/lib/plugin_initializer"
		DiscourseSolved::AssignsReminderForTopicsQuery.new(self).apply_plugin_api

FEATURE: Prevents assign notification & change status on solved #285

FEATURE: Prevents assign notification & change status on solved #285

Conversation

Grubba27 commented Apr 23, 2024 • edited

Todo:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjaffeux Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Grubba27 commented Apr 23, 2024 •

edited

jjaffeux Apr 25, 2024 •

edited