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

Add Option to Force Replies in Thread for Google Chat Bot #488

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

vsychov
Copy link
Contributor

@vsychov vsychov commented Nov 3, 2023

Proposed change

This patch introduces a enhancement to the bot functionality with Google Chat. It has been observed that Google Chat has ceased to provide the event.Space.Threaded value, leading to an issue where the bot could not determine whether a message was sent in a thread or directly to the group. As a result, the bot was defaulting to responding only in the group and never in a thread.

To resolve this issue, a new global configuration option,google_chat_force_reply_to_thread, has been added to the bot's settings. This boolean setting allows administrators to specify whether the bot should always reply within a thread if available or continue to respond in the group. The new setting ensures that the bot's replies are consistent with the desired conversation flow, enhancing the overall user interaction with the bot within Google Chat environments.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

@vsychov vsychov changed the title enhance(gchat): add way to send messages to thread in google chat Add Option to Force Replies in Thread for Google Chat Bot Nov 3, 2023
@coveralls
Copy link

Pull Request Test Coverage Report for Build 6826967060

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%

Totals Coverage Status
Change from base Build 6826937162: 0.0%
Covered Lines: 0
Relevant Lines: 0

💛 - Coveralls

Copy link
Member

@wass3rw3rk wass3rw3rk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the PR - just one question - looks good otherwise. grüße nach Berlin!

@@ -95,6 +94,7 @@ func toMessage(m *pubsub.Message) (models.Message, error) {
if event.User != nil {
message.Vars["_user.name"] = event.User.DisplayName
message.Vars["_user.id"] = event.User.Name
message.Vars["_user.internal_id"] = event.User.Name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious about this, since it's already available as _user.id?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_user.id is overwritten in the following lines by the email, if it exists.

message.Vars["_user.id"] = domainEvent.User.Email

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks! I missed that, sorry. I wonder why we are not using _user.email for that other spot as we are using that for Slack. Seems a little cleaner.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@justmiles friendly ping - was there are particular reason for overwriting _user.id with email? otherwise, i think populating _user.email would be more appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wass3r , I absolutely agree that _user.email looks much more logical and cleaner, I left it as is, adding a new field only in order not to break backward compatibility in existing user scripts. However, I am happy to rename the fields if you see no issues with backward compatibility.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vsychov i appreciate that you made the change in a thoughtful manner! i'll give it a day or so for chance of response here. otherwise, happy to merge this and might raise PR myself for making that breaking change and call it out in release notes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll go ahead and merge. thanks again.

@vsychov vsychov requested a review from wass3rw3rk November 11, 2023 07:37
@wass3rw3rk wass3rw3rk merged commit b294bca into target:main Nov 15, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants