Refactor existing code for Discord Service #65
Conversation
…to DiscordBaseService
* Refactor: Centralize command names in utils and update handlers to use constants * Refactor: Replace hardcoded command names with constants from utils
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes centralize command name management by replacing hardcoded strings with constants from the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DiscordBaseService
participant CommandService
Client->>DiscordBaseService: POST Request (Discord interaction)
DiscordBaseService->>DiscordBaseService: Read & unmarshal request body
DiscordBaseService->>CommandService: Invoke HandleMessage
CommandService->>CommandService: Evaluate command (Hello / Listening / Verify)
alt Command recognized
CommandService->>Client: Respond with 200 OK and corresponding message
else Unknown command
CommandService->>Client: Respond with 400 Bad Request
end
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
service/verifyService.go (1)
10-20: Function implementation lacks error handling and documentation.While the implementation is functional, consider the following improvements:
- Add error handling for potential request parsing failures
- Add function documentation comments explaining the purpose and parameters
- Consider making the "Work in progress" message a constant or configurable value
+// VerifyService handles Discord verify command interactions +// It returns a message indicating the verification feature is under development func (s *CommandService) VerifyService(response http.ResponseWriter, request *http.Request) { - msg := "Work in progress for Verify command" + const verifyInProgressMsg = "Work in progress for Verify command" + + // Check if the request is valid + if request.Body == nil { + utils.Error.BadRequest(response, "Request body is empty") + return + } + messageResponse := &discordgo.InteractionResponse{ Type: discordgo.InteractionResponseChannelMessageWithSource, Data: &discordgo.InteractionResponseData{ - Content: msg, + Content: verifyInProgressMsg, Flags: discordgo.MessageFlags(64), //Ephemeral message flag }, } utils.Success.NewDiscordResponse(response, "Success", messageResponse) }controllers/baseHandler_test.go (1)
18-18: Update test function name to match the handler being tested.The test function is still named
TestHomeHandler, but it's now testingDiscordBaseHandler. Consider renaming the test function toTestDiscordBaseHandlerto better reflect what's being tested.-func TestHomeHandler(t *testing.T) { +func TestDiscordBaseHandler(t *testing.T) {controllers/baseHandler.go (1)
10-12: Remove the redundantreturn.Since there's nothing else after calling
service.DiscordBaseService(response, request), the finalreturnis unnecessary.Apply the following diff:
func DiscordBaseHandler(response http.ResponseWriter, request *http.Request, params httprouter.Params) { service.DiscordBaseService(response, request) - return }🧰 Tools
🪛 golangci-lint (1.62.2)
12-12: S1023: redundant
returnstatement(gosimple)
service/main.go (1)
26-27: Log the invalid command name for better debugging.Currently, the user receives “Invalid Command” but doesn’t know which command caused the error. Consider including the command name for better traceability.
Example fix:
default: + // Log the invalid command name for debugging + // log.Printf("Invalid command: %s", s.discordMessage.Data.Name) utils.Errors.NewBadRequestError(response, "Invalid Command") return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
commands/handlers/main.go(2 hunks)commands/handlers/main_test.go(1 hunks)commands/main.go(2 hunks)controllers/baseHandler.go(1 hunks)controllers/baseHandler_test.go(4 hunks)dtos/commands.go(1 hunks)fixtures/message.go(2 hunks)models/discord_test.go(1 hunks)queue/main_test.go(1 hunks)routes/baseRoute.go(1 hunks)service/discordBaseService.go(1 hunks)service/helloService_test.go(1 hunks)service/listeningService.go(1 hunks)service/listeningService_test.go(1 hunks)service/main.go(1 hunks)service/main_test.go(2 hunks)service/verifyService.go(1 hunks)utils/constants.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
controllers/baseHandler.go
12-12: S1023: redundant return statement
(gosimple)
🔇 Additional comments (27)
dtos/commands.go (1)
3-7: Well-structured command name type definitionThis is a good approach to centralize command name definitions. Using a struct with named fields makes the code more maintainable and reduces the risk of typos when referencing command names throughout the codebase.
utils/constants.go (2)
3-4: Clean import of the new dtos packageThe import is correctly structured and follows Go conventions.
8-12: Effective centralization of command name constantsGood implementation of the command name constants using the new struct type. This aligns with the PR objective of refactoring services and improving maintainability through struct usage.
service/listeningService.go (1)
31-31: Replaced hardcoded string with constant referenceGood replacement of the hardcoded string with the centralized constant. This makes the code more maintainable and consistent with the overall refactoring approach.
queue/main_test.go (1)
111-111: Consistent use of command name constant in testsGreat job updating the test to use the same constant as the production code. This ensures that if command names are changed in the future, tests won't break due to inconsistency.
commands/handlers/main_test.go (1)
9-9: Code modernization: Using constants instead of hardcoded stringsThis change improves maintainability by replacing the hardcoded command name with a constant from the utils package. This type of refactoring reduces the risk of typos and makes future changes to command names easier to manage.
Also applies to: 17-17
routes/baseRoute.go (1)
10-10: Handler replacement as part of service refactoringThe change from
HomeHandlertoDiscordBaseHandleraligns with the PR's objective of refactoring services. This modification shifts the implementation from a direct handler to a struct-based service approach.Were there any functional changes in behavior between the old
HomeHandlerand the newDiscordBaseHandler? If so, it would be helpful to document them.commands/handlers/main.go (1)
9-9: Using constants for command namesGood refactoring of hardcoded strings to constants. This change is consistent with the modifications in other files and improves code maintainability.
Also applies to: 29-29
service/helloService_test.go (1)
25-27: Improved initialization of CommandService structThis change improves the initialization pattern by creating a new instance of
CommandServicewith properly set fields instead of modifying an existing global instance. This aligns with the goal of refactoring services to use struct throughout the codebase.service/listeningService_test.go (1)
32-32: Good refactoring to use constants instead of hardcoded strings.Using
utils.CommandNames.Listeninginstead of a hardcoded string is a positive change that improves code maintainability and consistency. This aligns with the PR objective of refactoring to use Struct throughout the codebase.fixtures/message.go (2)
4-6: Good addition of the utils import.Adding the utils import is necessary for accessing the CommandNames struct. This change supports the refactoring effort.
28-28: Good refactoring to use constants instead of hardcoded strings.Replacing the hardcoded string with
utils.CommandNames.Helloimproves maintainability and consistency. This is in line with the PR objective of refactoring to use Struct throughout the codebase.controllers/baseHandler_test.go (1)
22-22: Consistent handler replacement throughout test cases.All instances of
HomeHandlerhave been properly replaced withDiscordBaseHandler. This is a good consistent change that aligns with the refactoring objective.Also applies to: 30-30, 40-40, 53-53, 68-68
models/discord_test.go (2)
6-6: Good addition of utils import.The import of the utils package is necessary for accessing the CommandNames constants, which aligns with the overall refactoring approach.
12-12: Appropriate use of command name constant.Replacing the hardcoded string "hello" with utils.CommandNames.Hello improves maintainability by centralizing command name definitions. This change is consistent with the PR's goal of refactoring the codebase.
commands/main.go (3)
3-6: Clean import organization.The imports are properly organized to include the utils package needed for the CommandNames constants.
10-10: Good replacement of string literals with constants.Replacing hardcoded command name strings with constants from utils.CommandNames improves code maintainability and consistency across the codebase.
Also applies to: 14-15
25-28: Well-structured new Verify command.The new Verify command is properly defined following the same structure as other commands. The description clearly communicates its purpose of generating a link with a user-specific token for RDS backend integration.
service/discordBaseService.go (3)
13-25: Well-implemented request handling and error checks.The function properly reads the request body, checks for errors, and handles empty payloads appropriately. The error responses are clear and follow the application's error handling pattern.
26-41: Good use of switch statement for interaction types.The switch statement effectively handles different Discord interaction types with appropriate responses for each case. The InteractionPing case returns a Pong response, and the InteractionApplicationCommand case delegates to the MainService method of the CommandService struct.
33-37: Clean implementation of command delegation.Creating a CommandService struct with the parsed message and delegating to its MainService method follows good OOP principles, aligning with the PR's goal of using struct methods throughout the codebase.
service/main_test.go (4)
20-23: Good refactoring to use CommandService struct.The test has been properly updated to use the CommandService struct, aligning with the PR's goal of refactoring to use struct methods throughout the codebase.
28-53: Well-structured test for ListeningService.This new test case thoroughly verifies the ListeningService functionality by creating a proper DiscordMessage with the necessary data and validating both the response status and content.
55-74: Comprehensive test for the new Verify command.This test case properly validates the VerifyService functionality, checking both the response status and content. The "Work in progress" message suggests that the full implementation of this feature might still be ongoing.
86-92: Appropriate handling of unknown commands.The test for unknown commands has been updated to use the CommandService struct and now expects a more appropriate HTTP status code (BadRequest instead of OK) when a command is not recognized.
service/main.go (2)
7-7: Good addition of theutilsimport.Using a centralized constants file helps avoid typos and promotes maintainability.
14-25: Clean and clear command dispatch logic.Referencing
utils.CommandNamesinstead of literal strings makes the code more robust. Each command case is handled succinctly and returned early. This improves readability and ensures no further logic is executed after handling a specific command.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controllers/baseHandler_test.go(1 hunks)service/discordBaseService_test.go(1 hunks)
🔇 Additional comments (6)
controllers/baseHandler_test.go (2)
13-20: Test coverage has been significantly reducedMany test cases seem to have been removed, leaving only one test case that checks for a 400 status when the request body is empty. This reduces the test coverage for this handler.
Consider adding more test cases to maintain comprehensive test coverage, especially if the functionality has been moved to
DiscordBaseService. Verify if the removed test cases are now covered inservice/discordBaseService_test.go.
17-17: Addednilparameter without clear purposeThe
DiscordBaseHandleris called with anilparameter, which is not explained and could indicate a missing dependency.Please confirm that passing
nilas the third parameter is intentional and document the purpose of this parameter. If this is related to the refactoring mentioned in the PR objectives, ensure that the parameter is handled correctly in the implementation.service/discordBaseService_test.go (4)
19-33: Comprehensive test cases for error handlingGood job implementing thorough test cases for empty and malformed request bodies. These tests ensure robustness in handling invalid input.
35-61: Well-structured successful interaction testsThe tests for valid requests and interaction commands are well-structured and verify both the status code and response content. This provides good coverage of the happy path scenarios.
63-71: Edge case handling for unknown interaction typeGood inclusion of a test case for unknown interaction types. This ensures the service degrades gracefully when encountering unexpected inputs.
1-72: Test duplication with controller testThere seems to be duplication between this test file and
controllers/baseHandler_test.go. The first test case (empty request body) appears in both files, testing similar functionality at different layers.While testing at different layers can be beneficial, ensure that there's a clear understanding of what's being tested at each layer to avoid unnecessary duplication. Consider documenting the testing strategy or refactoring to make the distinction clearer.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
service/discordBaseService.go (2)
14-19: Reposition thedeferstatement to ensure execution.The
defer request.Body.Close()statement should be placed immediately afterio.ReadAll(request.Body)and before any potential early returns to ensure it's always executed.func DiscordBaseService(response http.ResponseWriter, request *http.Request) { payload, err := io.ReadAll(request.Body) + defer request.Body.Close() if err != nil || len(payload) == 0 { utils.Errors.NewBadRequestError(response, "Invalid Request Payload") return } - defer request.Body.Close()
39-41: Consider adding more context to the default case response.The default case returns a 200 OK status without any explanation or content, which might make debugging harder. Consider adding a response body or logging to provide more context.
default: + utils.Success.NewResponse(response, "Unhandled interaction type", http.StatusOK) - response.WriteHeader(http.StatusOK) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
service/discordBaseService.go(1 hunks)service/helloService.go(1 hunks)service/helloService_test.go(1 hunks)service/listeningService.go(3 hunks)service/listeningService_test.go(4 hunks)service/main.go(1 hunks)service/main_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- service/listeningService.go
- service/main_test.go
- service/helloService_test.go
- service/listeningService_test.go
🔇 Additional comments (4)
service/discordBaseService.go (1)
13-42: Overall implementation aligns well with the refactoring objectives.The new DiscordBaseService function follows good practices for handling HTTP requests, properly validates inputs, and delegates command processing to the CommandService struct. This change supports the PR objective of using Struct throughout the codebase to improve structure and maintainability.
service/main.go (3)
14-29: Good refactoring to use struct methods and constants.This change successfully implements the receiver method pattern, making use of the CommandService struct and improving maintainability with command name constants. The addition of the Verify command case and the improved error handling also enhance the codebase structure.
14-14: Method signature correctly implements the previous review suggestion.The method signature now uses a pointer receiver for CommandService, as suggested in the previous review.
22-24:Details
✅ Verification successful
Verify that the VerifyService method exists.
Make sure that the VerifyService method is properly implemented on the CommandService struct, as it's being called here but isn't visible in the current file.
🏁 Script executed:
#!/bin/bash # Check if the VerifyService method is defined on CommandService rg -p "func \(.*CommandService\) VerifyService\(" --type goLength of output: 183
Validation Complete: VerifyService method is correctly implemented.
- The
VerifyServicemethod is successfully defined on theCommandServicestruct inservice/verifyService.go(line 10), matching its usage inservice/main.go.- No further modifications are required as the method implementation is properly located and called.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
service/main.go(1 hunks)service/verifyService.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- service/verifyService.go
🔇 Additional comments (5)
service/main.go (5)
7-7: Good addition of utils import.Adding the utils import allows using centralized constants and error handling utilities, which is a positive change for maintainability.
14-14: Method signature change looks good.Converting from a standalone function to a method on the CommandService struct improves encapsulation and follows object-oriented principles. Consider using
sinstead ofserviceas the receiver variable name for consistency with other methods in the codebase, as suggested in a previous review.
15-21: Great improvement using constants for command names.Replacing string literals with constants from utils.CommandNames improves maintainability and reduces the risk of typos. The command pattern implementation is clean and follows a consistent structure.
22-24: New Verify command added correctly.The Verify command case is properly implemented, following the same pattern as the other commands.
25-27: Improved error handling for invalid commands.Using utils.Errors.NewBadRequestError is a more robust way to handle invalid commands compared to directly writing HTTP status codes. This approach provides better error messages and consistent error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
service/main.go (1)
14-15:⚠️ Potential issueAdd nil check for discordMessage before accessing its properties.
There's no validation of the discordMessage field before accessing its properties. If it's not properly initialized, this could cause a panic at runtime.
Add a nil check at the beginning of the method:
func (service *CommandService) HandleMessage(response http.ResponseWriter, request *http.Request) { + // Since we're using a value type now, we need to check if Data is nil + if service.discordMessage.Data == nil { + utils.Errors.NewBadRequestError(response, "Invalid message data") + return + } switch service.discordMessage.Data.Name {
🧹 Nitpick comments (1)
service/main.go (1)
14-29: Add error handling for service method calls.The service methods (Hello, Listening, Verify) might return errors that are currently not being handled. Consider updating these methods to return errors and handle them appropriately in this method.
func (service *CommandService) HandleMessage(response http.ResponseWriter, request *http.Request) { + // Check if Data is nil to prevent panic + if service.discordMessage.Data == nil { + utils.Errors.NewBadRequestError(response, "Invalid message data") + return + } switch service.discordMessage.Data.Name { case utils.CommandNames.Hello: - service.Hello(response, request) + if err := service.Hello(response, request); err != nil { + // Handle error appropriately + http.Error(response, err.Error(), http.StatusInternalServerError) + } return // Apply similar pattern to other cases
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
fixtures/message.go(2 hunks)service/discordBaseService.go(1 hunks)service/listeningService_test.go(7 hunks)service/main.go(1 hunks)service/main_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- service/main_test.go
- service/listeningService_test.go
- service/discordBaseService.go
- fixtures/message.go
🔇 Additional comments (4)
service/main.go (4)
7-7: Good addition of utils package import.Adding the utils import allows using the CommandNames constants, which improves maintainability by centralizing command name definitions.
16-24: Good usage of command name constants.Using constants from utils.CommandNames instead of string literals improves code maintainability and reduces the risk of typos.
The addition of the Verify command handler is well-structured and consistent with the existing pattern.
25-28: Improved error handling in default case.Using the centralized error handling utility is better than writing the HTTP status code directly. This ensures consistent error responses throughout the application.
11-11:Details
✅ Verification successful
Consider whether discordMessage should be a value or pointer type.
The discordMessage field was changed from a pointer type to a value type. For large structs, this could impact performance due to unnecessary copying. Evaluate whether a pointer would be more appropriate based on the size of dtos.DiscordMessage and how it's used.
🏁 Script executed:
#!/bin/bash # Check the size and structure of DiscordMessage struct echo "Examining DiscordMessage structure:" fd -e go -x grep -A 20 "type DiscordMessage struct" {}Length of output: 594
Performance and Memory Consideration for
discordMessage:
- The
dtos.DiscordMessagestruct is defined with several strings and pointers and totals roughly 80 bytes on a 64-bit system.- Given this moderate size, the copying overhead when used as a value is minimal under typical conditions.
- Unless profiling indicates that copying is occurring in a performance-critical hot path, using a value type here is acceptable. However, if heavy copying patterns or future size increases are anticipated, switching to a pointer type might be beneficial.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
dtos/commands.go (2)
3-7: Good addition of a centralized CommandNameTypes struct.This struct helps eliminate hardcoded command name strings throughout the codebase, which is a good practice for maintainability. If the command names ever need to change, they can be modified in one place.
Consider adding documentation comments to explain the purpose of this struct and its fields for better readability.
9-13: Well-structured CommandError type with proper JSON tags.The JSON tags are appropriately defined for API responses. The structure covers the basic needs of an error response with message, status code, and success flag.
For better maintainability, consider adding:
- Documentation comments explaining the purpose and usage of this struct
- Common error constants or factory methods for frequently used error scenarios
queue/main.go (1)
5-5: Import added but only used for debugging.The
fmtpackage is only being used for a debug print statement. Consider whether this debug statement is necessary for production code.service/listeningService.go (1)
56-57: Unnecessary use of fmt.Sprintf with a non-format string.The current code uses
fmt.Sprintf(msg)wheremsgis already a string. This is unnecessary and flagged by static analysis.- Content: fmt.Sprintf(msg), + Content: msg,🧰 Tools
🪛 golangci-lint (1.62.2)
56-56: printf: non-constant format string in call to fmt.Sprintf
(govet)
service/discordBaseService_test.go (4)
38-39: Add error handling for JSON marshaling operations.The current code ignores potential marshaling errors. While these are unlikely to occur in tests with fixed data, it's a good practice to handle these errors for completeness and to catch any unexpected issues.
- jsonBytes, _ := json.Marshal(message) + jsonBytes, err := json.Marshal(message) + assert.Nil(t, err) r, _ := http.NewRequest("POST", "/", bytes.NewBuffer(jsonBytes))
51-52: Add error handling for JSON marshaling operations.Similar to the previous case, error handling should be added for JSON marshaling operations.
- jsonBytes, _ := json.Marshal(fixtures.HelloCommand) + jsonBytes, err := json.Marshal(fixtures.HelloCommand) + assert.Nil(t, err) r, _ := http.NewRequest("POST", "/", bytes.NewBuffer(jsonBytes))
66-67: Add error handling for JSON marshaling operations.Error handling should be added for JSON marshaling operations here as well.
- jsonBytes, _ := json.Marshal(message) + jsonBytes, err := json.Marshal(message) + assert.Nil(t, err) r, _ := http.NewRequest("POST", "/", bytes.NewBuffer(jsonBytes))
18-72: Consider testing more edge cases.The current test suite covers basic cases well, but consider adding tests for:
- Different command types beyond HelloCommand
- Various error scenarios from the service
- Invalid interaction data structures
This would provide more comprehensive coverage of the DiscordBaseService functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
dtos/commands.go(1 hunks)queue/main.go(2 hunks)service/discordBaseService.go(1 hunks)service/discordBaseService_test.go(1 hunks)service/listeningService.go(3 hunks)service/listeningService_test.go(7 hunks)service/main_test.go(2 hunks)utils/errorHandler.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- service/discordBaseService.go
- service/listeningService_test.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
service/listeningService.go
56-56: printf: non-constant format string in call to fmt.Sprintf
(govet)
47-47: ineffectual assignment to msg
(ineffassign)
🔇 Additional comments (13)
utils/errorHandler.go (1)
25-29: Good refactoring to make error handling more flexible.The updated
NewInternalErrormethod now accepts variadic string parameters, allowing for custom error messages while maintaining a default message. This change aligns well with the existingNewUnauthorisedErrormethod's pattern, creating consistency across the error handling utilities.service/listeningService.go (5)
15-16: Function name change aligns with service refactoring.The method name has been updated from
ListeningServicetoListeningto maintain consistency with the new struct-based approach mentioned in the PR objectives.
33-33: Good use of constants for command names.Replacing the hardcoded string with
utils.CommandNames.Listeningimproves maintainability and reduces the risk of typos.
42-43: Improved error handling with proper logging.Error handling has been enhanced with detailed logging via logrus and using the new flexible error handler.
48-49: Improved error handling with proper logging.Same pattern of enhanced error handling applied consistently throughout the file.
57-57: Enhanced clarity with typed constant.Replacing the magic number
64withdiscordgo.MessageFlags(64)improves code readability by explicitly showing it's a message flag.service/main_test.go (5)
20-26: Good refactoring to use struct-based approach.The test has been updated to use the
CommandServicestruct withHandleMessagemethod, which aligns with the PR's objective of refactoring services to use structs throughout the codebase.
28-53: Comprehensive test for the Listening command.This new test case verifies that the Listening command functions correctly when a user is already set to listen. The test includes proper setup of the Discord message structure and verification of the response.
55-83: Excellent addition of failure case testing.This addresses the previous review comment about missing failure case tests. The test now properly verifies error handling when the queue service fails, including checking the error message and status code.
84-102: Good test coverage for the new Verify command.This test case ensures the newly added Verify command returns the expected response, which helps maintain code quality as new features are added.
106-121: Updated test expectations for unknown commands.The test now correctly expects a BadRequest status code for unknown commands, which is more appropriate than OK. This reflects an improvement in error handling.
service/discordBaseService_test.go (2)
27-33: Consider using 400 status code for malformed request body.Currently, the test expects a 500 Internal Server Error for a malformed request body. However, a 400 Bad Request might be more appropriate since this is a client error (invalid request format) rather than a server error. This aligns with HTTP standards where client-side errors use the 4xx range.
- t.Run("Should return 500 when request body is malformed", func(t *testing.T) { + t.Run("Should return 400 when request body is malformed", func(t *testing.T) { w := httptest.NewRecorder() r, _ := http.NewRequest("POST", "/", bytes.NewBuffer([]byte("malformed request"))) service.DiscordBaseService(w, r) - assert.Equal(t, http.StatusInternalServerError, w.Code) + assert.Equal(t, http.StatusBadRequest, w.Code) })
63-71:Details
❓ Verification inconclusive
Verify the design decision for handling unknown interaction types.
The test expects a 200 OK response for unknown interaction types. Confirm this is the intended behavior rather than returning an error status code. This depends on your API design, but typically unknown or invalid interaction types might warrant a 400 Bad Request response.
🏁 Script executed:
#!/bin/bash # Check how Discord's API documentation recommends handling unknown interaction types rg -i "unknown interaction type" --type mdLength of output: 44
Action Required: Confirm API Behavior for Unknown Interaction Types
The test in
service/discordBaseService_test.go(lines 63-71) expects a 200 OK response when an unknown interaction type is received. Please verify that this behavior is intentional per the API design and aligns with our usage requirements (or with Discord’s API guidelines if applicable). If a non-200 response would be more appropriate (e.g., a 400 Bad Request for invalid inputs), consider adjusting both the test expectations and the service implementation accordingly.
… cases for correct status codes
…sponse formatting
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
services/discordBaseService.go (2)
14-20: Consider adding error logging for debugging purposes.While the error handling logic is correct, adding logs for errors could help with debugging issues in production environments.
if err != nil || len(payload) == 0 { + utils.Logger.Error("Failed to read request body or empty payload", "error", err) utils.Errors.NewBadRequestError(response, "Invalid Request Payload") return }
22-26: Error logging recommendation for unmarshaling errors.Consider logging the unmarshaling error for better observability.
var message dtos.DiscordMessage if err := json.Unmarshal(payload, &message); err != nil { + utils.Logger.Error("Failed to unmarshal Discord message", "error", err) utils.Errors.NewBadRequestError(response, err.Error()) return }services/main_test.go (2)
31-31: Consider extracting expected response text to a constant.Using string literals directly in assertions makes tests harder to maintain if the message format changes. Consider storing this message in a constant or helper function.
- assert.Equal(t, response.Data.Content, "Hey there \u003c@123456789012345678\u003e! Congratulations, you just executed your first slash command") + const expectedHelloResponse = "Hey there \u003c@123456789012345678\u003e! Congratulations, you just executed your first slash command" + assert.Equal(t, expectedHelloResponse, response.Data.Content)
122-126: Remove unnecessary empty line.There's an extra blank line at position 123 that could be removed for consistency with the rest of the codebase.
CS := CommandService{discordMessage: discordMessage} - w := httptest.NewRecorder() r, _ := http.NewRequest("GET", "/", nil) CS.HandleMessage(w, r)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
services/discordBaseService.go(1 hunks)services/main_test.go(1 hunks)utils/errorHandler.go(2 hunks)
🔇 Additional comments (10)
utils/errorHandler.go (3)
25-29: Good improvement to error handling flexibility.The updated signature with variadic parameters makes the
NewInternalErrormethod more flexible and consistent with the existingNewUnauthorisedErrormethod. The null check with default error message is a good defensive programming practice.
35-36: Well implemented string formatting with fmt.SprintfReplacing string concatenation with
fmt.Sprintffollows Go best practices and improves code readability and maintainability. This change addresses a previous review comment effectively.
29-29: Consider consistent naming convention for error handlersWhile the function name
formatErroraccurately describes its purpose (it formats error responses rather than just returning them), you might want to consider a consistent naming convention across your error handling functions for better maintainability.services/discordBaseService.go (2)
13-43: Well-structured Discord interaction handler implementation.The DiscordBaseService function follows a clean pattern for handling Discord interactions, with proper payload validation and response handling for different interaction types.
40-42: Response details in default case.No response body is being sent in the default case, just a status code. This is fine since Discord only cares about the status code, but consider adding a log entry for unknown interaction types for monitoring purposes.
default: + utils.Logger.Warn("Unknown Discord interaction type received", "type", message.Type) response.WriteHeader(http.StatusNotFound)services/main_test.go (5)
18-21: Good setup and cleanup pattern for test configuration.The test properly sets and restores the MAX_RETRIES configuration, ensuring test isolation and preventing side effects that could impact other tests.
42-42: Good use of command constants.Using
utils.CommandNames.Listeninginstead of hardcoded strings improves maintainability and ensures consistency across the codebase.
61-89: Well-structured error case test.This test properly checks the error scenario for the ListeningService, ensuring robust error handling and response construction.
90-109: Good coverage for the new verify command.The test properly validates the "Work in progress" response for the newly added verify command functionality.
1-130: Well-structured test suite with good coverage.The test suite thoroughly covers the various command scenarios handled by the CommandService, including happy paths and error cases. The tests create appropriate mock DiscordMessage objects, simulate HTTP requests, and make comprehensive assertions on both response content and status codes.
iamitprakash
left a comment
iamitprakash
left a comment
There was a problem hiding this comment.
test coverage stats are missing
Added. |
why it was not there on first place, and what is over all coverage |
|
@joyguptaa errorhandler has 0% and few files has below 90, fix those first without any excuse. @pankajjs wait for me, for time being please don't approve and send me your PR's too |
iamitprakash
left a comment
iamitprakash
left a comment
There was a problem hiding this comment.
why it was not there on first place, and what is over all coverage
Have raised the MR for that |
| handler := MainHandler(data) | ||
| assert.NotNil(t, handler) | ||
| }) | ||
| t.Run("should handle error and return nil for 'listening' command", func(t *testing.T) { |
There was a problem hiding this comment.
Q: What does this line mean in the test case? Why does 'listening' return a nil?
There was a problem hiding this comment.
The purpose of this test was to demonstrate that if something goes wrong when listening to the command when queueing handover tasks to the listening command handler,
But I will change it, and also review other test cases' titles to make sure they all make sense.
Date: 26 Feb 2025
Developer Name: Joy Gupta
Issue Ticket Number
Description
Refactored MainService and DiscordBaseService. Using Struct throughout to access different service methods
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Discord Base Service
Main Service
Hello Service
Summary by CodeRabbit
New Features
Refactor