Repository navigation
Upmerge v9.0.1 from nyaruka/mailroom - #704
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Automated upmerge of nyaruka/mailroom v9.0.1 into the AGPL mirror, including substantial internal refactors (cron abstraction, DB access types), logging migration to slog, and removal of several ticket-service implementations/tests.
Changes:
- Introduces a
core/taskscron interface + sharedCronNextscheduling helper; updates multiple cron jobs to return structured results. - Refactors many model loaders and handlers (types, DB interfaces, logging with
slog, move toany, new opt-in assets support). - Updates build/runtime dependencies and tooling (Go 1.21, PostGIS 15 in CI, Dockerfile docs fetch).
Reviewed changes
Copilot reviewed 226 out of 301 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| services/tickets/rocketchat/web_test.go | Removes RocketChat ticketing web test as part of ticketing service removals |
| services/tickets/rocketchat/web.go | Removes RocketChat webhook handler and route registration |
| services/tickets/rocketchat/testdata/TestOpenAndForward_open_ticket.snap | Removes RocketChat snapshot fixture |
| services/tickets/rocketchat/testdata/TestOpenAndForward_forward_message.snap | Removes RocketChat snapshot fixture |
| services/tickets/rocketchat/testdata/TestCloseAndReopen_close_tickets.snap | Removes RocketChat snapshot fixture |
| services/tickets/rocketchat/service.go | Removes RocketChat ticket service implementation |
| services/tickets/rocketchat/client_test.go | Removes RocketChat client tests |
| services/tickets/rocketchat/client.go | Removes RocketChat client implementation |
| services/tickets/mailgun/web_test.go | Removes Mailgun ticketing web test |
| services/tickets/mailgun/testdata/TestSendMessage_mailgun_request.snap | Removes Mailgun snapshot fixture |
| services/tickets/mailgun/testdata/TestOpenAndForward_open_ticket.snap | Removes Mailgun snapshot fixture |
| services/tickets/mailgun/testdata/TestOpenAndForward_forward_message.snap | Removes Mailgun snapshot fixture |
| services/tickets/mailgun/testdata/TestCloseAndReopen_reopen_tickets.snap | Removes Mailgun snapshot fixture |
| services/tickets/mailgun/testdata/TestCloseAndReopen_close_tickets.snap | Removes Mailgun snapshot fixture |
| services/tickets/mailgun/client_test.go | Removes Mailgun client tests |
| services/tickets/mailgun/client.go | Removes Mailgun client implementation |
| services/tickets/intern/service_test.go | Removes internal ticket service tests |
| services/tickets/intern/service.go | Removes internal ticket service implementation |
| services/ivr/vonage/service_test.go | Updates tests to use any for decoded JSON |
| services/ivr/twiml/client.go | Switches TwiML XML inner payload type to any |
| runtime/runtime.go | Changes ReadonlyDB type from *sqlx.DB to *sql.DB |
| runtime/config_test.go | Removes invalid IP/network test cases for disallowed networks parsing |
| runtime/config.go | Replaces custom network parsing with httpx.ParseNetworks |
| goreleaser.yml | Restructures GoReleaser config keys (schema change) |
| core/tasks/timeouts/cron_test.go | Updates timeout cron test to new cron interface/result |
| core/tasks/timeouts/cron.go | Refactors timeouts cron into tasks.Cron implementation |
| core/tasks/starts/start_flow_test.go | Updates flow start construction signature |
| core/tasks/starts/start_flow.go | Refactors start batching (priority changes, flow type handling, slog logging, i18n language) |
| core/tasks/msgs/send_broadcast_batch.go | Switches to slog and queues messages instead of sending directly |
| core/tasks/msgs/send_broadcast.go | Switches error logging to slog |
| core/tasks/msgs/retry_errored_messages_test.go | Updates retry cron test to new cron interface/result and new msg struct usage |
| core/tasks/msgs/retry_errored_messages.go | Refactors retry cron into tasks.Cron and queues messages |
| core/tasks/ivr/start_ivr_flow_batch_test.go | Updates flow start signature and IVR service interface |
| core/tasks/ivr/start_ivr_flow_batch.go | Migrates IVR batch logging to slog |
| core/tasks/ivr/cron_test.go | Updates IVR retry cron test to new cron interface/result |
| core/tasks/ivr/cron.go | Refactors IVR retry cron into tasks.Cron and migrates to slog |
| core/tasks/interrupts/interrupt_channel_test.go | Updates retry invocation to new cron interface |
| core/tasks/interrupts/interrupt_channel.go | Fixes DB handle usage to match new DB abstractions |
| core/tasks/incidents/end_incidents_test.go | Updates incidents cron test to new cron interface/result |
| core/tasks/incidents/end_incidents.go | Refactors incidents cron into tasks.Cron and adds ended count result |
| core/tasks/handler/cron_test.go | Updates handler retry cron test to new cron interface/result and reset flags |
| core/tasks/handler/cron.go | Refactors handler retry cron into tasks.Cron and migrates to slog |
| core/tasks/cron_test.go | Adds unit tests for tasks.CronNext |
| core/tasks/cron.go | Introduces tasks.Cron interface and CronNext helper |
| core/tasks/contacts/populate_dynamic_group.go | Migrates smart group task logging to slog |
| core/tasks/contacts/import_contact_batch_test.go | Updates assert maps to map[string]any |
| core/tasks/campaigns/fire_campaign_event.go | Migrates campaign event worker logging to slog and updates marker API |
| core/tasks/analytics/cron.go | Refactors analytics cron into tasks.Cron and returns stats map |
| core/search/search.go | Migrates search debug logging to slog |
| core/search/queries.go | Updates locale formatting call for new locale type |
| core/search/groups.go | Migrates group sleep logging to slog |
| core/runner/runner_test.go | Updates flow start batching and language types (i18n) |
| core/queue/queue_test.go | Moves queue tests to external package and updates API calls |
| core/queue/queue.go | Reorders priority constants and updates AddTask signature to any |
| core/msgio/send_test.go | Renames send test to queue test; updates channel setup and queueing assertions |
| core/msgio/android_test.go | Refactors Android sync test to per-channel function and changes mock auth status |
| core/msgio/android.go | Replaces bulk Android sync with SyncAndroidChannel returning errors; uses slog |
| core/models/webhook_event_test.go | Adds cleanup to webhook event test |
| core/models/webhook_event.go | Updates DB interface type and uses any slice |
| core/models/utils_test.go | Adjusts test cleanup and uses []any |
| core/models/users.go | Refactors user asset loading to *sql.DB + JSON row scanning helper; updates null lib |
| core/models/topics.go | Refactors topic asset loading to *sql.DB + JSON row scanning helper; updates null lib |
| core/models/tokens.go | Updates DB interface type |
| core/models/ticket_events_test.go | Updates ticket insert helper signature and null lib version |
| core/models/ticket_events.go | Updates DB interface type and uses any slice |
| core/models/templates_test.go | Updates locale types and template expectations; adds template lookup asserts |
| core/models/teams.go | Updates null lib version |
| core/models/starts_test.go | Updates flow start signature and batch creation signature |
| core/models/starts.go | Removes FlowType from FlowStart, updates DB interfaces, changes CreateBatch signature |
| core/models/runs.go | Updates null lib version |
| core/models/resthooks.go | Refactors resthook loading to *sql.DB + JSON row scanning helper |
| core/models/orgs_test.go | Refactors org loading to use *sql.DB and new environment accessors |
| core/models/optins_test.go | Adds tests for opt-in asset loading |
| core/models/optins.go | Adds opt-in model + loader using *sql.DB + JSON row scanning helper |
| core/models/notifications_test.go | Updates ticket insert helper signature |
| core/models/notifications.go | Adds medium field, typed EmailStatus, updates insert SQL and DB interfaces |
| core/models/locations_test.go | Updates location name lookup to pass environment |
| core/models/locations.go | Refactors location loading to use *sql.DB and env-aware hierarchy parsing; uses slog |
| core/models/labels.go | Refactors label loading to *sql.DB + JSON row scanning helper |
| core/models/incident.go | Updates DB interfaces and null lib version; changes open incidents query signature |
| core/models/imports_test.go | Uses dbutil helper to scan IDs from *sql.Rows |
| core/models/http_logs_test.go | Removes HTTPLogger test coverage for ticketer-called logs |
| core/models/http_logs.go | Removes ticketer log type and HTTPLogger; updates insert SQL and DB interfaces |
| core/models/groups_test.go | Refactors group loading test to use org assets instead of mock DB |
| core/models/globals.go | Refactors globals loading to *sql.DB + JSON row scanning helper |
| core/models/flows_test.go | Updates flow loaders to accept *sql.DB |
| core/models/flows.go | Refactors flow loading to *sql.DB, migrates to slog, updates null map type, adjusts revision selection SQL |
| core/models/flow_stats_test.go | Updates redis key assertions API usage |
| core/models/fields.go | Refactors field loading to *sql.DB + JSON row scanning helper (signature change) |
| core/models/counts.go | Updates DB interface type |
| core/models/channels_test.go | Updates channel expectations (Facebook channel + features; removes parent checks) |
| core/models/channel_event_test.go | Updates channel event API with opt-in ID and adds extra string helper checks |
| core/models/assets_test.go | Updates expected flow definition prefix for clone |
| core/models/airtime_test.go | Updates assert map types to map[string]any |
| core/models/airtime.go | Updates DB interface type and uses any slice |
| core/hooks/update_campaign_events.go | Updates hook signature to []any and migrates debug logging to slog |
| core/hooks/unsubscribe_resthook.go | Updates hook signature to []any |
| core/hooks/start_start.go | Updates hook signature to []any |
| core/hooks/start_broadcasts.go | Updates hook signature to []any |
| core/hooks/send_messages.go | Updates hook signature to []any and queues messages |
| core/hooks/monitor_webhooks.go | Updates hook signature to []any |
| core/hooks/insert_webhook_event.go | Updates hook signature to []any |
| core/hooks/insert_tickets.go | Updates hook signature to []any |
| core/hooks/insert_start.go | Updates hook signature to []any and flow start creation signature |
| core/hooks/insert_http_logs.go | Updates hook signature to []any |
| core/hooks/insert_airtime_transfers.go | Updates hook signature to []any |
| core/hooks/contact_modified.go | Updates hook signature to []any |
| core/hooks/contact_last_seen.go | Updates hook signature to []any |
| core/hooks/commit_urn_changes.go | Updates hook signature to []any |
| core/hooks/commit_status_changes.go | Updates hook signature to []any |
| core/hooks/commit_name_changes.go | Updates hook signature to []any and null lib version |
| core/hooks/commit_messages.go | Updates hook signature to []any |
| core/hooks/commit_language_changes.go | Updates hook signature to []any and null lib version |
| core/hooks/commit_ivr.go | Updates hook signature to []any |
| core/hooks/commit_group_changes.go | Updates hook signature to []any |
| core/hooks/commit_flow_changes.go | Updates hook signature to []any |
| core/hooks/commit_field_changes.go | Updates hook signature, uses any collections, migrates debug logging to slog |
| core/hooks/commit_added_labels.go | Updates hook signature to []any |
| core/handlers/webhook_called_test.go | Updates SQL assertion arg types and contact load signature |
| core/handlers/webhook_called.go | Migrates debug logging to slog |
| core/handlers/ticket_opened_test.go | Removes ticket service imports & ticketer assertions; updates SQL assertions/args |
| core/handlers/ticket_opened.go | Removes ticketer lookup usage and migrates logging to slog |
| core/handlers/sprint_ended.go | Adds slog debug log for sprint end |
| core/handlers/session_triggered_test.go | Updates SQL assertion arg types |
| core/handlers/session_triggered.go | Migrates debug logging to slog |
| core/handlers/service_called_test.go | Updates SQL assertion arg types |
| core/handlers/service_called.go | Migrates logging to slog; removes ticketer HTTP log handling |
| core/handlers/resthook_called.go | Migrates logging to slog and changes missing resthook logging level/message |
| core/handlers/optin_requested_test.go | Adds opt-in requested handler test and queue assertions |
| core/handlers/optin_requested.go | Adds handler for opt-in requested events |
| core/handlers/noop.go | Import ordering cleanup |
| core/handlers/msg_received_test.go | Updates to use .FlowMsg and []any args |
| core/handlers/msg_received.go | Migrates logging to slog |
| core/handlers/msg_created.go | Migrates logging to slog; passes template to outgoing msg creation |
| core/handlers/ivr_created.go | Migrates logging to slog |
| core/handlers/input_labels_added_test.go | Updates to use .FlowMsg, new msg IDs, []any args |
| core/handlers/input_labels_added.go | Migrates logging to slog |
| core/handlers/flow_entered_test.go | Updates SQL assertion arg types |
| core/handlers/flow_entered.go | Migrates debug logging to slog |
| core/handlers/email_sent.go | Migrates email sent logging to slog |
| core/handlers/contact_urns_changed_test.go | Updates contact URN insert helper signature and SQL arg types |
| core/handlers/contact_urns_changed.go | Migrates logging to slog |
| core/handlers/contact_status_changed_test.go | Updates SQL assertion arg types |
| core/handlers/contact_status_changed.go | Migrates logging to slog |
| core/handlers/contact_name_changed_test.go | Updates SQL assertion arg types |
| core/handlers/contact_name_changed.go | Migrates logging to slog |
| core/handlers/contact_language_changed_test.go | Updates SQL assertion arg types |
| core/handlers/contact_language_changed.go | Migrates logging to slog |
| core/handlers/contact_groups_changed_test.go | Updates SQL assertion arg types |
| core/handlers/contact_groups_changed.go | Migrates logging to slog and improves warning fields |
| core/handlers/contact_field_changed_test.go | Updates SQL assertion arg types |
| core/handlers/contact_field_changed.go | Migrates logging to slog |
| core/handlers/campaigns_test.go | Updates SQL assertion arg types |
| core/handlers/broadcast_created_test.go | Updates SQL assertion arg types |
| core/handlers/broadcast_created.go | Migrates logging to slog |
| core/handlers/base_test.go | Updates SQL assertion arg types and engine/modifiers invocation |
| core/handlers/airtime_transferred_test.go | Updates SQL assertion arg types |
| core/handlers/airtime_transferred.go | Migrates logging to slog |
| core/goflow/modifiers_test.go | Import ordering cleanup |
| core/goflow/modifiers.go | Import spacing cleanup |
| core/goflow/flows_test.go | Updates language type to i18n and organizes imports |
| core/goflow/flows.go | Import ordering cleanup |
| core/goflow/engine_test.go | Removes simulator ticket test and related imports |
| core/goflow/engine.go | Removes ticket factory/service; configures max field/result chars |
| cmd/test-smtp/main.go | Migrates to slog and adjusts fatal error handling |
| Dockerfile | Updates Go version and fetches goflow docs at build time |
| .github/workflows/ci.yml | Updates Go version and PostGIS version used by CI |
Comments suppressed due to low confidence (1)
core/tasks/analytics/cron.go:1
redis_wait_countis returningdbWaitCountInPeriodinstead ofredisWaitCountInPeriod, which will report incorrect Redis wait counts.
package analytics
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+26
to
+36
| func CronNext(last time.Time, interval time.Duration) time.Time { | ||
| if interval >= time.Second && interval < time.Minute { | ||
| return last.Add(interval - ((time.Duration(last.Second()) * time.Second) % interval)) | ||
| } else if interval == time.Minute { | ||
| seconds := time.Duration(60-last.Second()) + 1 | ||
| return last.Add(seconds * time.Second) | ||
| } else { | ||
| // no special treatment for other things | ||
| return last.Add(interval) | ||
| } | ||
| } |
There was a problem hiding this comment.
The sub-minute scheduling math is incorrect because it only uses last.Second() and ignores the current sub-second offset; e.g. for 150ms intervals it can schedule earlier than expected within the same second. Consider computing the next fire based on last.UnixNano() modulo interval (or using last.Truncate(interval).Add(interval) with care for exact alignment) so that nanoseconds are accounted for.
Comment on lines
38
to
50
| throttledChannels := make(map[models.ChannelID]bool) | ||
| clogs := make([]*models.ChannelLog, 0, len(calls)) | ||
|
|
||
| // schedules requests for each call | ||
| for _, call := range calls { | ||
| log = log.WithField("call_id", call.ID()) | ||
| log = log.With("call_id", call.ID()) | ||
|
|
||
| // if the channel for this call is throttled, move on | ||
| if throttledChannels[call.ChannelID()] { | ||
| call.MarkThrottled(ctx, rt.DB, time.Now()) | ||
| log.WithField("channel_id", call.ChannelID()).Info("skipping call, throttled") | ||
| log.Info("skipping call, throttled", "channel_id", call.ChannelID()) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Reassigning log = log.With(\"call_id\", ...) inside the loop can cause attributes to accumulate across iterations with slog (duplicate call_id keys across logs). Prefer creating a per-iteration logger (e.g. callLog := log.With(...)) and use callLog for the rest of the loop body.
| err = smtpx.Send(client, m, nil) | ||
| if err != nil { | ||
| logrus.WithError(err).Fatal("error sending email") | ||
| slog.Error("error sending email", "error", err) |
There was a problem hiding this comment.
On send failure the program logs the error but continues and exits with status 0 (and still logs "email sent"). Previously this path was fatal. Consider returning early and exiting non-zero when smtpx.Send fails.
Suggested change
| slog.Error("error sending email", "error", err) | |
| slog.Error("error sending email", "error", err) | |
| os.Exit(1) |
Comment on lines
+15
to
+16
| RUN grep goflow go.mod | cut -d" " -f2 | cut -c2- > /tmp/goflow_version | ||
| RUN curl -L "https://github.com/nyaruka/goflow/releases/download/v`cat /tmp/goflow_version`/docs.tar.gz" | tar zxv |
There was a problem hiding this comment.
Fetching and extracting a remote tarball at build time without integrity verification makes builds non-reproducible and increases supply-chain risk. Consider pinning a checksum/signature verification step (or vendoring the docs artifact) before extracting.
Suggested change
| RUN grep goflow go.mod | cut -d" " -f2 | cut -c2- > /tmp/goflow_version | |
| RUN curl -L "https://github.com/nyaruka/goflow/releases/download/v`cat /tmp/goflow_version`/docs.tar.gz" | tar zxv | |
| ARG GOFLOW_DOCS_SHA256 | |
| RUN grep goflow go.mod | cut -d" " -f2 | cut -c2- > /tmp/goflow_version | |
| RUN version="$(cat /tmp/goflow_version)" \ | |
| && curl -fL "https://github.com/nyaruka/goflow/releases/download/v${version}/docs.tar.gz" -o /tmp/docs.tar.gz \ | |
| && echo "${GOFLOW_DOCS_SHA256} /tmp/docs.tar.gz" | sha256sum -c - \ | |
| && tar zxvf /tmp/docs.tar.gz \ | |
| && rm -f /tmp/docs.tar.gz |
Comment on lines
+1
to
+3
| build: | ||
| main: ./cmd/mailroom/main.go | ||
| binary: mailroom |
There was a problem hiding this comment.
This appears to change the GoReleaser config schema (e.g., removing version: 2 and the builds: list). Unless the repository has also upgraded to a GoReleaser version/config format that supports build: at the root, this will likely break releases. Please confirm the intended GoReleaser schema and adjust the config accordingly.
| rows, err := db.QueryContext(ctx, sqlSelectUsersByOrg, orgID) | ||
| if err != nil && err != sql.ErrNoRows { | ||
| return nil, errors.Wrapf(err, "error querying users for org: %d", orgID) | ||
| } |
There was a problem hiding this comment.
rows is no longer explicitly closed (previously defer rows.Close() was used). If ScanJSONRows doesn't guarantee closing the *sql.Rows, this can leak DB resources. Either ensure ScanJSONRows closes rows in all cases, or add defer rows.Close() here (and in the other loaders using the same pattern).
Suggested change
| } | |
| } | |
| if rows != nil { | |
| defer rows.Close() | |
| } |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #704 +/- ##
==========================================
- Coverage 62.94% 61.09% -1.85%
==========================================
Files 148 140 -8
Lines 12611 11316 -1295
==========================================
- Hits 7938 6914 -1024
+ Misses 3835 3629 -206
+ Partials 838 773 -65 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated upmerge of the latest
v9.0.1tag fromnyaruka/mailroom(BSL) to the AGPL mirror.