fix: trim messages longer than 3000 chars #8219
Merged
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Filespackage.json
yarn.lock
|
nunogois
reviewed
Sep 23, 2024
nunogois
reviewed
Sep 23, 2024
|
|
||
| const { text: formattedMessage, url: featureLink } = | ||
| this.msgFormatter.format(event); | ||
| const text = formattedMessage.substring(0, 3000); |
There was a problem hiding this comment.
Just curious: Is this implementation simpler because this integration is deprecated?
There was a problem hiding this comment.
Simpler than what other implementation? I did it here because the "msgFormatter" is kind of generic, it doesn't say anything about Slack, so just for precaution I decided not to modify it. I was thinking "If this is a message formatter inside a Slack folder package by feature, then this might be only for Slack", but because it was not I just err on the safe side.
There was a problem hiding this comment.
Simpler than the one in Slack App. In my head it should be pretty much the same thing, so I was just curious why it wasn't.
There was a problem hiding this comment.
Oh, just because the slack-app is using the formatted text also as output for the event, this one is only using it for the push
There was a problem hiding this comment.
There's still a:
this.registerEvent({
integrationId,
state,
stateDetails: stateDetails.join('\n'),
event: serializeDates(event),
details: {
url,
channels: slackChannels,
username,
message: text,
},
});here, which means that there's a small inconsistency between Slack and Slack App in the way they register integration events: Slack App will tell us the text was trimmed to 3000 characters, but Slack will just trim it.
It's a small detail, so I think it's fine, but again, was just curious 😄
There was a problem hiding this comment.
Oh! Thanks! I didn't understand. This is the PR for consistency: #8230
There was a problem hiding this comment.
I just didn't see the message at the bottom
nunogois
approved these changes
Sep 23, 2024
Co-authored-by: Nuno Góis <[email protected]>
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.
About the changes
When trying to send messages longer than 3000 chars we get this error:
This PR trims the text length to 3000 chars.
We also upgrade slack API due to some security fixes: https://github.com/slackapi/node-slack-sdk/releases/tag/%40slack%2Fweb-api%407.3.4
After checking the migration guide to v7 it seems that none of the breaking changes affect us: https://github.com/slackapi/node-slack-sdk/wiki/Migration-Guide-for-web%E2%80%90api-v7
Testing
I did manual test this integration and the fix. The way to reproduce is adding a very long strategy name and sending that as an update on Slack:
Now the event succeeds and we notice on the integration event log that the message was trimmed:
