Support webhook urls with missing threadkey parameter #24
Conversation
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
…eter
This parameter used to require a `{threadKey}` placeholder, which is
sometimes forgotten. We now support Webhook URLs that do not include
this placeholder, and strip the placeholder if it was present.
adamnfish
force-pushed
the
support-webhook-urls-with-missing-threadkey-parameter
branch
from
34836e6 to
a1037eb
Compare
AshCorr
approved these changes
Jan 12, 2024
There was a problem hiding this comment.
Awesome :)
Might be one for another PR, but you could add cargo test to https://github.com/guardian/actions-prnouncer/blob/main/.github/workflows/main.yml ?
src/google.rs
Outdated
Comment on lines
65
to
68
| match result { | ||
| Ok(url) => assert_eq!(url, String::from("https://example.com/ABCDEF?messageReplyOption=REPLY_MESSAGE_FALLBACK_TO_NEW_THREAD&threadKey=1234")), | ||
| Err(_) => assert!(false), | ||
| } |
There was a problem hiding this comment.
Suggested change
| match result { | |
| Ok(url) => assert_eq!(url, String::from("https://example.com/ABCDEF?messageReplyOption=REPLY_MESSAGE_FALLBACK_TO_NEW_THREAD&threadKey=1234")), | |
| Err(_) => assert!(false), | |
| } | |
| assert_eq!(url, Ok(String::from("https://example.com/ABCDEF?messageReplyOption=REPLY_MESSAGE_FALLBACK_TO_NEW_THREAD&threadKey=1234"))) |
Is it possible to assert on a result perhaps?
There was a problem hiding this comment.
That isn't possible (Result doesn't support equality checks), but Result::unwrap will work well in tests.
We include a test step in the GHA workflow file, and unwrap the Results in test assertions to simplify the code.
|
I've addressed both of these points, thank you! |
adamnfish
deleted the
support-webhook-urls-with-missing-threadkey-parameter
branch
|
We ran through testing this locally before merging 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
NOTE: this includes the changes in the new-inline-threads PR
What does this change?
Allows teams to provide a
{threadKey}placeholder if they want to, but also supports cases where this is not included.I am led to believe this sometimes casuses problems, when teams forget the parameter.
I'd suggest we officially deprecate the placeholder parameter from here, so that there is a single recommended approach. There's no reference to this placeholder in the README or elsewhere, so I've not made any documentation changes.
How to test
I've added tests for this logic, but we'll also need to make sure that the existing behaviour still works. I do not have the secrets for testing at my fingertips, so if someone wants to pair with me on testing this locally I'd be grateful!
How can we measure success?
Existing configurations should continue to work unchanged, and we might consider updating the webhook URL for one or more of the existing configurations, removing the deprecated threadKey placholder.
Have we considered potential risks?
Testing this change locally would mitigate the risk of breaking existign configurations.