-
-
Notifications
You must be signed in to change notification settings - Fork 626
feat: unarchive message thread on new inbound message (per-phone setting) #954
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 14 commits
91eac3b
2a046f5
ccf8c62
ba3b372
5987b0a
313b699
77a108a
604889e
db085c1
fa32d02
7e5ca43
1cb1449
e1c3d1f
f355239
3fc08ae
a4fc0af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,6 @@ SECURITY_AUDIT_REPORT.md | |
|
|
||
| *.exe | ||
|
|
||
| docs/ | ||
| .output | ||
| .agents/ | ||
| skills-lock.json | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ type MessageThreadService struct { | |
| logger telemetry.Logger | ||
| tracer telemetry.Tracer | ||
| repository repositories.MessageThreadRepository | ||
| phoneRepository repositories.PhoneRepository | ||
| eventDispatcher *EventDispatcher | ||
| } | ||
|
|
||
|
|
@@ -29,13 +30,15 @@ func NewMessageThreadService( | |
| logger telemetry.Logger, | ||
| tracer telemetry.Tracer, | ||
| repository repositories.MessageThreadRepository, | ||
| phoneRepository repositories.PhoneRepository, | ||
| eventDispatcher *EventDispatcher, | ||
| ) (s *MessageThreadService) { | ||
| return &MessageThreadService{ | ||
| logger: logger.WithService(fmt.Sprintf("%T", s)), | ||
| tracer: tracer, | ||
| eventDispatcher: eventDispatcher, | ||
| repository: repository, | ||
| phoneRepository: phoneRepository, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -50,6 +53,14 @@ type MessageThreadUpdateParams struct { | |
| Timestamp time.Time | ||
| } | ||
|
|
||
| // shouldCheckUnarchive reports whether a thread update is a new inbound message | ||
| // landing on an archived thread. Only in that case is the phone's | ||
| // UnarchiveThread setting consulted, so the phone is not loaded on the common | ||
| // path where the thread is not archived. | ||
| func (service *MessageThreadService) shouldCheckUnarchive(thread *entities.MessageThread, params MessageThreadUpdateParams) bool { | ||
| return thread.IsArchived && params.Status == entities.MessageStatusReceived | ||
| } | ||
|
|
||
| // DeleteAllForUser deletes all entities.MessageThread for an entities.UserID. | ||
| func (service *MessageThreadService) DeleteAllForUser(ctx context.Context, userID entities.UserID) error { | ||
| ctx, span, ctxLogger := service.tracer.StartWithLogger(ctx, service.logger) | ||
|
|
@@ -92,6 +103,16 @@ func (service *MessageThreadService) UpdateThread(ctx context.Context, params Me | |
| return nil | ||
| } | ||
|
|
||
| if service.shouldCheckUnarchive(thread, params) { | ||
| phone, phoneErr := service.phoneRepository.Load(ctx, params.UserID, params.Owner) | ||
| if phoneErr != nil { | ||
| ctxLogger.Warn(stacktrace.Propagate(phoneErr, "cannot load phone [%s] for user [%s] to resolve UnarchiveThread; leaving thread [%s] archived", params.Owner, params.UserID, thread.ID)) | ||
| } else if phone.UnarchiveThread { | ||
|
Comment on lines
+106
to
+110
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the PR description to match the implemented flow. The event-based approach was intentionally replaced with a lazy phone load in UpdateThread (gated by shouldCheckUnarchive) so the common non-archived path does zero extra DB reads; the phone is only read when an inbound message hits an already-archived thread. |
||
| thread.UpdateArchive(false) | ||
| ctxLogger.Info(fmt.Sprintf("unarchiving thread [%s] after inbound message [%s]", thread.ID, params.MessageID)) | ||
| } | ||
| } | ||
|
|
||
| if err = service.repository.Update(ctx, thread.Update(params.Timestamp, params.MessageID, params.Content, params.Status)); err != nil { | ||
| msg := fmt.Sprintf("cannot update message thread with id [%s] after adding message [%s]", thread.ID, params.MessageID) | ||
| return service.tracer.WrapErrorSpan(span, stacktrace.Propagate(err, msg)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| package services | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/NdoleStudio/httpsms/pkg/entities" | ||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestShouldCheckUnarchive(t *testing.T) { | ||
| service := &MessageThreadService{} | ||
|
|
||
| archived := &entities.MessageThread{IsArchived: true} | ||
| notArchived := &entities.MessageThread{IsArchived: false} | ||
|
|
||
| received := MessageThreadUpdateParams{Status: entities.MessageStatusReceived} | ||
| sent := MessageThreadUpdateParams{Status: entities.MessageStatusSent} | ||
|
|
||
| assert.True(t, service.shouldCheckUnarchive(archived, received), "archived + inbound -> consult phone setting") | ||
| assert.False(t, service.shouldCheckUnarchive(archived, sent), "outbound status -> no check") | ||
| assert.False(t, service.shouldCheckUnarchive(notArchived, received), "already unarchived -> no check") | ||
| } | ||
|
Comment on lines
+19
to
+23
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in a4fc0af. TestShouldCheckUnarchive now covers not-archived + non-received. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not changing this — the stated goal (keep the field optional in the OpenAPI schema) is not achievable in this repo. Swagger is generated with
--requiredByDefault, which marks every field required regardless of pointer type.missed_call_auto_replyis a*stringand is already listed underrequiredforrequests.PhoneUpsert, so switchingunarchive_threadto*boolwould not remove it fromrequired. The non-pointerboolis consistent with the other partial-update fields (max_send_attempts,messages_per_minute), and the server never enforces required at runtime — absent keys are skipped via thefieldspresence check, so partial updates keep working.