fix: Ensure s/ command only triggers on prefix and improve tests - #14
Open
thehunmonkgroup wants to merge 1 commit into
Open
fix: Ensure s/ command only triggers on prefix and improve tests#14thehunmonkgroup wants to merge 1 commit into
thehunmonkgroup wants to merge 1 commit into
Conversation
Corrects the MessageWillBePosted hook to only process messages explicitly starting with s/ after trimming whitespace. Previously, any message containing s/ could incorrectly trigger the replacement logic. Improves error handling for invalid command formats and cases where no previous post is found by sending an ephemeral message and preventing the original s/ message from being posted. Updates tests to align with the corrected logic, adds cases for non-commands and invalid formats, and adjusts mock expectations to be conditional based on the execution path, resolving previous test failures.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This PR addresses a bug where the
s/replacement command could be incorrectly triggered by messages that containeds/but did not start with it.It also improves test coverage and reliability.
Changes:
server/plugin.go:MessageWillBePostedto strictly check if the trimmed messagestrings.HasPrefix("s/"). Messages not starting withs/are now correctly ignored.splitAndValidateInput(e.g.,s/foo,s//). An ephemeral error message is now sent to the user, and the invalid command message is prevented from posting.getLastPostfails to find a post. It now sends the specific error via ephemeral message and prevents thes/command from posting (returnsdismiss_post).server/plugin_test.go:MessageWillBePosted.TestExecuteCommandto cover:s/.s/command formats.setupAPI) and conditionalized mock expectations within the test loop to accurately reflect the different API calls made depending on whether the input is a valid command, an invalid format, or not a command at all.testAPIConfig) to provide necessary details (like User ID, Team ID) required by the code under test, fixing specific mock mismatches.Context:
These changes were made to fix the core prefix bug and improve user feedback for errors, while adapting the existing test structure to support properly testing the code changes, rather than performing a major refactor of the tests.
It was also necessary to make some minor modifications to the Makefile and go.mod to support more modern versions of Go.