Feature: add chat integration reference post #216

Grubba27 · 2024-08-29T20:33:33Z

This class works similarly to a post, but it is not a post.

Before merge needs discourse/discourse#28714

This also adds get_channel_by_name to be used by every provider when the automation script requires it.

To help out with the migration, use get_channel_name

This class works similar to a post but it is not a post.

add `send_chat_integration_message` scriptable that uses the rules to send a message to the chat provider add locale strings for the new scriptable update `ChatIntegrationReferencePost` `excerpt` method add tests for `ChatIntegrationReferencePost`

This makes using `trigger_notification` easier with every provider as well.

This method gets the name of the channel based on how the provider identifies it. Updates channel_name in locales yaml Adds migrate_tag_added_filter_to_all_providers.rb to move all existing rules to use Automation

Update small action locales with strings from core

markvanlan · 2024-09-04T15:09:52Z

lib/discourse_chat_integration/chat_integration_reference_post.rb

+      @topic = context["topic"]
+      @kind = context["kind"]
+      @raw = context["raw"] if context["raw"].present?
+      @full_url = (@topic.posts.empty? ? @topic.full_url : @topic.posts.first.full_url)


I think maybe move this logic down into the method rather than having in intiializer

markvanlan · 2024-09-04T15:10:29Z

lib/discourse_chat_integration/chat_integration_reference_post.rb

+              "topic_tag_changed.topic_tag_changed.added",
+              added: tag_list_to_raw.call(added_tags),
+            )
+          elsif removed.present?


Should this be removed_tags

lib/discourse_chat_integration/provider/matrix/matrix_provider.rb

jjaffeux · 2024-09-05T18:11:00Z

db/migrate/20240903184807_migrate_tag_added_filter_to_all_providers.rb

@@ -0,0 +1,93 @@
+# frozen_string_literal: true


We can't have any rails class in a migration, it should be all sql, if we change these class, migrations are borked

This ^. You'll have to inline whatever DiscourseChatIntegration::Provider.constants resolves to.

Update provider helper to use hashes instead of dot notation

…SQL queries Commented out migrate_tag_added_from_filter_to_automation.rb

jjaffeux · 2024-09-09T21:38:44Z

db/migrate/20240903184807_migrate_tag_added_filter_to_all_providers.rb

+              ).with_indifferent_access
+
+            provider_name = channel[:provider]
+            provider = DiscourseChatIntegration::Provider.get_by_name(provider_name)


I think even this should be avoided, we should harcode everything in migrations

jjaffeux · 2024-09-09T21:40:55Z

config/locales/client.en.yml

@@ -281,3 +281,12 @@ en:
              label: URL
            channel:
              label: Channel
+
+        send_chat_integration_message:
+            title: Send Chat-Integration message


is that valid indentation here? Maybe it's github tripping

It took me a print to find that it was actually with weird but valid indentation

jjaffeux · 2024-09-09T21:44:04Z

plugin.rb

+        begin
+          provider.trigger_notification(post, channel, nil)
+        rescue StandardError => _
+          Rails.logger.error("Error while sending chat integration message")


Maybe say it's coming from discourse automation here. It's one of the things I want to improve one day... a propre error report for automation, but meanwhile:

[discourse-automation] Error .... (automation id: x)

jjaffeux · 2024-09-09T21:44:36Z

plugin.rb

+        post = DiscourseChatIntegration::ChatIntegrationReferencePost.new(context)
+        provider = DiscourseChatIntegration::Provider.get_by_name(provider)
+
+        channel = provider.get_channel_by_name(channel_name) # user must have created a channel in /admin/plugins/chat-integration/<provider> page


Should we capture a specific exception in this case and log a specific message to explain this?

jjaffeux · 2024-09-09T21:45:23Z

plugin.rb

+        provider = fields.dig("provider", "value")
+        channel_name = fields.dig("channel_name", "value")
+
+        post = DiscourseChatIntegration::ChatIntegrationReferencePost.new(context)


Im not sure we should pass the context directly here, maybe be more specific on what you pass to DiscourseChatIntegration::ChatIntegrationReferencePost.new, the API of automation might send different things in the future

jjaffeux · 2024-09-09T21:46:22Z

Made few comments, congrats on your understanding of automation 👏 The approach looks good to me.

I can't approve because, the tricky part is the migration and I don't have the time to test this locally and more seriously.

spec/lib/discourse_chat_integration/chat_integration_reference_post_spec.rb

spec/integration/automation_spec.rb

lib/discourse_chat_integration/provider/discord/discord_provider.rb

lib/discourse_chat_integration/chat_integration_reference_post.rb

CvX · 2024-09-10T18:13:40Z

db/migrate/20240903184807_migrate_tag_added_filter_to_all_providers.rb

@@ -0,0 +1,93 @@
+# frozen_string_literal: true


This ^. You'll have to inline whatever DiscourseChatIntegration::Provider.constants resolves to.

…_post_spec.rb Co-authored-by: Jarek Radosz <[email protected]>

Co-authored-by: Jarek Radosz <[email protected]>

…tion

Grubba27 added 2 commits August 29, 2024 17:29

FEATURE: Add chat integration reference post

af5e7e8

This class works similar to a post but it is not a post.

DEV: change how excerpt method works

c5d7568

Grubba27 changed the title ~~Feature/add chat integration reference post~~ Feature: add chat integration reference post Aug 29, 2024

Grubba27 added 3 commits August 30, 2024 17:01

DEV: Add get_channel_by_name to every provider

b316558

This makes using `trigger_notification` easier with every provider as well.

DEV: Add get_channel_name to all providers

846996d

This method gets the name of the channel based on how the provider identifies it. Updates channel_name in locales yaml Adds migrate_tag_added_filter_to_all_providers.rb to move all existing rules to use Automation

Grubba27 marked this pull request as ready for review September 3, 2024 20:53

DEV: Add removal of old migration data

8c1513d

Update small action locales with strings from core

markvanlan reviewed Sep 4, 2024

View reviewed changes

Grubba27 added 6 commits September 4, 2024 13:33

DEV: solve review comments

3d54fd1

DEV: update test locale strings

6987ca3

DEV: remove empty line to trigger lint

96b91bf

DEV: lint applied

42e6830

DEV: Add tests for automation integration

1df0946

DEV: add rails logger for when automatio error occurs

f3eda71

jjaffeux reviewed Sep 5, 2024

View reviewed changes

Grubba27 added 4 commits September 5, 2024 18:17

DEV: move migration to be SQL only

96f8da9

Update provider helper to use hashes instead of dot notation

DEV: update migration with correct table names

5144318

DEV: Update migrate_tag_added_filter_to_all_providers to use smaller …

b865868

…SQL queries Commented out migrate_tag_added_from_filter_to_automation.rb

DEV: update comments in migration file

d5c0774

Grubba27 requested a review from jjaffeux September 9, 2024 14:07

jjaffeux reviewed Sep 9, 2024

View reviewed changes

Grubba27 added 2 commits September 10, 2024 11:25

DEV: update indentation in client.en.yml

620a5a9

DEV: update with review comments

2e07334

CvX reviewed Sep 10, 2024

View reviewed changes

Grubba27 and others added 6 commits September 10, 2024 15:39

Update spec/lib/discourse_chat_integration/chat_integration_reference…

e4d464e

…_post_spec.rb Co-authored-by: Jarek Radosz <[email protected]>

Update spec/lib/discourse_chat_integration/chat_integration_reference…

86979fb

…_post_spec.rb Co-authored-by: Jarek Radosz <[email protected]>

Update spec/lib/discourse_chat_integration/chat_integration_reference…

3d46798

…_post_spec.rb Co-authored-by: Jarek Radosz <[email protected]>

Update spec/integration/automation_spec.rb

94bdec9

Co-authored-by: Jarek Radosz <[email protected]>

Update lib/discourse_chat_integration/chat_integration_reference_post.rb

3ea5b6c

Co-authored-by: Jarek Radosz <[email protected]>

DEV: update specs with review comments

4f681a8

Grubba27 requested a review from CvX September 10, 2024 19:06

Grubba27 added 2 commits September 10, 2024 16:20

DEV: update typos in tests

fa2a8eb

DEV: inlined functions for getting channel name for provider in migra…

9a60a45

…tion

CvX approved these changes Sep 11, 2024

View reviewed changes

Grubba27 merged commit 3f8b67d into main Sep 11, 2024
5 checks passed

Grubba27 deleted the feature/add-chat-integration-reference-post branch September 11, 2024 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: add chat integration reference post #216

Feature: add chat integration reference post #216

Grubba27 commented Aug 29, 2024 •

edited

Loading

markvanlan Sep 4, 2024

markvanlan Sep 4, 2024

jjaffeux Sep 5, 2024

CvX Sep 10, 2024

jjaffeux Sep 9, 2024

jjaffeux Sep 9, 2024

Grubba27 Sep 10, 2024

jjaffeux Sep 9, 2024

jjaffeux Sep 9, 2024

jjaffeux Sep 9, 2024

jjaffeux commented Sep 9, 2024

CvX Sep 10, 2024

Feature: add chat integration reference post #216

Feature: add chat integration reference post #216

Conversation

Grubba27 commented Aug 29, 2024 • edited Loading

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

Choose a reason for hiding this comment

jjaffeux commented Sep 9, 2024

Choose a reason for hiding this comment

Grubba27 commented Aug 29, 2024 •

edited

Loading