Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion contract/battery.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ func Cases() []Case {

// --- Tickets: get by id / ref ---------------------------------------
{Name: "tickets/get_by_id_happy", Method: "GET", Path: "/api/v1/tickets/42", Auth: AuthReadTickets,
Notes: "Ticket + firstMessages + deprecated top-level totalMessageCount duplicating ticket.totalMessageCount."},
Notes: "Ticket + firstMessages."},
{Name: "tickets/get_by_id_not_found", Method: "GET", Path: "/api/v1/tickets/9999", Auth: AuthReadTickets,
Notes: "NotFound names the numeric id."},
{Name: "tickets/get_by_id_parse_error", Method: "GET", Path: "/api/v1/tickets/abc", Auth: AuthReadTickets,
Expand Down
7 changes: 3 additions & 4 deletions contract/fake_datastore_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -674,16 +674,15 @@ func (recordingDatastore) GetTicketByRef(_ context.Context, _ datastores.TicketR
return nil, gorm.ErrRecordNotFound
}

func (recordingDatastore) GetTicketFirstMessages(_ context.Context, ticketID uint32, n int, _ bool) ([]*types.Message, uint32, error) {
func (recordingDatastore) GetTicketFirstMessages(_ context.Context, ticketID uint32, n int, _ bool) ([]*types.Message, error) {
if ticketID != 42 {
return []*types.Message{}, 0, nil
return []*types.Message{}, nil
}
msgs := seedMessages42()
total := uint32(len(msgs))
if n < len(msgs) {
msgs = msgs[:n]
}
return msgs, total, nil
return msgs, nil
}

func (recordingDatastore) ListTicketMessages(_ context.Context, ticketID uint32, afterCursor string, perPage uint32, _ bool) ([]*types.Message, string, bool, error) {
Expand Down
5 changes: 2 additions & 3 deletions contract/goldens/tickets/get_by_id_happy.golden.json
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,14 @@
"ticketRef": "MF1UI9HE",
"title": "Enlistment — John.Doe",
"totalMessageCount": 3
},
"totalMessageCount": 3
}
},
"case": "tickets/get_by_id_happy",
"header": {
"Content-Type": "application/json"
},
"method": "GET",
"notes": "Ticket + firstMessages + deprecated top-level totalMessageCount duplicating ticket.totalMessageCount.",
"notes": "Ticket + firstMessages.",
"path": "/api/v1/tickets/42",
"status": 200
}
3 changes: 1 addition & 2 deletions contract/goldens/tickets/get_by_ref_happy.golden.json
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,7 @@
"ticketRef": "MF1UI9HE",
"title": "Enlistment — John.Doe",
"totalMessageCount": 3
},
"totalMessageCount": 3
}
},
"case": "tickets/get_by_ref_happy",
"header": {
Expand Down
2 changes: 1 addition & 1 deletion datastores/datastore.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ type Datastore interface {
ListTickets(ctx context.Context, rc TicketReferenceCache, filter *ListTicketsFilter) (tickets []*types.Ticket, nextCursor string, hasMore bool, err error)
GetTicket(ctx context.Context, rc TicketReferenceCache, ticketID uint32, forumBaseURL string) (*types.Ticket, error)
GetTicketByRef(ctx context.Context, rc TicketReferenceCache, ref string, forumBaseURL string) (*types.Ticket, error)
GetTicketFirstMessages(ctx context.Context, ticketID uint32, n int, includeHidden bool) (msgs []*types.Message, totalCount uint32, err error)
GetTicketFirstMessages(ctx context.Context, ticketID uint32, n int, includeHidden bool) ([]*types.Message, error)
ListTicketMessages(ctx context.Context, ticketID uint32, afterCursor string, perPage uint32, includeHidden bool) (msgs []*types.Message, nextCursor string, hasMore bool, err error)
ListCategories(ctx context.Context, rc TicketReferenceCache) ([]*types.Category, error)
}
Expand Down
21 changes: 6 additions & 15 deletions datastores/tickets.go
Original file line number Diff line number Diff line change
Expand Up @@ -392,30 +392,21 @@ func (ds *Mysql) GetTicket(ctx context.Context, rc TicketReferenceCache, ticketI
return generateTicketProto(&row, rc, strings.TrimRight(base, "/")), nil
}

func (ds *Mysql) GetTicketFirstMessages(ctx context.Context, ticketID uint32, n int, includeHidden bool) ([]*types.Message, uint32, error) {
var total int64
q := ds.Db.WithContext(ctx).Model(&xenforo.TicketMessage{}).Where("ticket_id = ?", ticketID)
if !includeHidden {
q = q.Where("message_state = ?", "visible")
}
if tx := q.Count(&total); tx.Error != nil {
return nil, 0, tx.Error
}

func (ds *Mysql) GetTicketFirstMessages(ctx context.Context, ticketID uint32, n int, includeHidden bool) ([]*types.Message, error) {
var rows []xenforo.TicketMessage
q2 := ds.Db.WithContext(ctx).Where("ticket_id = ?", ticketID)
q := ds.Db.WithContext(ctx).Where("ticket_id = ?", ticketID)
if !includeHidden {
q2 = q2.Where("message_state = ?", "visible")
q = q.Where("message_state = ?", "visible")
}
tx := q2.Order("position ASC").Limit(n).Find(&rows)
tx := q.Order("position ASC").Limit(n).Find(&rows)
if tx.Error != nil {
return nil, 0, tx.Error
return nil, tx.Error
}
out := make([]*types.Message, 0, len(rows))
for i := range rows {
out = append(out, messageToProto(&rows[i]))
}
return out, uint32(total), nil
return out, nil
}

func (ds *Mysql) ListTicketMessages(ctx context.Context, ticketID uint32, afterCursor string, perPage uint32, includeHidden bool) ([]*types.Message, string, bool, error) {
Expand Down
31 changes: 13 additions & 18 deletions datastores/tickets_harness_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -541,42 +541,37 @@ func TestGetTicketByRef(t *testing.T) {
}
}

// First-messages preview: hidden replies are excluded from both the
// page and the total unless requested; the page is position-ordered
// and capped at n while the total counts the whole ticket.
// First-messages preview: hidden replies are excluded unless requested;
// the page is position-ordered and capped at n.
func TestGetTicketFirstMessages(t *testing.T) {
ds, _ := openTicketsHarness(t)

msgs, total, err := ds.GetTicketFirstMessages(context.Background(), 1, 10, false)
msgs, err := ds.GetTicketFirstMessages(context.Background(), 1, 10, false)
if err != nil {
t.Fatalf("GetTicketFirstMessages(visible): %v", err)
}
if total != 2 {
t.Errorf("visible total = %d, want 2 (hidden internal note excluded)", total)
}
if len(msgs) != 2 || msgs[0].Position != 0 || msgs[1].Position != 2 {
t.Fatalf("visible positions = %v, want [0 2]", messagePositions(msgs))
}
if msgs[0].Message != "I cannot see my milpacs page." || msgs[0].Username != "TicketGuy.G" {
t.Errorf("starter message = %q by %q", msgs[0].Message, msgs[0].Username)
}

msgs, total, err = ds.GetTicketFirstMessages(context.Background(), 1, 10, true)
msgs, err = ds.GetTicketFirstMessages(context.Background(), 1, 10, true)
if err != nil {
t.Fatalf("GetTicketFirstMessages(include hidden): %v", err)
}
if total != 3 || len(msgs) != 3 || msgs[1].MessageState != "hidden" {
t.Errorf("with hidden: total=%d positions=%v state[1]=%q, want 3/[0 1 2]/hidden",
total, messagePositions(msgs), msgs[1].MessageState)
if len(msgs) != 3 || msgs[1].MessageState != "hidden" {
t.Errorf("with hidden: positions=%v state[1]=%q, want [0 1 2]/hidden",
messagePositions(msgs), msgs[1].MessageState)
}

// n caps the page, not the total.
msgs, total, err = ds.GetTicketFirstMessages(context.Background(), 1, 1, false)
msgs, err = ds.GetTicketFirstMessages(context.Background(), 1, 1, false)
if err != nil {
t.Fatalf("GetTicketFirstMessages(n=1): %v", err)
}
if len(msgs) != 1 || total != 2 {
t.Errorf("n=1: got %d messages, total %d; want 1 message, total 2", len(msgs), total)
if len(msgs) != 1 {
t.Errorf("n=1: got %d messages, want 1", len(msgs))
}
}

Expand Down Expand Up @@ -677,12 +672,12 @@ func TestListTicketMessages_CursorWalksAcrossHiddenGap(t *testing.T) {
func TestTicketMessages_NonexistentTicketIsEmptyNotError(t *testing.T) {
ds, _ := openTicketsHarness(t)

msgs, total, err := ds.GetTicketFirstMessages(context.Background(), 9999, 10, true)
msgs, err := ds.GetTicketFirstMessages(context.Background(), 9999, 10, true)
if err != nil {
t.Fatalf("GetTicketFirstMessages(9999): %v", err)
}
if len(msgs) != 0 || total != 0 {
t.Errorf("nonexistent ticket: got %d messages, total %d; want 0/0", len(msgs), total)
if len(msgs) != 0 {
t.Errorf("nonexistent ticket: got %d messages, want 0", len(msgs))
}

listed, next, more, err := ds.ListTicketMessages(context.Background(), 9999, "", 10, true)
Expand Down
7 changes: 1 addition & 6 deletions openapi/openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1915,12 +1915,7 @@ components:
type: array
items:
$ref: '#/components/schemas/Message'
totalMessageCount:
type: integer
minimum: 0
deprecated: true
description: 'DEPRECATED: use ticket.totalMessageCount. Removal target: v2.3.0.'
required: [ticket, firstMessages, totalMessageCount]
required: [ticket, firstMessages]
additionalProperties: false

ListTicketsResponse:
Expand Down
7 changes: 3 additions & 4 deletions rest/fake_tickets_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -314,19 +314,18 @@ func (f *fakeDatastore) GetTicketByRef(_ context.Context, rc datastores.TicketRe
return nil, gorm.ErrRecordNotFound
}

func (f *fakeDatastore) GetTicketFirstMessages(_ context.Context, ticketID uint32, n int, includeHidden bool) ([]*types.Message, uint32, error) {
func (f *fakeDatastore) GetTicketFirstMessages(_ context.Context, ticketID uint32, n int, includeHidden bool) ([]*types.Message, error) {
if f.getTicketFirstMessages != nil {
return f.getTicketFirstMessages(ticketID, n, includeHidden)
}
if ticketID != 42 {
return []*types.Message{}, 0, nil
return []*types.Message{}, nil
}
msgs := seedMessages42()
total := uint32(len(msgs))
if n < len(msgs) {
msgs = msgs[:n]
}
return msgs, total, nil
return msgs, nil
}

func (f *fakeDatastore) ListTicketMessages(_ context.Context, ticketID uint32, afterCursor string, perPage uint32, includeHidden bool) ([]*types.Message, string, bool, error) {
Expand Down
2 changes: 1 addition & 1 deletion rest/rest_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ type fakeDatastore struct {
listTickets func(*datastores.ListTicketsFilter) ([]*types.Ticket, string, bool, error)
getTicket func(ticketID uint32) (*types.Ticket, error)
getTicketByRef func(ref string) (*types.Ticket, error)
getTicketFirstMessages func(ticketID uint32, n int, includeHidden bool) ([]*types.Message, uint32, error)
getTicketFirstMessages func(ticketID uint32, n int, includeHidden bool) ([]*types.Message, error)
listTicketMessages func(ticketID uint32, afterCursor string, perPage uint32, includeHidden bool) ([]*types.Message, string, bool, error)
listCategories func() ([]*types.Category, error)

Expand Down
14 changes: 5 additions & 9 deletions rest/tickets.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,19 +114,16 @@ func getTicketByRef(ds datastores.Datastore, rc datastores.TicketReferenceCache)
}

// writeTicketResponse assembles the shared GetTicketResponse envelope (the
// by-id and by-ref bindings return the same shape): the ticket, its first
// messages, and the deprecated top-level total that mirrors
// ticket.totalMessageCount but is computed independently (visible-message
// COUNT vs replyCount+1 — they diverge on hidden messages; see
// types.GetTicketResponse).
// by-id and by-ref bindings return the same shape): the ticket and its first
// messages.
func writeTicketResponse(w http.ResponseWriter, r *http.Request, ds datastores.Datastore, ticket *types.Ticket) {
if ticket == nil {
// A (nil, nil) datastore return is a bug — fail loudly rather than
// serve a zeroed-garbage 200.
writeError(w, r, codeInternal, "fetch ticket: nil ticket")
return
}
msgs, total, err := ds.GetTicketFirstMessages(r.Context(), ticket.TicketId, firstMessagesCount, false)
msgs, err := ds.GetTicketFirstMessages(r.Context(), ticket.TicketId, firstMessagesCount, false)
if err != nil {
writeError(w, r, codeInternal, "fetch ticket messages: %v", err)
return
Expand All @@ -135,9 +132,8 @@ func writeTicketResponse(w http.ResponseWriter, r *http.Request, ds datastores.D
msgs = []*types.Message{}
}
writeJSON(w, r, types.GetTicketResponse{
Ticket: ticket,
FirstMessages: msgs,
TotalMessageCount: total,
Ticket: ticket,
FirstMessages: msgs,
})
}

Expand Down
8 changes: 4 additions & 4 deletions rest/tickets_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ func TestNewStack_NilTicketWithNilErrorIsInternalJSON(t *testing.T) {
// An outage on the first-messages fetch (after the ticket resolved) keeps its
// own frozen message string: "fetch ticket messages: %v".
func TestNewStack_GetTicketFirstMessagesOutageIsInternalJSON(t *testing.T) {
h := rest.New(&fakeDatastore{getTicketFirstMessages: func(uint32, int, bool) ([]*types.Message, uint32, error) {
return nil, 0, io.ErrUnexpectedEOF
h := rest.New(&fakeDatastore{getTicketFirstMessages: func(uint32, int, bool) ([]*types.Message, error) {
return nil, io.ErrUnexpectedEOF
}}, &stubReferenceCache{})

rr := ticketsGet(t, h, "/api/v1/tickets/42")
Expand Down Expand Up @@ -543,9 +543,9 @@ func TestNewStack_GetTicketByRefDatastoreOutageIsInternalJSON(t *testing.T) {
func TestNewStack_FirstMessagesCountAndVisibilityFrozen(t *testing.T) {
for _, path := range []string{"/api/v1/tickets/42", "/api/v1/tickets/ref/MF1UI9HE"} {
gotN, gotHidden, called := 0, true, false
h := rest.New(&fakeDatastore{getTicketFirstMessages: func(_ uint32, n int, includeHidden bool) ([]*types.Message, uint32, error) {
h := rest.New(&fakeDatastore{getTicketFirstMessages: func(_ uint32, n int, includeHidden bool) ([]*types.Message, error) {
gotN, gotHidden, called = n, includeHidden, true
return []*types.Message{}, 0, nil
return []*types.Message{}, nil
}}, &stubReferenceCache{})

rr := ticketsGet(t, h, path)
Expand Down
11 changes: 3 additions & 8 deletions types/tickets.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,10 @@ type Message struct {

// GetTicketResponse is the envelope shared by GET /api/v1/tickets/{ticketId}
// and GET /api/v1/tickets/ref/{ticketRef}: the ticket plus its first
// messages (oldest first). The top-level TotalMessageCount mirrors the old
// surface's duplicate of ticket.totalMessageCount — deprecated there, frozen
// here — but the two are computed INDEPENDENTLY (ticket.totalMessageCount is
// replyCount+1; this one is a COUNT of visible message rows) and may diverge
// for threads with hidden messages.
// messages (oldest first).
type GetTicketResponse struct {
Ticket *Ticket `json:"ticket"`
FirstMessages []*Message `json:"firstMessages"`
TotalMessageCount uint32 `json:"totalMessageCount"`
Ticket *Ticket `json:"ticket"`
FirstMessages []*Message `json:"firstMessages"`
}

// ListTicketsResponse is the GET /api/v1/tickets envelope: one page of
Expand Down
Loading