add setting page to add notification message feature #99
Conversation
…interfaces_gen.go、schema.sql、models.gen.go、oas_request_encoders_gen.go、query.sql、usecase.go、server.go、interface.go*** ***
WalkthroughThis update introduces a new Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yaml
Files ignored due to path filters (13)
api/prel_api/oas_client_gen.gois excluded by:!**/*_gen.goapi/prel_api/oas_handlers_gen.gois excluded by:!**/*_gen.goapi/prel_api/oas_interfaces_gen.gois excluded by:!**/*_gen.goapi/prel_api/oas_json_gen.gois excluded by:!**/*_gen.goapi/prel_api/oas_request_decoders_gen.gois excluded by:!**/*_gen.goapi/prel_api/oas_request_encoders_gen.gois excluded by:!**/*_gen.goapi/prel_api/oas_response_decoders_gen.gois excluded by:!**/*_gen.goapi/prel_api/oas_response_encoders_gen.gois excluded by:!**/*_gen.goapi/prel_api/oas_router_gen.gois excluded by:!**/*_gen.goapi/prel_api/oas_schemas_gen.gois excluded by:!**/*_gen.goapi/prel_api/oas_server_gen.gois excluded by:!**/*_gen.goapi/prel_api/oas_unimplemented_gen.gois excluded by:!**/*_gen.goapi/prel_api/oas_validators_gen.gois excluded by:!**/*_gen.go
Files selected for processing (25)
- Makefile (1 hunks)
- api/api.yaml (2 hunks)
- db/query.sql (1 hunks)
- db/schema.sql (1 hunks)
- go.mod (1 hunks)
- internal/gateway/notification/notification.go (4 hunks)
- internal/gateway/postgresql/models.gen.go (1 hunks)
- internal/gateway/postgresql/query.sql.gen.go (2 hunks)
- internal/gateway/repository/setting.go (1 hunks)
- internal/handler/api_handler.go (2 hunks)
- internal/handler/page_handler.go (1 hunks)
- internal/interface.go (1 hunks)
- internal/model/repository.go (1 hunks)
- internal/model/setting.go (1 hunks)
- internal/usecase/request.go (5 hunks)
- internal/usecase/usecase.go (2 hunks)
- static/js/admin_setting.js (1 hunks)
- test/e2e/setting_scenario.spec.ts (1 hunks)
- test/integration/request_test.go (4 hunks)
- test/integration/setting_test.go (1 hunks)
- test/testutil/server.go (2 hunks)
- tools/go.mod (1 hunks)
- web/template/template.go (3 hunks)
- web/template/templates/_header.tpl (1 hunks)
- web/template/templates/admin_setting.tpl (1 hunks)
Files skipped from review due to trivial changes (1)
- tools/go.mod
Additional comments: 35
internal/interface.go (1)
- 22-23: Adding a
messageparameter to bothSendRequestMessageandSendJudgeMessagemethods in theNotificationClientinterface is a significant change. Ensure all implementations of this interface are updated accordingly to avoid compilation errors. This change enhances the flexibility of notification messages, allowing for more dynamic content.test/e2e/setting_scenario.spec.ts (1)
- 6-34: The end-to-end test for setting notification messages is well-structured and covers the essential interactions with the admin settings page. It's good practice to also verify the persistence of these settings by checking the database or through the application's API directly, ensuring that the settings are not only updated in the UI but also correctly stored and retrievable.
static/js/admin_setting.js (1)
- 1-44: The JavaScript code for handling setting updates is clear and functional. However, consider adding more descriptive error messages to help users understand what went wrong during the update process. Additionally, it might be beneficial to disable the update button after a click to prevent multiple submissions until the current request is completed.
internal/usecase/usecase.go (1)
- 19-25: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [22-41]
The addition of the
settingRepofield to theUsecasestruct and its initialization in theNewUsecasefunction is correctly implemented. This change integrates the settings repository into the use case layer, allowing for the manipulation of settings within the application's business logic. Ensure that thesettingRepois utilized appropriately in the use case methods that require access to settings.internal/gateway/postgresql/models.gen.go (1)
- 44-50: The
Settingstruct is well-defined with appropriate fields for storing notification messages and timestamps. Usingpgtype.Textandpgtype.Timestamptzfor the fields ensures compatibility with PostgreSQL data types. This struct will facilitate the storage and retrieval of settings from the database effectively.internal/gateway/repository/setting.go (1)
- 33-49: The
Findmethod provides a sensible default by returning a new setting object if no rows are found in the database. This approach simplifies the handling of settings in the application by ensuring that aSettingobject is always available. Good use of error wrapping for detailed error reporting.internal/model/setting.go (1)
- 5-62: The
Settingmodel and its methods are well-designed, providing a clear and encapsulated way to manage setting data within the application. The use of private fields with public accessor and updater methods follows good object-oriented design principles. The generation of a new UUID for each new setting ensures uniqueness. This model will serve as a solid foundation for settings management.web/template/templates/admin_setting.tpl (1)
- 1-55: The HTML template for the admin settings page is well-structured and follows good practices for form design. Ensure that user input from the textareas is properly sanitized on the server side before being stored or displayed elsewhere in the application to prevent XSS attacks. The use of
maxlengthattribute on textareas is a good practice to limit input size.web/template/template.go (3)
- 16-16: The addition of
AdminSettingPageTplconstant is correctly implemented and follows the existing pattern for template constants.- 87-90: The
AdminSettingPagestruct is well-defined, adhering to Go's naming conventions and struct declaration practices.- 34-34: The inclusion of
AdminSettingPagein thePageDatastruct is correctly implemented, enabling the passing of new admin settings page data to templates.test/integration/setting_test.go (1)
- 1-65: The integration tests for the settings feature are well-structured and logically sound. The setup using
BeforeEachand the clear separation of tests for updating request and judge notification messages demonstrate adherence to best testing practices.internal/model/repository.go (1)
- 50-53: The
SettingRepositoryinterface is correctly declared, adhering to Go's interface declaration practices. TheFindandSavemethods are well-defined, facilitating dependency injection and mocking in tests.db/schema.sql (1)
- 49-55: The creation statement for the
settingtable is correctly formatted, including appropriate data types for columns and a primary key declaration. This table structure supports the new settings feature effectively.web/template/templates/_header.tpl (1)
- 40-42: The addition of the "Setting" navigation item in the header template is correctly implemented within an admin conditional block and follows Bootstrap's conventions for navigation items.
Makefile (1)
- 7-7: The addition of the
npx playwright installcommand to theinstalltarget in the Makefile is correctly implemented and ensures that Playwright dependencies are installed as part of the project setup.go.mod (1)
- 3-3: Updating the Go version in
go.modto1.22.1is correctly done, indicating the minimum Go version required for the project. The removal of thetoolchaindirective is appropriate if it's no longer needed.internal/usecase/request.go (2)
- 91-102: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [79-99]
The addition of settings handling in the
CreateRequestandJudgeRequestfunctions, including the declaration ofsetting *model.Settingand its retrieval from the repository, is correctly implemented. This enhances the feature's flexibility by allowing use case functions to access and use settings data.
- 109-119: The updates to the parameters passed to
SendRequestMessageandSendJudgeMessagefunctions to include setting-based messages are correctly implemented. This ensures that the notification messages can be customized based on the settings, providing a more flexible notification system.Also applies to: 186-196
internal/gateway/notification/notification.go (4)
- 41-42: The addition of the
messageparameter toSendRequestMessagemethod is correctly positioned and follows Go's convention for parameter ordering. However, ensure that all calls to this method throughout the codebase have been updated to include the new parameter.- 50-51: Similar to the previous comment, the addition of the
messageparameter toSendJudgeMessageis correctly implemented. Again, verify that all invocations of this method have been updated accordingly.Verification successful
The verification process, based on the corrected script's execution, suggests that all invocations of
SendJudgeMessagehave been updated to include the newmessageparameter. This aligns with the expectation that there should be no calls toSendJudgeMessagewith the outdated parameter count. Therefore, the review comment is verified as accurate.* 70-73: The `requestMessage` method correctly incorporates the new `message` parameter. The logic for constructing the Slack message payload appears sound, ensuring that the custom message is included in the payload. This enhances the flexibility of notification messages. * 144-150: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [147-160]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to SendJudgeMessage without the new message parameter. Expecting no results. ast-grep --lang go --pattern $'SendJudgeMessage($_, $_, $_, $_, $_, $_, $_, $_)'Length of output: 81
Script:
#!/bin/bash # Corrected search for calls to SendJudgeMessage with the outdated parameter count (7 parameters instead of the updated 8). Expecting no results for correct implementation. ast-grep --lang go --pattern $'SendJudgeMessage($_, $_, $_, $_, $_, $_, $_)'Length of output: 77
The
judgeMessagemethod has been updated to include themessageparameter, which is used effectively to customize the judge notification messages. The switch statement for determining the message color based on the request status is a good practice for visual differentiation in notifications.db/query.sql (1)
- 247-261: The SQL queries for finding (
FindSetting) and upserting (UpsertSetting) settings in thesettingtable are correctly implemented. The use ofON CONFLICTfor the upsert operation is a good practice for handling potential duplicate entries. Ensure that appropriate indexes exist on theidcolumn of thesettingtable to optimize these operations.internal/handler/api_handler.go (1)
- 170-196: The
APISettingsPatchfunction correctly checks if the user is an admin before proceeding with the update operation, which is a crucial security measure. The use ofhtml.UnescapeStringfor unescaping HTML entities in the notification messages is appropriate to prevent HTML injection vulnerabilities. However, ensure that the updated messages are properly sanitized or escaped when rendered in a context where HTML or JavaScript execution is possible to prevent Cross-Site Scripting (XSS) attacks.test/testutil/server.go (2)
- 40-40: The addition of the
SettingRepofield to theTestHelperstruct is a good practice for providing easy access to the setting repository in tests. This enhances the testability of features related to settings.- 86-86: Correctly initializing the
SettingRepoin theNewTestHelperfunction ensures that tests have a consistent setup and can interact with the settings repository as needed. This is crucial for testing the new settings functionality.internal/handler/page_handler.go (1)
- 279-309: The
AdminSettingGetfunction correctly checks if the user is an admin before proceeding, which is essential for access control. The use oftemplate.ParseFSandtmpl.Executefor rendering the admin settings page follows best practices for handling dynamic content in Go'shtml/templatepackage, effectively preventing XSS vulnerabilities by automatically escaping dynamic content. Ensure that all dynamic content rendered on the page, especially user input, is processed through this templating system to maintain security.test/integration/request_test.go (3)
- 59-75: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [50-70]
The setup logic in the
BeforeEachblock for the "Create" test scenario, including the creation of a setting and the setup of a mock notification server, is well-implemented. The assertions within the notification server's handler effectively verify the content of the notification message.
- 59-75: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [72-90]
The test case "should create expected request" correctly sends a POST request to create a new request and verifies its creation by fetching it from the repository. The assertions are well-placed to ensure the request's properties match the expectations.
- 187-193: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [179-198]
The setup logic in the
BeforeEachblock for the "approve" context, including the creation of a setting and the setup of a mock notification server, is well-implemented. The assertions within the notification server's handler effectively verify the content of the notification message during the approval process.api/api.yaml (2)
- 304-327: The new endpoint
/admin/settingis well-defined with aGETmethod to return the admin setting page. The security and response specifications are appropriately set, ensuring consistency with the application's security model.- 784-819: The new endpoint
/api/settingsis well-defined with aPATCHmethod to update settings. The detailed request body schema allows for updating notification messages for requests and judgments. The security and response specifications are appropriately set, ensuring consistency with the application's API model.internal/gateway/postgresql/query.sql.gen.go (2)
- 457-472: The
FindSettingfunction correctly retrieves the settings from the database. However, consider adding error handling for the case where thesettingtable might be empty. This could lead to unexpected behavior if the application assumes the presence of at least one setting row.Consider adding a check after the
row.Scancall to handlesql.ErrNoRowsspecifically, possibly returning a custom error or a defaultSettinginstance.
- 822-845: The
UpsertSettingfunction is well-implemented for updating or inserting settings. However, ensure that theidused here is either statically defined or properly managed elsewhere in the application to avoid conflicts or unintended behavior.Verification successful
The
UpsertSettingfunction's use ofidis managed correctly across the application, with eachSettinginstance receiving a unique UUID as itsid. This approach minimizes the risk of conflicts or unintended behavior. The review comment's concern is addressed by the application's structured and best-practice-aligned 'id' management strategy.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the 'id' for settings is managed correctly across the application rg --type go "UpsertSetting"Length of output: 458
Script:
#!/bin/bash # Examine the context of UpsertSetting usage in setting.go to understand how 'id' is managed rg "UpsertSetting" internal/gateway/repository/setting.go -C 10Length of output: 684
Script:
#!/bin/bash # Examine the implementation of the ID() method within the model.Setting structure rg "func \(.*Setting\) ID()" --type go -C 10Length of output: 1056
| func (r *settingRepository) Save(ctx context.Context, req *model.Setting) error { | ||
|
|
||
| err := postgresql.GetQuery(ctx).UpsertSetting(ctx, postgresql.UpsertSettingParams{ | ||
| ID: req.ID(), | ||
| NotificationMessageForRequest: pgtype.Text{String: req.NotificationMessageForRequest(), Valid: true}, | ||
| NotificationMessageForJudge: pgtype.Text{String: req.NotificationMessageForJudge(), Valid: true}, | ||
| }) | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to save user") | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
The Save method in the setting repository correctly uses an upsert operation to either update existing settings or insert new ones if they don't exist. This approach is efficient for settings management. However, the error message "failed to save user" seems to be a copy-paste error and should be corrected to reflect the actual operation, such as "failed to save setting".
- return errors.Wrap(err, "failed to save user")
+ return errors.Wrap(err, "failed to save setting")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (r *settingRepository) Save(ctx context.Context, req *model.Setting) error { | |
| err := postgresql.GetQuery(ctx).UpsertSetting(ctx, postgresql.UpsertSettingParams{ | |
| ID: req.ID(), | |
| NotificationMessageForRequest: pgtype.Text{String: req.NotificationMessageForRequest(), Valid: true}, | |
| NotificationMessageForJudge: pgtype.Text{String: req.NotificationMessageForJudge(), Valid: true}, | |
| }) | |
| if err != nil { | |
| return errors.Wrap(err, "failed to save user") | |
| } | |
| return nil | |
| func (r *settingRepository) Save(ctx context.Context, req *model.Setting) error { | |
| err := postgresql.GetQuery(ctx).UpsertSetting(ctx, postgresql.UpsertSettingParams{ | |
| ID: req.ID(), | |
| NotificationMessageForRequest: pgtype.Text{String: req.NotificationMessageForRequest(), Valid: true}, | |
| NotificationMessageForJudge: pgtype.Text{String: req.NotificationMessageForJudge(), Valid: true}, | |
| }) | |
| if err != nil { | |
| return errors.Wrap(err, "failed to save setting") | |
| } | |
| return nil |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- internal/gateway/repository/setting.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- internal/gateway/repository/setting.go
Summary by CodeRabbit
settingtable to store notification messages and timestamps.