feat: Add send schedule settings and backend support#840
feat: Add send schedule settings and backend support#840giresse19 wants to merge 3 commits intoNdoleStudio:mainfrom
Conversation
|
|
PR Summary
|
Greptile SummaryThis PR adds a full Send Schedule feature — allowing users to define weekly time windows during which outgoing SMS messages may be sent, with the backend deferring messages created outside those windows to the next allowed slot. It introduces new The architecture follows existing project patterns well. Two notable bugs were introduced:
Additional minor items:
Confidence Score: 2/5Not safe to merge — the GetOutstanding race condition can permanently stall outgoing messages in a 'sending' state under normal usage. Two P1 issues block a clean merge: the message-stuck-in-sending bug is a production reliability issue that occurs on every phone check-in that arrives just after a schedule window closes, and the container.go copy-paste error produces misleading fatal-level migration logs. Both are straightforward to fix but need to be resolved before shipping. api/pkg/services/message_service.go (GetOutstanding schedule check ordering) and api/pkg/di/container.go (wrong %T args on lines 378 and 382). Important Files Changed
Sequence DiagramsequenceDiagram
participant Phone as Phone App
participant Handler as MessageHandler
participant MsgSvc as MessageService
participant SchedSvc as SendScheduleService
participant DB as Database
Phone->>Handler: GET /v1/outstanding
Handler->>MsgSvc: GetOutstanding(params)
MsgSvc->>DB: BEGIN TX — UPDATE messages SET status='sending'<br/>WHERE status IN (pending,scheduled,expired)<br/>AND (scheduled_send_time IS NULL OR <= timestamp)
DB-->>MsgSvc: message (status committed as 'sending')
MsgSvc->>DB: SELECT default send schedule
DB-->>MsgSvc: schedule
MsgSvc->>SchedSvc: IsAllowedAt(schedule, now)
alt Now is INSIDE window
SchedSvc-->>MsgSvc: true
MsgSvc-->>Handler: message
Handler-->>Phone: 200 OK
else Now is OUTSIDE window
SchedSvc-->>MsgSvc: false
Note over MsgSvc,DB: Message already committed as 'sending' — STUCK!
MsgSvc-->>Handler: ErrCodeNotFound
Handler-->>Phone: 404
end
Reviews (1): Last reviewed commit: "feat: Add send schedule settings and bac..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| container.logger.Fatal(stacktrace.Propagate(err, "cannot migrate %T", &entities.Webhook{})) | ||
| } | ||
|
|
||
| if err = db.AutoMigrate(&entities.Integration3CX{}); err != nil { | ||
| container.logger.Fatal(stacktrace.Propagate(err, fmt.Sprintf("cannot migrate %T", &entities.Integration3CX{}))) | ||
| container.logger.Fatal(stacktrace.Propagate(err, "cannot migrate %T", &entities.Discord{})) |
There was a problem hiding this comment.
Lines 378 and 382 reference the wrong entity types in their stacktrace.Propagate error messages, introduced when converting from fmt.Sprintf. When the Discord migration fails the log says cannot migrate *entities.Webhook, and when the Integration3CX migration fails it says cannot migrate *entities.Discord. This makes diagnosing migration failures confusing.
| container.logger.Fatal(stacktrace.Propagate(err, "cannot migrate %T", &entities.Webhook{})) | |
| } | |
| if err = db.AutoMigrate(&entities.Integration3CX{}); err != nil { | |
| container.logger.Fatal(stacktrace.Propagate(err, fmt.Sprintf("cannot migrate %T", &entities.Integration3CX{}))) | |
| container.logger.Fatal(stacktrace.Propagate(err, "cannot migrate %T", &entities.Discord{})) | |
| if err = db.AutoMigrate(&entities.Discord{}); err != nil { | |
| container.logger.Fatal(stacktrace.Propagate(err, "cannot migrate %T", &entities.Discord{})) | |
| } | |
| if err = db.AutoMigrate(&entities.Integration3CX{}); err != nil { | |
| container.logger.Fatal(stacktrace.Propagate(err, "cannot migrate %T", &entities.Integration3CX{})) |
| message, err := service.repository.GetOutstanding(ctx, params.UserID, params.MessageID, params.PhoneNumbers, params.Timestamp) | ||
| if err != nil { | ||
| msg := fmt.Sprintf("could not fetch outstanding messages with params [%s]", spew.Sdump(params)) | ||
| return nil, service.tracer.WrapErrorSpan(span, stacktrace.PropagateWithCode(err, stacktrace.GetCode(err), msg)) | ||
| } | ||
|
|
||
| schedule, scheduleErr := service.sendScheduleService.repository.Default(ctx, params.UserID) | ||
| if scheduleErr == nil && !service.sendScheduleService.IsAllowedAt(schedule, params.Timestamp) { | ||
| msg := fmt.Sprintf("message [%s] is outside the active send schedule", params.MessageID) | ||
| return nil, service.tracer.WrapErrorSpan(span, stacktrace.NewErrorWithCode(repositories.ErrCodeNotFound, msg)) | ||
| } |
There was a problem hiding this comment.
sending status when schedule check fails
The call to service.repository.GetOutstanding at line 69 atomically commits a DB transaction (crdbgorm.ExecuteTx) that changes the message status from pending/scheduled/expired → sending. The schedule check at lines 75–78 runs after that commit. If the current time falls outside the allowed window and IsAllowedAt returns false, the service returns a not-found error — but the message is permanently left in sending status, causing it to never be delivered.
This is a concrete data-integrity issue: any message that arrives at a phone after a send-window closes (e.g., scheduled_send_time IS NULL with an ASAP intent) will get stuck. The SQL-level scheduled_send_time filter already gates future-scheduled messages correctly; the additional Go-level window check needs to be applied before the status is updated (i.e., inside the SQL transaction), or the two checks need to be consolidated so a message is only transitioned to sending when the window check is known to pass.
| if err = db.AutoMigrate(&entities.SendSchedule{}); err != nil { | ||
| container.logger.Fatal(stacktrace.Propagate(err, fmt.Sprintf("cannot migrate %T", &entities.SendSchedule{}))) | ||
| } | ||
|
|
||
| if err = db.AutoMigrate(&entities.SendScheduleWindow{}); err != nil { | ||
| container.logger.Fatal(stacktrace.Propagate(err, fmt.Sprintf("cannot migrate %T", &entities.SendScheduleWindow{}))) |
There was a problem hiding this comment.
fmt.Sprintf in new migration blocks
The rest of this PR converts stacktrace.Propagate(err, fmt.Sprintf(...)) to stacktrace.Propagate(err, "...", arg), but the two newly added SendSchedule migration blocks still use fmt.Sprintf. For consistency:
| if err = db.AutoMigrate(&entities.SendSchedule{}); err != nil { | |
| container.logger.Fatal(stacktrace.Propagate(err, fmt.Sprintf("cannot migrate %T", &entities.SendSchedule{}))) | |
| } | |
| if err = db.AutoMigrate(&entities.SendScheduleWindow{}); err != nil { | |
| container.logger.Fatal(stacktrace.Propagate(err, fmt.Sprintf("cannot migrate %T", &entities.SendScheduleWindow{}))) | |
| if err = db.AutoMigrate(&entities.SendSchedule{}); err != nil { | |
| container.logger.Fatal(stacktrace.Propagate(err, "cannot migrate %T", &entities.SendSchedule{})) | |
| } | |
| if err = db.AutoMigrate(&entities.SendScheduleWindow{}); err != nil { | |
| container.logger.Fatal(stacktrace.Propagate(err, "cannot migrate %T", &entities.SendScheduleWindow{})) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| error?.response?.data?.message || 'Failed to save send schedule', | ||
| }) | ||
| } finally { |
There was a problem hiding this comment.
/default API call when saving a schedule with is_default: true
The Create and Update service methods already call ClearDefault and persist is_default: true as part of the main save. Immediately after, saveSendSchedule also fires POST /v1/send-schedules/{id}/default, causing a second ClearDefault + Update cycle for the same record. This is harmless today but wastes a round-trip and two extra DB writes on every default-schedule save.
Consider removing the extra /default call here, since the is_default flag in the payload is already honoured by the backend:
| error?.response?.data?.message || 'Failed to save send schedule', | |
| }) | |
| } finally { | |
| // is_default is already handled by Create/Update — no extra call needed |
|
|
||
| // GetOutstanding fetches messages that still to be sent to the phone | ||
| func (repository *gormMessageRepository) GetOutstanding(ctx context.Context, userID entities.UserID, messageID uuid.UUID, phoneNumbers []string) (*entities.Message, error) { | ||
| func (repository *gormMessageRepository) GetOutstanding(ctx context.Context, userID entities.UserID, messageID uuid.UUID, phoneNumbers []string, timestamp time.Time) (*entities.Message, error) { |
There was a problem hiding this comment.
The scheduling is not done when we fetch the outstanding message. The schedule is created here
httpsms/api/pkg/repositories/gorm_phone_notification_repository.go
Lines 69 to 109 in ec1da99
This is where we schedule when a notification will be sent to the phone to send an outgoing message that is where we need to handle the schduelding logic.
The way the flow works,
When the API receives a new request, it adds the outgoing message in the queue.
The message will be fetched from the queu automatically and we'll figure out when to send the messagae based on the send rate logic in phone_notification_service. This process has locks to ensure that even if multiple messages are sent at once, we will process them sequentially.
| Timezone string `json:"timezone"` | ||
| IsDefault bool `json:"is_default" gorm:"default:false"` | ||
| IsActive bool `json:"is_active" gorm:"default:true"` | ||
| Windows []SendScheduleWindow `json:"windows" gorm:"constraint:OnDelete:CASCADE;foreignKey:ScheduleID"` |
There was a problem hiding this comment.
serialize this as JSONb instead of creating a new sql table if possible, it may amke the query much more complex though https://gorm.io/docs/serializer.html
| router.Get("/v1/send-schedules/:scheduleID", h.computeRoute(middlewares, h.Show)...) | ||
| router.Put("/v1/send-schedules/:scheduleID", h.computeRoute(middlewares, h.Update)...) | ||
| router.Delete("/v1/send-schedules/:scheduleID", h.computeRoute(middlewares, h.Delete)...) | ||
| router.Post("/v1/send-schedules/:scheduleID/default", h.computeRoute(middlewares, h.SetDefault)...) |
There was a problem hiding this comment.
There will be no concept of a default schedule. We should be able to attach a schedule to a phone entity in a 1:1 relationshonsip meaning a phone can have only1 schedule. You can add a nullable schedule_id on the phone entity.
| @@ -1,3 +1,28 @@ | |||
|
|
|||
| export interface EntitiesSendScheduleWindow { | |||
| </v-btn> | ||
| </div> | ||
| </div> | ||
| <v-dialog v-model="showSendScheduleDialog" :fullscreen="$vuetify.breakpoint.smAndDown" max-width="900"> |
There was a problem hiding this comment.
Let's have a separate page for managing schedules with a back button like the settings page.
| Send Schedule | ||
| </h5> | ||
| <p class="text--secondary"> | ||
| Define when outgoing SMS messages are allowed to be sent. When a |
There was a problem hiding this comment.
messages will ALWAYS be sent according to the avaialbiltiy schdule and also respoecting the sending rate https://docs.httpsms.com/features/control-sms-send-rate
|
|
||
| if err != nil { | ||
| l.Error(stacktrace.Propagate(err, msg)) | ||
| l.Error(stacktrace.Propagate(err, "%s", msg)) |
There was a problem hiding this comment.
l.Error(stacktrace.Propagate(err, msg))
| ctxLogger := service.tracer.CtxLogger(service.logger, span) | ||
|
|
||
| sendAttempts, sim := service.phoneSettings(ctx, params.UserID, phonenumbers.Format(params.Owner, phonenumbers.E164)) | ||
| resolvedSendAt, err := service.sendScheduleService.ResolveScheduledSendTime(ctx, params.UserID, params.SendAt) |
There was a problem hiding this comment.
The send schedule is determined on the notification flow. See the other comment.
There was a problem hiding this comment.
Why are you deleting this file?
There was a problem hiding this comment.
use pnpm instead of yarn
You may need this command on windows $env:NODE_OPTIONS = "--openssl-legacy-provider"
| return &gormSendScheduleRepository{logger: logger.WithService(fmt.Sprintf("%T", &gormSendScheduleRepository{})), tracer: tracer, db: db} | ||
| } | ||
|
|
||
| func (r *gormSendScheduleRepository) Store(ctx context.Context, schedule *entities.SendSchedule) error { |
There was a problem hiding this comment.
Add swag,go documentation on all the endpoints. https://github.com/swaggo/swag
3 participants
Adds Send Schedule support across frontend and backend.