feat: send schedules based on review feedback#841
feat: send schedules based on review feedback#841giresse19 wants to merge 4 commits intoNdoleStudio:mainfrom
Conversation
|
|
Greptile SummaryThis PR introduces a Send Schedules feature that lets users define weekly availability windows (day-of-week + minute ranges) and attach one schedule to each phone. Outgoing message notifications respect both the configured send rate and the selected schedule, delaying delivery to the next open window when necessary. Key additions:
Issues found:
Confidence Score: 4/5Safe to merge after addressing the ownership validation gap and the end_minute/UI mismatch; no data loss or security breach in the primary path Three P2 findings remain: dead code (
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant PhoneNotificationService
participant SendScheduleRepository
participant PhoneNotificationRepository
Client->>PhoneNotificationService: Schedule(params)
PhoneNotificationService->>PhoneNotificationService: Load phone by ID
alt phone.ScheduleID != nil
PhoneNotificationService->>SendScheduleRepository: Load(userID, scheduleID)
SendScheduleRepository-->>PhoneNotificationService: schedule / 404
Note over PhoneNotificationService: If 404, schedule = nil
end
PhoneNotificationService->>PhoneNotificationRepository: Schedule(messagesPerMinute, schedule, notification)
alt messagesPerMinute == 0
PhoneNotificationRepository->>PhoneNotificationRepository: resolveScheduledAt(now, schedule)
PhoneNotificationRepository-->>PhoneNotificationService: insert notification
else messagesPerMinute > 0
PhoneNotificationRepository->>PhoneNotificationRepository: load last notification (tx lock)
PhoneNotificationRepository->>PhoneNotificationRepository: resolveScheduledAt(now, schedule)
PhoneNotificationRepository->>PhoneNotificationRepository: rateLimitedAt = lastScheduledAt + 60/rate
PhoneNotificationRepository->>PhoneNotificationRepository: resolveScheduledAt(max(now, rateLimitedAt), schedule)
PhoneNotificationRepository-->>PhoneNotificationService: insert notification with scheduled time
end
Reviews (2): Last reviewed commit: "feat: Refactor send schedules based on r..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return nil | ||
| } | ||
|
|
||
| func (repository *gormPhoneNotificationRepository) resolveScheduledAt(current time.Time, schedule *entities.SendSchedule) time.Time { | ||
| if schedule == nil || !schedule.IsActive || len(schedule.Windows) == 0 { | ||
| return current.UTC() | ||
| } | ||
|
|
||
| location, err := time.LoadLocation(schedule.Timezone) | ||
| if err != nil { | ||
| return current.UTC() | ||
| } | ||
|
|
||
| base := current.In(location) | ||
| var best time.Time | ||
| for dayOffset := 0; dayOffset <= 7; dayOffset++ { | ||
| day := base.AddDate(0, 0, dayOffset) | ||
| weekday := int(day.Weekday()) | ||
| for _, window := range schedule.Windows { | ||
| if window.DayOfWeek != weekday { | ||
| continue | ||
| } | ||
|
|
||
| start := time.Date(day.Year(), day.Month(), day.Day(), 0, 0, 0, 0, location).Add(time.Duration(window.StartMinute) * time.Minute) | ||
| end := time.Date(day.Year(), day.Month(), day.Day(), 0, 0, 0, 0, location).Add(time.Duration(window.EndMinute) * time.Minute) | ||
|
|
||
| var candidate time.Time | ||
| switch { | ||
| case dayOffset == 0 && base.Before(start): | ||
| candidate = start | ||
| case dayOffset == 0 && (base.Equal(start) || (base.After(start) && base.Before(end))): | ||
| candidate = base | ||
| case dayOffset > 0: | ||
| candidate = start | ||
| default: | ||
| continue | ||
| } | ||
|
|
||
| if best.IsZero() || candidate.Before(best) { | ||
| best = candidate | ||
| } | ||
| } | ||
| if !best.IsZero() { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if best.IsZero() { | ||
| return current.UTC() | ||
| } | ||
| return best.UTC() | ||
| } | ||
|
|
||
| func (repository *gormPhoneNotificationRepository) maxTime(a, b time.Time) time.Time { | ||
| if a.Unix() > b.Unix() { | ||
| return a |
There was a problem hiding this comment.
resolveScheduledAt here is an exact copy of the algorithm already implemented as ResolveScheduledSendTime on SendScheduleService (introduced in send_schedule_service.go). Maintaining two independent copies of the same scheduling logic will inevitably lead to subtle divergences.
The service method returns (time.Time, error) and already handles the timezone-error path, so the notification service (or even the repository, if it held a reference to the service) could delegate to that single implementation instead of duplicating it here.
| input.Name = strings.TrimSpace(input.Name) | ||
| input.Timezone = strings.TrimSpace(input.Timezone) | ||
| windows := make([]SendScheduleWindow, 0, len(input.Windows)) | ||
| for _, item := range input.Windows { | ||
| windows = append(windows, SendScheduleWindow{DayOfWeek: item.DayOfWeek, StartMinute: item.StartMinute, EndMinute: item.EndMinute}) | ||
| } | ||
| sort.SliceStable(windows, func(i, j int) bool { | ||
| if windows[i].DayOfWeek == windows[j].DayOfWeek { | ||
| return windows[i].StartMinute < windows[j].StartMinute | ||
| } | ||
| return windows[i].DayOfWeek < windows[j].DayOfWeek | ||
| }) | ||
| input.Windows = windows | ||
| return *input | ||
| } | ||
|
|
||
| func (input *SendScheduleStore) ToParams(user entities.AuthContext) *services.SendScheduleUpsertParams { | ||
| windows := make([]entities.SendScheduleWindow, 0, len(input.Windows)) | ||
| for _, item := range input.Windows { |
There was a problem hiding this comment.
Sanitize() here sorts windows by DayOfWeek / StartMinute, and then sanitizeWindows() in send_schedule_service.go sorts the slice again after converting it to []entities.SendScheduleWindow. One of the two sorts is redundant and can be removed.
…le-review-fixes # Conflicts: # web/pages/settings/index.vue
|
|
||
| async loadSendSchedules() { | ||
| try { | ||
| const response = await axios.get('/v1/send-schedules') |
There was a problem hiding this comment.
Use the state/stoore to perfom axios requests instead of calling axios directly e.g
Lines 1131 to 1154 in 14b718e
| missed_call_auto_reply: phone.missed_call_auto_reply, | ||
| max_send_attempts: parseInt(phone.max_send_attempts.toString()), | ||
| messages_per_minute: parseInt(phone.messages_per_minute.toString()), | ||
| schedule_id: (phone as any).schedule_id || null, |
There was a problem hiding this comment.
Let's use the typescript type for the phone entity instead of any
| async deleteSchedule() { | ||
| this.saving = true | ||
| try { | ||
| await axios.delete(`/v1/send-schedules/${this.activeSchedule.id}`) |
There was a problem hiding this comment.
Use axios state/store as discussed in the other comment.
| } | ||
|
|
||
| // SendSchedule controls when a phone is allowed to send outgoing SMS messages. | ||
| type SendSchedule struct { |
There was a problem hiding this comment.
Let's call the entity MessageSendSchedule
| </v-dialog> | ||
| </div> | ||
|
|
||
| <h5 class="text-h4 mb-3 mt-12">Send Schedules</h5> |
There was a problem hiding this comment.
On this page, display all the existing scheudles in a table. But instead of having a "manage send schedules" button, you should have a button called "AddSendSchedule" which takes you to the other page /settings/send-schedules
Similaly we should have a button to "Edit" the send schedule which takes you to the other page with the items pre-filled.
The problem with using v-dialog is that we're not using the "full width" of the screen and on mobile phones every horizontal space counts.
|
|
||
| // SendScheduleRepository loads and persists entities.SendSchedule. | ||
| type SendScheduleRepository interface { | ||
| Store(ctx context.Context, schedule *entities.SendSchedule) error |
There was a problem hiding this comment.
Seup pre-commit hooks it will detect that missing docblocks so you add them before you commit. https://pre-commit.com/
| return result | ||
| } | ||
|
|
||
| func (validator *SendScheduleHandlerValidator) validateWindows(result url.Values, windows []requests.SendScheduleWindow) { |
There was a problem hiding this comment.
Set a reasonable max number of windows per day e.g only 6
| openCreateDialog() { | ||
| this.activeSchedule = { | ||
| id: null, | ||
| name: 'Business Hours', |
There was a problem hiding this comment.
The name should be empty by default, You can add it as a placeholder in the component
| if err != nil { | ||
| return h.responseBadRequest(c, err) | ||
| } | ||
| if err = h.service.Delete(ctx, h.userIDFomContext(c), scheduleID); err != nil { |
There was a problem hiding this comment.
Respond with 404 if the schedule doesn't exist.
| var schedule *entities.SendSchedule | ||
| if phone.ScheduleID != nil { | ||
| schedule, err = service.sendScheduleRepository.Load(ctx, params.UserID, *phone.ScheduleID) | ||
| if err != nil && stacktrace.GetCode(err) != repositories.ErrCodeNotFound { |
There was a problem hiding this comment.
You an reverse the order to make it clerer
if stacktrace.GetCode(err) == repositories.ErrCodeNotFound {
schedule = nil
}
if err != nil {
msg := fmt.Sprintf("cannot load send schedule [%s] for phone [%s]", *phone.ScheduleID, phone.ID)
return service.tracer.WrapErrorSpan(span, stacktrace.Propagate(err, msg))
}3 participants
No description provided.