Skip to content

Conversation

Zabuzard
Copy link
Member

@Zabuzard Zabuzard commented Oct 7, 2022

Overview

Fixes #603 which describes a bug when using /top-helpers resulting in a too long message, not accepted by Discord.

The approach is simple yet effective:

  • reduced the rows from 20 to 18
  • abbreviate long user names (limit 15)
  • move the description out of the column

these factors effectively reduced the message that was too long from 2300 characters down to 1100 characters, which is perfectively within limits.

Notes

Since this class also needed abbreviation logic, I elevated the abbreviate method from within the DiscordLogForwarder to MessageUtils. And also gave it a unit test.

@Zabuzard Zabuzard added bug Something isn't working priority: normal labels Oct 7, 2022
@Zabuzard Zabuzard self-assigned this Oct 7, 2022
@Zabuzard Zabuzard requested review from a team as code owners October 7, 2022 10:34
@Zabuzard Zabuzard force-pushed the bugfix/top_helper_too_long branch 2 times, most recently from 68a7866 to 3a94d89 Compare October 9, 2022 06:41
Copy link
Member

@Tais993 Tais993 left a comment

Choose a reason for hiding this comment

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

Can you add screenshots of how it looks now?

Tais993
Tais993 previously approved these changes Oct 10, 2022
Copy link
Member

@Tais993 Tais993 left a comment

Choose a reason for hiding this comment

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

next time give screenshot of before your changes and after.
Would make it a little easier to understand what actually changed

@Zabuzard
Copy link
Member Author

next time give screenshot of before your changes and after. Would make it a little easier to understand what actually changed

sure. was just a bit difficult, since i dont have 20+ users on my private server to make the problem obvious. and i was too lazy to mock the DB just for screenshots.

and the real tables from PROD contained sensitive info that i also was too lazy to mock lol.

but yeah, agreed.

Taz03
Taz03 previously approved these changes Oct 11, 2022
* limit long user names
* move description before table
* reduce amount of helpers listed
@Zabuzard Zabuzard dismissed stale reviews from Taz03 and Tais993 via 4944510 October 11, 2022 12:03
@Zabuzard Zabuzard force-pushed the bugfix/top_helper_too_long branch from 8cb5b76 to 4944510 Compare October 11, 2022 12:03
@Zabuzard
Copy link
Member Author

just rebased, no code changes. already had 2 approvals. merging once pipeline succeeds

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@Zabuzard Zabuzard merged commit 1911c64 into develop Oct 11, 2022
@Zabuzard Zabuzard deleted the bugfix/top_helper_too_long branch October 11, 2022 12:41
@Zabuzard Zabuzard mentioned this pull request Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority: normal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TopHelpersCommand message is too long

3 participants