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

feat(hangouts): Threading! #138

Merged
merged 2 commits into from
Nov 1, 2023
Merged

feat(hangouts): Threading! #138

merged 2 commits into from
Nov 1, 2023

Conversation

akash1810
Copy link
Member

@akash1810 akash1810 commented Oct 31, 2023

What does this change?

Add support for threaded messages in Hangouts. This is useful when multiple messages are sent in quick succession from the same app, for example RepoCop. Threading messages together, hopefully, reduces the sense of spam/noise.

This change requires a new version of the libraries to be published. The additional field is optional, so should be backwards compatible for clients using older versions of the libraries.

How to test

Run the "dev" project locally and observe threaded messages:

sbt "project dev" "run fields --config-stage PROD --targets Stack=deploy --subject test --message testing --source test --hangouts --thread-key test"
image

Add support for threaded messages in Hangouts.

This is useful when multiple messages are sent in quick succession from the same app,
for example RepoCop.

The additional field is optional, so should be backwards compatible.
@akash1810 akash1810 marked this pull request as ready for review October 31, 2023 17:57
@akash1810 akash1810 requested review from a team October 31, 2023 17:58
@@ -46,7 +47,8 @@ case class Notification(
actions: List[Action],
target: List[Target],
channel: RequestedChannel,
sourceSystem: String
sourceSystem: String,
threadKey: Option[String] // only used for Hangouts messages
Copy link
Member Author

Choose a reason for hiding this comment

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

This Notification case class is the contract between Anghammarad and users. Adding the additional field as an Option should mean this change is backwards compatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: we could default this to None?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes! I hadn't set a default value so I could see all the compiler warnings. Can safely set one now though.

Copy link
Contributor

@jacobwinch jacobwinch left a comment

Choose a reason for hiding this comment

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

Great work 👍

@akash1810 akash1810 merged commit 587cb86 into main Nov 1, 2023
1 check passed
@akash1810 akash1810 deleted the aa/threading branch November 1, 2023 09:11
@akash1810
Copy link
Member Author

akash1810 commented Nov 1, 2023

This has been released now in v1.8.1 of the Scala plugin, and v1.1.0 in the NPM module.

EDIT: I've unpublished v1.1.0 from NPM, and released this change as v1.8.1 instead - https://www.npmjs.com/package/@guardian/anghammarad?activeTab=versions. We haven't followed semver here, but the version numbers align at least.

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.

None yet

2 participants