Add device settings management with new API endpoints and database updates #145
Conversation
coderabbitai
bot
changed the title
WalkthroughThis update introduces a comprehensive device settings management feature, including database schema, repository, service, and HTTP API endpoints for both third-party and mobile clients. It adds new database models and migrations, implements filtering/merging utilities for settings, and integrates the settings module into the application's dependency injection. The push notification and webhooks modules are refactored to support settings updates, and API documentation is extended to cover the new endpoints. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ThirdPartyAPI
participant ThirdPartyController
participant SettingsService
participant SettingsRepository
participant DB
User->>ThirdPartyAPI: Authenticated HTTP request (GET/PUT/PATCH /3rdparty/v1/settings)
ThirdPartyAPI->>ThirdPartyController: Route to controller
ThirdPartyController->>SettingsService: Get/Update/ReplaceSettings(userID, settings)
SettingsService->>SettingsRepository: Query/update device_settings
SettingsRepository->>DB: Read/write device_settings table
SettingsRepository-->>SettingsService: Result/Error
SettingsService-->>ThirdPartyController: Filtered settings/Error
ThirdPartyController-->>ThirdPartyAPI: JSON response/Error
ThirdPartyAPI-->>User: Response
Note over ThirdPartyController,SettingsService: On settings update, may trigger push notification
sequenceDiagram
participant Device
participant MobileAPI
participant MobileController
participant SettingsService
participant SettingsRepository
participant DB
Device->>MobileAPI: Authenticated HTTP GET /mobile/v1/settings
MobileAPI->>MobileController: Route to controller
MobileController->>SettingsService: GetSettings(userID)
SettingsService->>SettingsRepository: Query device_settings
SettingsRepository->>DB: Read device_settings table
SettingsRepository-->>SettingsService: Settings/Error
SettingsService-->>MobileController: Filtered settings/Error
MobileController-->>MobileAPI: JSON response/Error
MobileAPI-->>Device: Response
sequenceDiagram
participant Service
participant DevicesService
participant PushService
Service->>DevicesService: Query devices for user (optionally by deviceID)
DevicesService-->>Service: List of devices
Service->>PushService: Enqueue push event for each device with push token
PushService-->>Service: Success/Error
Service-->>Caller: Combined result
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (23)
🚧 Files skipped from review as they are similar to previous changes (23)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 8
🧹 Nitpick comments (14)
internal/sms-gateway/models/migrations/mysql/20250521225803_add_device_settings.sql (2)
3-8: Consider adding auditing fields and ON UPDATE cascade
- A settings table is often updated frequently. Adding
created_at/updated_at(or embedding them later viaTimedModel) makes troubleshooting and eventual clean-up much easier.- If the
users.idvalue can ever be regenerated (unlikely but not impossible in some domains), anON UPDATE CASCADEkeeps the FK consistent.`created_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, `updated_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, CONSTRAINT `fk_device_settings_user` FOREIGN KEY (`user_id`) REFERENCES `users`(`id`) ON UPDATE CASCADE ON DELETE CASCADEPlease confirm whether you require temporal auditing; if so, I can supply the full migration diff.
11-14: Safe down-migration checkDropping the table is fine, but goose will run this on every environment.
If any other table referencesdevice_settingsin future,DROP TABLEwill fail. You may wish to addIF EXISTSto make rollback idempotent:DROP TABLE IF EXISTS `device_settings`;internal/sms-gateway/modules/devices/models.go (1)
15-17: Propagate migrate errors upstream
AutoMigratehides useful context (SQL, driver, etc.). Wrapping the error improves debuggability:-func Migrate(db *gorm.DB) error { - return db.AutoMigrate(&DeviceSettings{}) -} +func Migrate(db *gorm.DB) error { + if err := db.AutoMigrate(&DeviceSettings{}); err != nil { + return fmt.Errorf("device_settings migration failed: %w", err) + } + return nil +}internal/sms-gateway/handlers/mobile.go (1)
37-39: Nil-safety & route-scope clarification for settings controller
h.settingsCtrlis dereferenced unconditionally inRegister. If the DI graph fails to provide the controller (e.g. it was disabled behind a build tag), the app will panic.if h.settingsCtrl == nil { return fmt.Errorf("settings controller not initialised") }- The
/settingsroutes inherit thedeviceauth.Newmiddleware registered above. ✔️ Just confirm that the settings endpoints indeed require device auth; Swagger examples show both “user” & “device” contexts.- FX param struct already enforces presence, but you may want
fx.Annotate()withoptional:"true"if you foresee running without the feature.Also applies to: 309-310, 323-336
internal/sms-gateway/modules/devices/repository.go (3)
97-105: Consider explicit handling of "not found" case.The GetSettings method returns successfully even when no settings are found for the user. Consider either returning ErrNotFound or documenting that an empty settings object is returned when no record exists.
func (r *repository) GetSettings(userID string) (*DeviceSettings, error) { settings := &DeviceSettings{} err := r.db.Where("user_id = ?", userID).Limit(1).Find(settings).Error if err != nil { return nil, err } + // If the settings object is empty (no record found), return explicit error + if settings.UserID == "" { + return nil, ErrNotFound + } return settings, nil }
119-129: Remove commented-out code.Consider removing the commented-out alternative implementation using JSON_MERGE_PATCH. If this is kept for future reference, add a comment explaining why the current implementation was chosen over this approach.
- // return r.db. - // Clauses(clause.OnConflict{ - // DoUpdates: clause.Assignments( - // map[string]interface{}{ - // "settings": gorm.Expr("JSON_MERGE_PATCH(settings, VALUES(settings))"), - // }, - // ), - // }). - // Create(settings). - // Error
132-134: Consider adding transaction handling to ReplaceSettings.For consistency with UpdateSettings, consider adding transaction handling to ReplaceSettings, especially since it could potentially replace a concurrently modified record.
func (r *repository) ReplaceSettings(settings *DeviceSettings) (*DeviceSettings, error) { - return settings, r.db.Save(settings).Error + err := r.db.Transaction(func(tx *gorm.DB) error { + return tx.Save(settings).Error + }) + return settings, err }internal/sms-gateway/modules/push/service.go (1)
144-186: Consider optimizing error handling and loggingThe implementation of the Notify method is solid, but there are a few small opportunities for improvement:
- The error slice allocation could be optimized since not all devices will have push tokens
- The log message counts all devices, not just those that were actually notified
- errs := make([]error, 0, len(devices)) + errs := make([]error, 0) // Later in the code: + notifiedCount := 0 for _, device := range devices { if device.PushToken == nil { s.logger.Info("Device has no push token", zap.String("user_id", userID), zap.String("device_id", device.ID)) continue } if err := s.Enqueue(*device.PushToken, event); err != nil { s.logger.Error("Failed to send push notification", zap.String("user_id", userID), zap.String("device_id", device.ID), zap.Error(err)) errs = append(errs, err) + } else { + notifiedCount++ } } - s.logger.Info("Notified devices", append(logFields, zap.Int("count", len(devices)))...) + s.logger.Info("Notified devices", append(logFields, zap.Int("count", notifiedCount), zap.Int("total", len(devices)))...)internal/sms-gateway/modules/devices/utils.go (1)
65-90: Improve error message formattingThe error message uses string concatenation which is less efficient than using fmt.Errorf.
- return nil, errors.New("The field: '" + field + "' is not a map to dive") + return nil, fmt.Errorf("the field: '%s' is not a map to dive", field)This requires adding
fmtto the imports.internal/sms-gateway/handlers/settings/3rdparty.go (3)
67-75: Consider adding error details to the return errorThe body parser error handling could be improved by including the error details in the returned error message, making debugging easier.
- if err := h.BodyParserValidator(c, &smsgateway.DeviceSettings{}); err != nil { - return fiber.NewError(fiber.StatusBadRequest, err.Error()) - } + if err := h.BodyParserValidator(c, &smsgateway.DeviceSettings{}); err != nil { + return fiber.NewError(fiber.StatusBadRequest, fmt.Sprintf("Invalid settings format: %v", err)) + }
71-75: Consider error handling inconsistencyYou're making two body parse attempts, but only returning a generic error on the second parse. Consider consistent error handling.
- if err := c.BodyParser(&settings); err != nil { - return err - } + if err := c.BodyParser(&settings); err != nil { + return fiber.NewError(fiber.StatusBadRequest, fmt.Sprintf("Failed to parse request body: %v", err)) + }
87-95: Same validation inconsistency as in put methodConsider the same error handling improvement as suggested for the put method.
- if err := h.BodyParserValidator(c, &smsgateway.DeviceSettings{}); err != nil { - return fiber.NewError(fiber.StatusBadRequest, err.Error()) - } + if err := h.BodyParserValidator(c, &smsgateway.DeviceSettings{}); err != nil { + return fiber.NewError(fiber.StatusBadRequest, fmt.Sprintf("Invalid settings format: %v", err)) + } - if err := c.BodyParser(&settings); err != nil { - return err - } + if err := c.BodyParser(&settings); err != nil { + return fiber.NewError(fiber.StatusBadRequest, fmt.Sprintf("Failed to parse request body: %v", err)) + }internal/sms-gateway/handlers/settings/mobile.go (2)
40-47: Consider adding context to the error messageAdding more context to the error message would help with debugging.
- if err != nil { - return fmt.Errorf("can't get settings: %w", err) - } + if err != nil { + return fmt.Errorf("can't get settings for device %s (user ID: %s): %w", device.ID, device.UserID, err) + }
54-60: Missing Validator initialization in MobileControllerUnlike the ThirdPartyController, the MobileController doesn't initialize the Validator field in the base.Handler struct, which could lead to nil pointer dereferences if validation is attempted.
type mobileControllerParams struct { fx.In DevicesSvc *devices.Service Logger *zap.Logger + Validator *validator.Validate } func NewMobileController(params mobileControllerParams) *MobileController { return &MobileController{ Handler: base.Handler{ Logger: params.Logger.Named("settings"), + Validator: params.Validator, }, devicesSvc: params.DevicesSvc, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
go.mod(1 hunks)internal/sms-gateway/handlers/3rdparty.go(5 hunks)internal/sms-gateway/handlers/mobile.go(5 hunks)internal/sms-gateway/handlers/module.go(2 hunks)internal/sms-gateway/handlers/settings/3rdparty.go(1 hunks)internal/sms-gateway/handlers/settings/mobile.go(1 hunks)internal/sms-gateway/models/migrations/mysql/20250521225803_add_device_settings.sql(1 hunks)internal/sms-gateway/modules/devices/models.go(1 hunks)internal/sms-gateway/modules/devices/module.go(2 hunks)internal/sms-gateway/modules/devices/repository.go(2 hunks)internal/sms-gateway/modules/devices/service.go(1 hunks)internal/sms-gateway/modules/devices/utils.go(1 hunks)internal/sms-gateway/modules/push/service.go(5 hunks)internal/sms-gateway/modules/webhooks/service.go(2 hunks)pkg/swagger/docs/local.http(1 hunks)pkg/swagger/docs/mobile.http(1 hunks)pkg/swagger/docs/requests.http(1 hunks)pkg/swagger/docs/swagger.json(6 hunks)pkg/swagger/docs/swagger.yaml(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
internal/sms-gateway/handlers/module.go (2)
internal/sms-gateway/handlers/settings/3rdparty.go (1)
NewThirdPartyController(111-119)internal/sms-gateway/handlers/settings/mobile.go (1)
NewMobileController(53-60)
internal/sms-gateway/modules/devices/module.go (1)
internal/sms-gateway/modules/devices/models.go (1)
Migrate(15-17)
internal/sms-gateway/modules/devices/repository.go (1)
internal/sms-gateway/modules/devices/models.go (1)
DeviceSettings(8-13)
internal/sms-gateway/handlers/mobile.go (1)
internal/sms-gateway/handlers/settings/mobile.go (1)
MobileController(23-27)
internal/sms-gateway/handlers/3rdparty.go (1)
internal/sms-gateway/handlers/settings/3rdparty.go (1)
ThirdPartyController(26-30)
internal/sms-gateway/modules/devices/models.go (1)
internal/sms-gateway/models/models.go (1)
User(23-29)
internal/sms-gateway/modules/devices/service.go (3)
internal/sms-gateway/modules/webhooks/service.go (1)
Service(28-37)internal/sms-gateway/modules/auth/service.go (1)
Service(37-49)internal/sms-gateway/modules/devices/models.go (1)
DeviceSettings(8-13)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build / Docker image (linux/arm64)
🔇 Additional comments (33)
pkg/swagger/docs/mobile.http (1)
75-77: Endpoint definition looks good.The new GET endpoint for retrieving device settings follows the established pattern in the file and correctly includes the Bearer token authentication header.
internal/sms-gateway/modules/devices/module.go (1)
35-37: Migration registration looks good.Proper initialization of the database migration for device settings. This ensures the migration runs automatically during application startup.
go.mod (1)
9-9: Consider using a stable release version.You're using a pseudo-version (
v1.5.9-0.20250522134006-6e8b4dd3057a) rather than a stable release. This could potentially introduce unexpected behavior if there are breaking changes.Is this intended to be a temporary solution until a stable v1.5.9 release is available? Consider pinning to a stable release version when it becomes available.
internal/sms-gateway/handlers/module.go (2)
7-7: Settings handler import is correctly added.The import for the new settings package is properly included.
31-32: Controller registration looks good.The registration of both the ThirdPartyController and MobileController for settings follows the existing pattern in the codebase and ensures they're properly integrated into the dependency injection framework.
pkg/swagger/docs/local.http (3)
84-86: LGTM: New GET endpoint for settings retrieval looks good.The added GET endpoint for retrieving device settings is properly defined with the correct authorization header.
89-123: LGTM: Full PATCH endpoint for settings updates is well-structured.This example demonstrates a comprehensive settings update with all available configuration categories. The JSON structure is clear and shows the expected format for each settings category.
126-135: LGTM: Partial PATCH endpoint demonstrates focused updates.This example nicely demonstrates how clients can update only specific settings (webhooks in this case) without affecting other configuration categories, following REST API best practices.
pkg/swagger/docs/requests.http (3)
107-108: LGTM: GET endpoint for 3rdparty settings retrieval looks good.The added GET endpoint for retrieving device settings via the 3rdparty API is properly defined with the correct authorization header.
126-160: LGTM: PUT endpoint for full settings replacement.The PUT endpoint example provides a comprehensive template for replacing all device settings. Using
{{$guid}}for security-sensitive fields like passphrases and signing keys is a good practice in example templates.
111-123:❓ Verification inconclusive
Verify negative value validity for send_interval_max.
The example shows
"send_interval_max": -1- is this a valid value or a special case? Negative intervals are typically not valid for time-based settings.
🏁 Script executed:
#!/bin/bash # Search for validation or documentation of send_interval_max to check if negative values are handled # Look for validation rules echo "Searching for validation rules for send_interval_max..." rg -A 3 -B 3 "send_interval_max" --type go # Look for comments or documentation about this field echo "Searching for documentation about send_interval_max..." rg "send_interval_max" --type md --type txtLength of output: 1319
Validate negative
send_interval_maxvaluesThere’s no evidence in the codebase that negative intervals are handled specially (e.g.,
-1as “unlimited”), nor any server-side validation forsend_interval_max. Please confirm with the API contract whether negative values are allowed, and if not:
- Add or update validation logic to reject negative
send_interval_max.- Document the permitted range (or sentinel values) in your Swagger spec.
Locations to review:
- pkg/swagger/docs/requests.http (lines 111–123)
- Internal handler/model for
/3rdparty/v1/settings(search for wheremessages.send_interval_maxis parsed)internal/sms-gateway/handlers/3rdparty.go (5)
9-9: LGTM: Import for settings package.The settings package is properly imported to support the new settings handler.
25-25: LGTM: SettingsHandler field added to params.The SettingsHandler field is correctly added to the ThirdPartyHandlerParams struct and marked for dependency injection with fx.In.
41-42: LGTM: settingsHandler field added to handler struct.The settingsHandler field is properly added to the thirdPartyHandler struct.
63-64: LGTM: Settings handler registered with router.The settings handler is properly registered with a dedicated route group at "/settings" and will inherit the authentication middleware applied at lines 52-55.
77-78: LGTM: Settings handler initialized in constructor.The settings handler is properly initialized in the newThirdPartyHandler constructor.
internal/sms-gateway/modules/devices/repository.go (2)
10-10: LGTM: Import for GORM clause package.The clause package is correctly imported to support locking and conflict resolution in the settings update method.
107-118: LGTM: Transaction-based settings update with locking.The UpdateSettings method uses proper transaction handling and row-level locking to ensure consistency during the update process. The appendMap function with rules ensures proper merging of settings maps.
internal/sms-gateway/modules/push/service.go (4)
3-9: LGTM: Import of required packagesThe added imports for
errorsanddevicespackage support the new notification functionality.
29-39: LGTM: Updated Params structProper injection of the DevicesSvc dependency through the fx.In struct.
41-56: LGTM: Updated Service structThe Service struct now includes the devicesSvc field to access device information.
87-103: LGTM: Updated constructorThe constructor properly initializes the devicesSvc field from the params.
internal/sms-gateway/modules/devices/service.go (1)
131-138: LGTM: GetSettings implementationThe GetSettings method properly retrieves and filters settings according to public rules.
internal/sms-gateway/modules/devices/utils.go (2)
5-35: LGTM: Rules definitionThe rules map properly defines the schema for device settings with appropriate nesting.
37-63: LGTM: Public rules definitionThe rulesPublic map correctly defines a subset of rules for public access, omitting sensitive fields.
pkg/swagger/docs/swagger.yaml (5)
29-44: LGTM: DeviceSettings definitionWell-structured schema definition for device settings with appropriate references to nested objects.
434-444: LGTM: Updated PushEventType enumAdded SettingsUpdated event type to notify devices when settings change.
491-552: LGTM: Settings component schemasWell-defined schemas for each settings component with appropriate property types and validations.
853-913: LGTM: 3rdparty settings endpointsThe API endpoints for third-party settings management are well-defined with appropriate request/response schemas and security.
1152-1175: LGTM: Mobile settings endpointsThe API endpoint for mobile device settings retrieval is well-defined with appropriate security and response schema.
internal/sms-gateway/handlers/settings/3rdparty.go (1)
105-109: Consistent API pattern implemented correctlyThe Register method properly sets up routes with authentication middleware. Good approach using the WithUser middleware consistently across all endpoints.
pkg/swagger/docs/swagger.json (2)
1120-1142: DeviceSettings schema looks goodThe DeviceSettings schema properly organizes different setting categories into logical groupings, making the structure clean and maintainable.
1665-1666: Good addition of SettingsUpdated event typeAdding the SettingsUpdated event type to the PushEventType enum ensures that mobile devices can be notified when settings are updated, which is essential for the new feature.
Also applies to: 1694-1695
| type DeviceSettings struct { | ||
| UserID string `gorm:"primaryKey;not null;type:varchar(32)"` | ||
| Settings map[string]any `gorm:"not null;type:json;serializer:json"` | ||
|
|
||
| User models.User `gorm:"foreignKey:UserID;constraint:OnDelete:CASCADE"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
GORM JSON mapping & missing timestamps
map[string]any+serializer:jsoncompiles, but GORM’s built-in JSON serializer currently supportsmap[string]interface{}ordatatypes.JSONMap.
Usinganyrelies on the Go 1.18 alias and has caused scan errors with MySQL drivers in the past.- The model has no
CreatedAt/UpdatedAtfields while every other entity embedsTimedModel. This makes it impossible to know when a user last changed settings.
Suggested refactor:
-import (
- "github.com/android-sms-gateway/server/internal/sms-gateway/models"
- "gorm.io/gorm"
-)
+import (
+ "github.com/android-sms-gateway/server/internal/sms-gateway/models"
+ "gorm.io/datatypes"
+ "gorm.io/gorm"
+)
type DeviceSettings struct {
UserID string `gorm:"primaryKey;not null;type:varchar(32)"`
- Settings map[string]any `gorm:"not null;type:json;serializer:json"`
+ Settings datatypes.JSONMap `gorm:"type:json"`
+
+ models.TimedModel // gives CreatedAt / UpdatedAt
User models.User `gorm:"foreignKey:UserID;constraint:OnDelete:CASCADE"`
}Happy to provide the full patch if you’d like.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type DeviceSettings struct { | |
| UserID string `gorm:"primaryKey;not null;type:varchar(32)"` | |
| Settings map[string]any `gorm:"not null;type:json;serializer:json"` | |
| User models.User `gorm:"foreignKey:UserID;constraint:OnDelete:CASCADE"` | |
| } | |
| import ( | |
| "github.com/android-sms-gateway/server/internal/sms-gateway/models" | |
| "gorm.io/datatypes" | |
| "gorm.io/gorm" | |
| ) | |
| type DeviceSettings struct { | |
| UserID string `gorm:"primaryKey;not null;type:varchar(32)"` | |
| Settings datatypes.JSONMap `gorm:"type:json"` | |
| models.TimedModel // adds CreatedAt / UpdatedAt | |
| User models.User `gorm:"foreignKey:UserID;constraint:OnDelete:CASCADE"` | |
| } |
🤖 Prompt for AI Agents
In internal/sms-gateway/modules/devices/models.go around lines 8 to 13, replace
the type of the Settings field from map[string]any to map[string]interface{} to
ensure compatibility with GORM's JSON serializer and avoid scan errors with
MySQL drivers. Additionally, embed the TimedModel struct or add CreatedAt and
UpdatedAt timestamp fields to the DeviceSettings struct to track when settings
are created or updated, maintaining consistency with other entities.
| // notifyDevices asynchronously notifies all the user's devices. | ||
| func (s *Service) notifyDevices(userID string, deviceID *string) { | ||
| logFields := []zap.Field{ | ||
| zap.String("user_id", userID), | ||
| } | ||
| if deviceID != nil { | ||
| logFields = append(logFields, zap.String("device_id", *deviceID)) | ||
| } | ||
|
|
||
| s.logger.Info("Notifying devices", logFields...) | ||
|
|
||
| var filters []devices.SelectFilter | ||
| if deviceID != nil { | ||
| filters = []devices.SelectFilter{devices.WithID(*deviceID)} | ||
| } | ||
|
|
||
| devices, err := s.devicesSvc.Select(userID, filters...) | ||
| if err != nil { | ||
| s.logger.Error("Failed to select devices", append(logFields, zap.Error(err))...) | ||
| return | ||
| } | ||
|
|
||
| if len(devices) == 0 { | ||
| s.logger.Info("No devices found", logFields...) | ||
| return | ||
| } | ||
|
|
||
| for _, device := range devices { | ||
| if device.PushToken == nil { | ||
| s.logger.Info("Device has no push token", zap.String("user_id", userID), zap.String("device_id", device.ID)) | ||
| continue | ||
| go func(userID string, deviceID *string) { | ||
| if err := s.pushSvc.Notify(userID, deviceID, push.NewWebhooksUpdatedEvent()); err != nil { | ||
| s.logger.Error("can't notify devices", zap.Error(err)) | ||
| } | ||
|
|
||
| if err := s.pushSvc.Enqueue(*device.PushToken, push.NewWebhooksUpdatedEvent()); err != nil { | ||
| s.logger.Error("Failed to send push notification", zap.String("user_id", userID), zap.String("device_id", device.ID), zap.Error(err)) | ||
| } | ||
| } | ||
|
|
||
| s.logger.Info("Notified devices", append(logFields, zap.Int("count", len(devices)))...) | ||
| }(userID, deviceID) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential goroutine flood – consider a worker or context propagation
notifyDevices spawns a new goroutine on every webhook change. Under heavy load (bulk imports or script loops) this may create thousands of short-lived goroutines and hammer the DB via pushSvc.Notify.
Suggestions:
-func (s *Service) notifyDevices(userID string, deviceID *string) {
- go func(userID string, deviceID *string) {
- if err := s.pushSvc.Notify(userID, deviceID, push.NewWebhooksUpdatedEvent()); err != nil {
- s.logger.Error("can't notify devices", zap.Error(err))
- }
- }(userID, deviceID)
+func (s *Service) notifyDevices(userID string, deviceID *string) {
+ // pushSvc.Notify is already I/O bound; run synchronously or
+ // hand off to an internal worker pool to limit concurrency.
+ if err := s.pushSvc.Notify(userID, deviceID, push.NewWebhooksUpdatedEvent()); err != nil {
+ s.logger.Error("can't notify devices", zap.Error(err))
+ }
}This keeps the call depth predictable and lets pushSvc decide whether to fan-out.
I can draft a simple worker-pool implementation if required.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // notifyDevices asynchronously notifies all the user's devices. | |
| func (s *Service) notifyDevices(userID string, deviceID *string) { | |
| logFields := []zap.Field{ | |
| zap.String("user_id", userID), | |
| } | |
| if deviceID != nil { | |
| logFields = append(logFields, zap.String("device_id", *deviceID)) | |
| } | |
| s.logger.Info("Notifying devices", logFields...) | |
| var filters []devices.SelectFilter | |
| if deviceID != nil { | |
| filters = []devices.SelectFilter{devices.WithID(*deviceID)} | |
| } | |
| devices, err := s.devicesSvc.Select(userID, filters...) | |
| if err != nil { | |
| s.logger.Error("Failed to select devices", append(logFields, zap.Error(err))...) | |
| return | |
| } | |
| if len(devices) == 0 { | |
| s.logger.Info("No devices found", logFields...) | |
| return | |
| } | |
| for _, device := range devices { | |
| if device.PushToken == nil { | |
| s.logger.Info("Device has no push token", zap.String("user_id", userID), zap.String("device_id", device.ID)) | |
| continue | |
| go func(userID string, deviceID *string) { | |
| if err := s.pushSvc.Notify(userID, deviceID, push.NewWebhooksUpdatedEvent()); err != nil { | |
| s.logger.Error("can't notify devices", zap.Error(err)) | |
| } | |
| if err := s.pushSvc.Enqueue(*device.PushToken, push.NewWebhooksUpdatedEvent()); err != nil { | |
| s.logger.Error("Failed to send push notification", zap.String("user_id", userID), zap.String("device_id", device.ID), zap.Error(err)) | |
| } | |
| } | |
| s.logger.Info("Notified devices", append(logFields, zap.Int("count", len(devices)))...) | |
| }(userID, deviceID) | |
| } | |
| func (s *Service) notifyDevices(userID string, deviceID *string) { | |
| // pushSvc.Notify is already I/O bound; run synchronously or | |
| // hand off to an internal worker pool to limit concurrency. | |
| if err := s.pushSvc.Notify(userID, deviceID, push.NewWebhooksUpdatedEvent()); err != nil { | |
| s.logger.Error("can't notify devices", zap.Error(err)) | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/sms-gateway/modules/webhooks/service.go around lines 119 to 126, the
notifyDevices method spawns a new goroutine for every call, which can lead to
excessive goroutines and overload the pushSvc.Notify under heavy load. Refactor
notifyDevices to use a worker pool or a bounded queue to limit concurrent
goroutines. Implement a context propagation mechanism to manage cancellation and
timeouts properly. This approach will control concurrency, prevent goroutine
flooding, and allow pushSvc to handle fan-out more efficiently.
| func (s *Service) ReplaceSettings(userID string, settings map[string]any) (map[string]any, error) { | ||
| filtered, err := filterMap(settings, rules) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| updated, err := s.devices.ReplaceSettings(&DeviceSettings{ | ||
| UserID: userID, | ||
| Settings: filtered, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return filterMap(updated.Settings, rulesPublic) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider notifying devices after settings replacement
Similar to UpdateSettings, the ReplaceSettings method should notify devices of changes.
Consider adding notification logic after successful settings replacement:
func (s *Service) ReplaceSettings(userID string, settings map[string]any) (map[string]any, error) {
filtered, err := filterMap(settings, rules)
if err != nil {
return nil, err
}
updated, err := s.devices.ReplaceSettings(&DeviceSettings{
UserID: userID,
Settings: filtered,
})
if err != nil {
return nil, err
}
+ // Consider adding code to notify devices about settings changes
+ // This would require injecting the push service or using an event system
+ // Example: pushSvc.Notify(userID, nil, &domain.SettingsUpdatedEvent{})
return filterMap(updated.Settings, rulesPublic)
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *Service) ReplaceSettings(userID string, settings map[string]any) (map[string]any, error) { | |
| filtered, err := filterMap(settings, rules) | |
| if err != nil { | |
| return nil, err | |
| } | |
| updated, err := s.devices.ReplaceSettings(&DeviceSettings{ | |
| UserID: userID, | |
| Settings: filtered, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return filterMap(updated.Settings, rulesPublic) | |
| } | |
| func (s *Service) ReplaceSettings(userID string, settings map[string]any) (map[string]any, error) { | |
| filtered, err := filterMap(settings, rules) | |
| if err != nil { | |
| return nil, err | |
| } | |
| updated, err := s.devices.ReplaceSettings(&DeviceSettings{ | |
| UserID: userID, | |
| Settings: filtered, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Consider adding code to notify devices about settings changes | |
| // This would require injecting the push service or using an event system | |
| // Example: pushSvc.Notify(userID, nil, &domain.SettingsUpdatedEvent{}) | |
| return filterMap(updated.Settings, rulesPublic) | |
| } |
🤖 Prompt for AI Agents
In internal/sms-gateway/modules/devices/service.go around lines 157 to 172, the
ReplaceSettings method updates device settings but does not notify devices of
the changes. To fix this, add logic to notify devices after a successful
settings replacement, similar to the notification implemented in the
UpdateSettings method. Ensure the notification occurs only if the settings
update succeeds and handle any errors appropriately.
| func (s *Service) UpdateSettings(userID string, settings map[string]any) (map[string]any, error) { | ||
| filtered, err := filterMap(settings, rules) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| err = s.devices.UpdateSettings(&DeviceSettings{ | ||
| UserID: userID, | ||
| Settings: filtered, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return s.GetSettings(userID) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider notifying devices after settings update
The UpdateSettings method correctly updates settings, but doesn't notify devices of the changes.
Since a new PushSettingsUpdated event type has been added (as seen in the Swagger docs), consider sending a notification to affected devices:
func (s *Service) UpdateSettings(userID string, settings map[string]any) (map[string]any, error) {
filtered, err := filterMap(settings, rules)
if err != nil {
return nil, err
}
err = s.devices.UpdateSettings(&DeviceSettings{
UserID: userID,
Settings: filtered,
})
if err != nil {
return nil, err
}
+ // Consider adding code to notify devices about settings changes
+ // This would require injecting the push service or using an event system
+ // Example: pushSvc.Notify(userID, nil, &domain.SettingsUpdatedEvent{})
return s.GetSettings(userID)
}This would require dependency injection of the push service, or implementing an event system.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *Service) UpdateSettings(userID string, settings map[string]any) (map[string]any, error) { | |
| filtered, err := filterMap(settings, rules) | |
| if err != nil { | |
| return nil, err | |
| } | |
| err = s.devices.UpdateSettings(&DeviceSettings{ | |
| UserID: userID, | |
| Settings: filtered, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return s.GetSettings(userID) | |
| } | |
| func (s *Service) UpdateSettings(userID string, settings map[string]any) (map[string]any, error) { | |
| filtered, err := filterMap(settings, rules) | |
| if err != nil { | |
| return nil, err | |
| } | |
| err = s.devices.UpdateSettings(&DeviceSettings{ | |
| UserID: userID, | |
| Settings: filtered, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Consider adding code to notify devices about settings changes | |
| // This would require injecting the push service or using an event system | |
| // Example: pushSvc.Notify(userID, nil, &domain.SettingsUpdatedEvent{}) | |
| return s.GetSettings(userID) | |
| } |
🤖 Prompt for AI Agents
In internal/sms-gateway/modules/devices/service.go around lines 140 to 155,
after successfully updating device settings in UpdateSettings, add logic to
notify devices of the changes using the new PushSettingsUpdated event type. To
do this, inject the push notification service into the Service struct or
implement an event system, then call the appropriate method to send the
PushSettingsUpdated notification to affected devices before returning the
updated settings.
Docstrings generation was requested by @capcom6. * #145 (comment) The following files were modified: * `internal/sms-gateway/handlers/3rdparty.go` * `internal/sms-gateway/handlers/mobile.go` * `internal/sms-gateway/handlers/settings/3rdparty.go` * `internal/sms-gateway/handlers/settings/mobile.go` * `internal/sms-gateway/modules/devices/models.go` * `internal/sms-gateway/modules/devices/module.go` * `internal/sms-gateway/modules/devices/repository.go` * `internal/sms-gateway/modules/devices/service.go` * `internal/sms-gateway/modules/devices/utils.go` * `internal/sms-gateway/modules/push/service.go`
|
Note Generated docstrings for this pull request at #146 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/swagger/docs/swagger.yaml (2)
29-56: Add a top-level description for DeviceSettings
TheDeviceSettingsdefinition currently has nodescriptionto explain its overall purpose. Consider adding a concise description and, if certain nested objects are mandatory, arequiredlist to guide consumers.Proposed diff:
+ description: "Aggregated device-specific settings for encryption, gateway, logs, messages, ping, and webhooks." properties: encryption: allOf: - $ref: '#/definitions/smsgateway.SettingsEncryption' description: Encryption contains settings related to message encryption. gateway: allOf: - $ref: '#/definitions/smsgateway.SettingsGateway' description: Gateway contains settings related to the Cloud/Private server configuration. logs: allOf: - $ref: '#/definitions/smsgateway.SettingsLogs' description: Logs contains settings related to logging. messages: allOf: - $ref: '#/definitions/smsgateway.SettingsMessages' description: Messages contains settings related to message handling. ping: allOf: - $ref: '#/definitions/smsgateway.SettingsPing' description: Ping contains settings related to ping functionality. webhooks: allOf: - $ref: '#/definitions/smsgateway.SettingsWebhooks' description: Webhooks contains settings related to webhook functionality.
122-133: Add description to LimitPeriod enum
TheLimitPeriodenum lacks context. A briefdescriptionwould help API consumers understand its role withinSettingsMessages.Proposed diff:
+ description: "Period over which message sending limits apply." enum: - Disabled - PerMinute - PerHour - PerDay type: string x-enum-varnames: - Disabled - PerMinute - PerHour - PerDay
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
go.mod(1 hunks)internal/sms-gateway/handlers/converters/devices_test.go(1 hunks)internal/sms-gateway/handlers/settings/3rdparty.go(1 hunks)internal/sms-gateway/handlers/settings/mobile.go(1 hunks)internal/sms-gateway/models/migrations/mysql/20250521225803_add_device_settings.sql(1 hunks)internal/sms-gateway/models/models.go(4 hunks)internal/sms-gateway/modules/devices/models.go(1 hunks)internal/sms-gateway/modules/devices/repository.go(2 hunks)internal/sms-gateway/modules/devices/utils.go(1 hunks)internal/sms-gateway/modules/push/service.go(5 hunks)internal/sms-gateway/modules/push/types.go(1 hunks)internal/sms-gateway/modules/webhooks/models.go(1 hunks)pkg/swagger/docs/requests.http(1 hunks)pkg/swagger/docs/swagger.json(7 hunks)pkg/swagger/docs/swagger.yaml(7 hunks)
✅ Files skipped from review due to trivial changes (3)
- internal/sms-gateway/modules/push/types.go
- internal/sms-gateway/handlers/converters/devices_test.go
- internal/sms-gateway/models/migrations/mysql/20250521225803_add_device_settings.sql
🚧 Files skipped from review as they are similar to previous changes (8)
- go.mod
- internal/sms-gateway/modules/devices/models.go
- internal/sms-gateway/modules/devices/repository.go
- pkg/swagger/docs/requests.http
- internal/sms-gateway/handlers/settings/mobile.go
- internal/sms-gateway/modules/push/service.go
- internal/sms-gateway/modules/devices/utils.go
- internal/sms-gateway/handlers/settings/3rdparty.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/sms-gateway/modules/webhooks/models.go (1)
internal/sms-gateway/models/models.go (1)
SoftDeletableModel(22-25)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build / Docker image (linux/arm64)
🔇 Additional comments (17)
internal/sms-gateway/models/models.go (5)
17-20: SimplifiedTimedModelstruct by removingDeletedAtfieldThe
TimedModelstruct now only contains timestamp fields for creation and updates, which better aligns with its core responsibility of tracking entity timestamps.
22-25: Good separation of concerns withSoftDeletableModelCreating a separate
SoftDeletableModelthat embedsTimedModelfollows good object-oriented design principles by:
- Separating core timestamp tracking from soft-deletion capability
- Creating a clear inheritance hierarchy
- Making the codebase more maintainable through composition
This change allows models to opt-in to soft deletion capability rather than forcing it on all timed models.
27-33: Updated model inheritance forUserThe
Usermodel now embedsSoftDeletableModelinstead ofTimedModel, maintaining the same fields while gaining explicit soft deletion support.
35-46: Updated model inheritance forDeviceThe
Devicemodel now usesSoftDeletableModelinstead ofTimedModel, maintaining the same functionality while gaining explicit soft deletion support.
56-75: Updated model inheritance forMessageThe
Messagemodel now embedsSoftDeletableModelinstead ofTimedModel, maintaining the same fields while gaining explicit soft deletion support.internal/sms-gateway/modules/webhooks/models.go (2)
22-22: UpdatedWebhookmodel to useSoftDeletableModelThe change from
models.TimedModeltomodels.SoftDeletableModelcorrectly implements soft deletion capability for webhooks, ensuring consistency with other models in the system. This change maintains backward compatibility while adding the ability to mark webhooks as deleted without physically removing them from the database.
25-27:✅ Verification successful
Database migrations need verification
The migration function remains unchanged. Please verify that database migrations will properly handle the schema changes introduced by switching to
SoftDeletableModel, particularly ensuring thedeleted_atcolumn is added to the webhooks table.
🏁 Script executed:
#!/bin/bash # Check if there are any migration files that handle the addition of deleted_at column to webhooks table # Search for migration files in the repository find . -type f -name "*.go" -o -name "*.sql" | grep -i migr | grep -i webhook | xargs grep -l "deleted_at\|DeletedAt" # Check if GORM's AutoMigrate will add the deleted_at column by looking at its behavior grep -r "AutoMigrate" --include="*.go" .Length of output: 564
🏁 Script executed:
#!/bin/bash # Inspect the Webhook struct to verify SoftDelete usage grep -R "type Webhook" -n internal/sms-gateway/modules/webhooks/models.go -A20 # Check for DeletedAt field or SoftDeletableModel embedding in the model grep -R "DeletedAt" -n internal/sms-gateway/modules/webhooks/models.go || echo "No DeletedAt field in Webhook model" # Inspect the SQL migration to see if deleted_at column is added grep -n "deleted_at" internal/sms-gateway/models/migrations/mysql/20240607230753_webhooks.sql || echo "No deleted_at in SQL migration"Length of output: 1305
🏁 Script executed:
#!/bin/bash # Locate SoftDeletableModel definition to confirm it includes DeletedAt grep -R "type SoftDeletableModel" -n internal/sms-gateway/models -A5Length of output: 424
Migrations correctly include
deleted_at
The SQL migration20240607230753_webhooks.sqlalready adds thedeleted_atcolumn, and theSoftDeletableModel(embedded inWebhook) defines theDeletedAtfield. The existingdb.AutoMigrate(&Webhook{})call will also ensure GORM recognizes this column. No further changes are required.pkg/swagger/docs/swagger.yaml (2)
516-616: Nested settings schemas look correct
The newly introducedSettingsEncryption,SettingsGateway,SettingsLogs,SettingsMessages,SettingsPing, andSettingsWebhooksdefinitions are well-structured, with clear descriptions and validation constraints. They align with the device settings management requirements.
617-626: SimSelectionMode enum definition is consistent
TheSimSelectionModeenum correctly lists valid SIM selection strategies and follows the established pattern of other enums.pkg/swagger/docs/swagger.json (8)
396-545: New third-party settings endpoints are well-defined
The/3rdparty/v1/settingsGET, PUT, and PATCH operations correctly reference theDeviceSettingsschema, include proper security requirements, and define expected request/response payloads.
920-957: Mobile settings GET endpoint is correctly specified
The/mobile/v1/settingsGET operation is secured withMobileTokenand returns theDeviceSettingsschema, mirroring the third-party endpoint for device-specific use cases.
1176-1227: DeviceSettings JSON definition matches YAML schema
Thesmsgateway.DeviceSettingsobject is accurately represented with nested properties. If you adopt the YAML suggestion, remember to add a top-leveldescription.
1319-1333: LimitPeriod enum in JSON looks good
The JSON definition forLimitPeriodincludes the full set of values and varnames consistent with the YAML. Consider adding adescriptionas noted above.
1766-1773: Extended PushEventType enum is correct
The addition ofMessagesExportRequestedandSettingsUpdated(with corresponding varnames) aligns with the new push-notification capabilities.
1795-1796: PushNotification event value additions are consistent
Theeventproperty’s enum now includesMessagesExportRequestedandSettingsUpdated, matching thePushEventTypeupdates.
1842-1953: JSON nested settings schemas are accurate
The definitions forSettingsEncryption,SettingsGateway,SettingsLogs,SettingsMessages,SettingsPing, andSettingsWebhookscorrectly mirror the YAML specifications, complete with property descriptions and validation rules.
1955-1966: SimSelectionMode JSON definition matches YAML
TheSimSelectionModeenum in JSON accurately defines valid values and varnames, consistent with the overall schema design.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/sms-gateway/handlers/settings/3rdparty.go (1)
89-120: Swagger documentation properly added, but validation logic needs simplification.The Swagger documentation is now complete and comprehensive, addressing the previous review feedback. However, the same redundant validation issue exists here as in the
putmethod.The validation logic should be simplified similar to the
putmethod suggestion.
🧹 Nitpick comments (4)
internal/sms-gateway/modules/settings/utils.go (1)
5-50: Consider improving rule definition clarity.The use of empty strings as placeholders in the rule maps could be confusing. Consider using a more explicit approach to define the schema.
-var rules = map[string]any{ - "encryption": map[string]any{ - "passphrase": "", - }, +var rules = map[string]any{ + "encryption": map[string]any{ + "passphrase": nil, // or use a specific type marker + },This would make the intent clearer that these are schema definitions rather than default values.
internal/sms-gateway/modules/settings/repository.go (2)
12-20: Clarify behavior when no settings exist for a user.The
Find()method returns an emptyDeviceSettingsstruct when no records are found, rather than an error. This might cause confusion as the caller receives a non-nil result even when no settings exist in the database.Consider using
First()if you expect settings to always exist, or document that an empty struct with zero-valueUserIDindicates no settings found.func (r *repository) GetSettings(userID string) (*DeviceSettings, error) { settings := &DeviceSettings{} - err := r.db.Where("user_id = ?", userID).Limit(1).Find(settings).Error + err := r.db.Where("user_id = ?", userID).First(settings).Error + if err != nil && errors.Is(err, gorm.ErrRecordNotFound) { + return nil, nil // or return a specific "not found" error + } if err != nil { return nil, err } return settings, nil }
39-44: Implementation looks good with minor naming suggestion.The method correctly uses a transaction for the replace operation. Consider using the transaction parameter more explicitly for clarity.
func (r *repository) ReplaceSettings(settings *DeviceSettings) (*DeviceSettings, error) { - err := r.db.Transaction(func(tx *gorm.DB) error { + var err error + err = r.db.Transaction(func(tx *gorm.DB) error { return tx.Save(settings).Error }) return settings, err }internal/sms-gateway/handlers/settings/3rdparty.go (1)
69-87: Simplify redundant validation logic.The method performs validation twice - first with
BodyParserValidatoragainst theDeviceSettingsschema, then withBodyParserinto a generic map. This is redundant and potentially confusing.Consider simplifying to a single parsing step:
func (h *ThirdPartyController) put(user models.User, c *fiber.Ctx) error { - if err := h.BodyParserValidator(c, &smsgateway.DeviceSettings{}); err != nil { - return fiber.NewError(fiber.StatusBadRequest, fmt.Sprintf("Invalid settings format: %v", err)) - } - settings := make(map[string]any, 8) if err := c.BodyParser(&settings); err != nil { return fiber.NewError(fiber.StatusBadRequest, fmt.Sprintf("Failed to parse request body: %v", err)) } + // Optional: Add custom validation for the settings map if needed + updated, err := h.settingsSvc.ReplaceSettings(user.ID, settings)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/settings/3rdparty.go(1 hunks)internal/sms-gateway/handlers/settings/mobile.go(1 hunks)internal/sms-gateway/modules/settings/models.go(1 hunks)internal/sms-gateway/modules/settings/module.go(1 hunks)internal/sms-gateway/modules/settings/repository.go(1 hunks)internal/sms-gateway/modules/settings/service.go(1 hunks)internal/sms-gateway/modules/settings/utils.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/sms-gateway/app.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sms-gateway/handlers/settings/mobile.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
internal/sms-gateway/modules/settings/models.go (1)
internal/sms-gateway/models/models.go (2)
User(27-33)TimedModel(17-20)
internal/sms-gateway/modules/settings/module.go (4)
internal/sms-gateway/modules/devices/module.go (1)
Module(16-32)internal/sms-gateway/app.go (1)
Module(30-48)internal/sms-gateway/modules/settings/service.go (1)
NewService(65-70)internal/sms-gateway/modules/settings/models.go (1)
Migrate(19-24)
internal/sms-gateway/modules/settings/repository.go (1)
internal/sms-gateway/modules/settings/models.go (1)
DeviceSettings(10-17)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build / Docker image (linux/amd64)
- GitHub Check: Build / Docker image (linux/arm64)
- GitHub Check: Analyze (go)
🔇 Additional comments (13)
internal/sms-gateway/modules/settings/module.go (2)
9-21: LGTM! Module structure follows established patterns.The dependency injection setup is well-structured and follows the same pattern as other modules in the codebase (like the devices module). The logger decoration provides proper scoping, and the separation between private repository and public service providers is appropriate.
23-25: Migration registration follows the expected pattern.The migration registration in the
init()function is consistent with how other modules handle database migrations in this codebase.internal/sms-gateway/modules/settings/models.go (2)
10-17: Well-designed model with proper relationships.The
DeviceSettingsstruct is well-structured with:
- Correct primary key and data type consistency with the
Usermodel (varchar(32))- Appropriate use of JSON serialization for flexible settings storage
- Proper foreign key relationship with cascade delete behavior
- Good embedding of
TimedModelfor automatic timestamp management
19-24: Migration function is simple and appropriate.The migration approach using GORM's
AutoMigrateis suitable for this use case, with proper error wrapping for debugging.internal/sms-gateway/modules/settings/utils.go (1)
52-77: Good recursive filtering with proper error handling.The
filterMapfunction correctly handles nested map structures and provides appropriate error messages for type mismatches. The logic properly handles nil values and missing fields.internal/sms-gateway/modules/settings/service.go (4)
8-20: Clean service structure with proper dependency injection.The service struct and dependency injection setup follow good practices with clear separation of concerns.
22-29: GetSettings method is well-implemented.The method correctly retrieves settings from the repository and applies public filtering for external consumption.
48-63: ReplaceSettings implementation is efficient and correct.The method efficiently filters input, replaces settings, and returns the filtered result without unnecessary database calls.
65-70: Constructor follows dependency injection best practices.The
NewServiceconstructor properly initializes the service with scoped logging.internal/sms-gateway/modules/settings/repository.go (1)
46-50: Constructor implementation is correct.Clean and straightforward dependency injection pattern.
internal/sms-gateway/handlers/settings/3rdparty.go (3)
18-33: Well-structured dependency injection setup.The controller properly uses Uber Fx for dependency injection and correctly embeds the base handler for common functionality like logging and validation.
35-53: Get handler implementation is solid.Good Swagger documentation, proper error handling with error wrapping, and clean service interaction.
122-137: Route registration and constructor are well-implemented.Proper HTTP method mapping, consistent use of user authentication middleware, and clean dependency injection in the constructor.
capcom6
force-pushed
the
feature/device-settings
branch
from
f92dfa7 to
4f23d10
Compare
capcom6
force-pushed
the
feature/device-settings
branch
from
960f032 to
a181aab
Compare
Summary by CodeRabbit
New Features
Improvements
Refactor
Bug Fixes
Chores