From 6bc8d2a9d96db9480f32bd2140ce0296d4e56026 Mon Sep 17 00:00:00 2001 From: Rowan Seymour Date: Tue, 7 Sep 2021 16:28:14 -0500 Subject: [PATCH] Add force param to close tickets endpoint to let us ignore errors on external ticket service when removing a ticketer --- core/models/tickets.go | 4 +- core/models/tickets_test.go | 4 +- .../rocketchat/testdata/event_callback.json | 6 +- services/tickets/utils.go | 2 +- web/testing.go | 10 +-- web/ticket/base.go | 1 + web/ticket/base_test.go | 1 + web/ticket/close.go | 8 +- web/ticket/testdata/close.json | 79 ++++++++++++++++++- 9 files changed, 94 insertions(+), 21 deletions(-) diff --git a/core/models/tickets.go b/core/models/tickets.go index d88af5333..4f787d88f 100644 --- a/core/models/tickets.go +++ b/core/models/tickets.go @@ -517,7 +517,7 @@ WHERE ` // CloseTickets closes the passed in tickets -func CloseTickets(ctx context.Context, db Queryer, oa *OrgAssets, userID UserID, tickets []*Ticket, externally bool, logger *HTTPLogger) (map[*Ticket]*TicketEvent, error) { +func CloseTickets(ctx context.Context, db Queryer, oa *OrgAssets, userID UserID, tickets []*Ticket, externally, force bool, logger *HTTPLogger) (map[*Ticket]*TicketEvent, error) { byTicketer := make(map[TicketerID][]*Ticket) ids := make([]TicketID, 0, len(tickets)) events := make([]*TicketEvent, 0, len(tickets)) @@ -550,7 +550,7 @@ func CloseTickets(ctx context.Context, db Queryer, oa *OrgAssets, userID UserID, } err = service.Close(ticketerTickets, logger.Ticketer(ticketer)) - if err != nil { + if err != nil && !force { return nil, err } } diff --git a/core/models/tickets_test.go b/core/models/tickets_test.go index af3cb8106..2a662183b 100644 --- a/core/models/tickets_test.go +++ b/core/models/tickets_test.go @@ -292,7 +292,7 @@ func TestCloseTickets(t *testing.T) { modelTicket2 := ticket2.Load(db) logger := &models.HTTPLogger{} - evts, err := models.CloseTickets(ctx, db, oa, testdata.Admin.ID, []*models.Ticket{modelTicket1, modelTicket2}, true, logger) + evts, err := models.CloseTickets(ctx, db, oa, testdata.Admin.ID, []*models.Ticket{modelTicket1, modelTicket2}, true, false, logger) require.NoError(t, err) assert.Equal(t, 1, len(evts)) assert.Equal(t, models.TicketEventTypeClosed, evts[modelTicket1].EventType()) @@ -316,7 +316,7 @@ func TestCloseTickets(t *testing.T) { ticket3 := testdata.InsertOpenTicket(db, testdata.Org1, testdata.Cathy, testdata.Mailgun, testdata.DefaultTopic, "Problem", "Where my shoes", "123", nil) modelTicket3 := ticket3.Load(db) - evts, err = models.CloseTickets(ctx, db, oa, models.NilUserID, []*models.Ticket{modelTicket3}, false, logger) + evts, err = models.CloseTickets(ctx, db, oa, models.NilUserID, []*models.Ticket{modelTicket3}, false, false, logger) require.NoError(t, err) assert.Equal(t, 1, len(evts)) assert.Equal(t, models.TicketEventTypeClosed, evts[modelTicket3].EventType()) diff --git a/services/tickets/rocketchat/testdata/event_callback.json b/services/tickets/rocketchat/testdata/event_callback.json index 75add7bfd..d999947bf 100644 --- a/services/tickets/rocketchat/testdata/event_callback.json +++ b/services/tickets/rocketchat/testdata/event_callback.json @@ -32,7 +32,7 @@ "text": "We can help" } }, - "status": 404, + "status": 401, "response": { "status": "unauthorized" } @@ -54,7 +54,7 @@ "text": "We can help" } }, - "status": 404, + "status": 401, "response": { "status": "unauthorized" } @@ -97,7 +97,7 @@ "text": "We can help" } }, - "status": 400, + "status": 404, "response": { "error": "no such ticket 88bfa1dc-be33-45c2-b469-294ecb0eba90" } diff --git a/services/tickets/utils.go b/services/tickets/utils.go index ed2389a56..27fdb0506 100644 --- a/services/tickets/utils.go +++ b/services/tickets/utils.go @@ -142,7 +142,7 @@ func FetchFile(url string, headers map[string]string) (*File, error) { // CloseTicket closes the given ticket, and creates and queues a closed event func CloseTicket(ctx context.Context, rt *runtime.Runtime, oa *models.OrgAssets, ticket *models.Ticket, externally bool, l *models.HTTPLogger) error { - events, err := models.CloseTickets(ctx, rt.DB, oa, models.NilUserID, []*models.Ticket{ticket}, externally, l) + events, err := models.CloseTickets(ctx, rt.DB, oa, models.NilUserID, []*models.Ticket{ticket}, externally, false, l) if err != nil { return errors.Wrap(err, "error closing ticket") } diff --git a/web/testing.go b/web/testing.go index 2ff1317c9..94ab7d3ab 100644 --- a/web/testing.go +++ b/web/testing.go @@ -65,7 +65,7 @@ func RunWebTests(t *testing.T, truthFile string, substitutions map[string]string actualResponse []byte } - tcs := make([]*TestCase, 0, 20) + tcs := make([]TestCase, 0, 20) tcJSON, err := os.ReadFile(truthFile) require.NoError(t, err) @@ -73,8 +73,7 @@ func RunWebTests(t *testing.T, truthFile string, substitutions map[string]string tcJSON = bytes.ReplaceAll(tcJSON, []byte("$"+key+"$"), []byte(value)) } - err = json.Unmarshal(tcJSON, &tcs) - require.NoError(t, err) + jsonx.MustUnmarshal(tcJSON, &tcs) for i, tc := range tcs { dates.SetNowSource(dates.NewSequentialNowSource(time.Date(2018, 7, 6, 12, 30, 0, 123456789, time.UTC))) @@ -91,14 +90,13 @@ func RunWebTests(t *testing.T, truthFile string, substitutions map[string]string var req *http.Request if tc.BodyEncode == "multipart" { var parts []MultiPartPart - err = json.Unmarshal(tc.Body, &parts) - require.NoError(t, err) + jsonx.MustUnmarshal(tc.Body, &parts) req, err = MakeMultipartRequest(tc.Method, testURL, parts, tc.Headers) } else if len(tc.Body) >= 2 && tc.Body[0] == '"' { // if body is a string, treat it as a URL encoded submission bodyStr := "" - json.Unmarshal(tc.Body, &bodyStr) + jsonx.MustUnmarshal(tc.Body, &bodyStr) bodyReader := strings.NewReader(bodyStr) req, err = httpx.NewRequest(tc.Method, testURL, bodyReader, tc.Headers) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") diff --git a/web/ticket/base.go b/web/ticket/base.go index d67a2907d..a05a6f376 100644 --- a/web/ticket/base.go +++ b/web/ticket/base.go @@ -10,6 +10,7 @@ type bulkTicketRequest struct { OrgID models.OrgID `json:"org_id" validate:"required"` UserID models.UserID `json:"user_id" validate:"required"` TicketIDs []models.TicketID `json:"ticket_ids"` + Force bool `json:"force"` } type bulkTicketResponse struct { diff --git a/web/ticket/base_test.go b/web/ticket/base_test.go index 3fbe09168..9a7d72cc1 100644 --- a/web/ticket/base_test.go +++ b/web/ticket/base_test.go @@ -48,6 +48,7 @@ func TestTicketClose(t *testing.T) { testdata.InsertOpenTicket(db, testdata.Org1, testdata.Cathy, testdata.Mailgun, testdata.DefaultTopic, "Need help", "Have you seen my cookies?", "17", testdata.Admin) testdata.InsertOpenTicket(db, testdata.Org1, testdata.Cathy, testdata.Zendesk, testdata.DefaultTopic, "More help", "Have you seen my cookies?", "21", nil) testdata.InsertClosedTicket(db, testdata.Org1, testdata.Cathy, testdata.Zendesk, testdata.DefaultTopic, "Old question", "Have you seen my cookies?", "34", testdata.Editor) + testdata.InsertOpenTicket(db, testdata.Org1, testdata.Cathy, testdata.Zendesk, testdata.DefaultTopic, "More help", "Have you seen my cookies?", "21", nil) web.RunWebTests(t, "testdata/close.json", nil) } diff --git a/web/ticket/close.go b/web/ticket/close.go index 3979297ba..d72250bf2 100644 --- a/web/ticket/close.go +++ b/web/ticket/close.go @@ -16,12 +16,14 @@ func init() { web.RegisterJSONRoute(http.MethodPost, "/mr/ticket/close", web.RequireAuthToken(web.WithHTTPLogs(handleClose))) } -// Closes any open tickets with the given ids +// Closes any open tickets with the given ids. If force=true then even if tickets can't be closed on external service, +// they are still closed locally. This is used in case of deleting a ticketing service which may no longer be functioning. // // { // "org_id": 123, // "user_id": 234, -// "ticket_ids": [1234, 2345] +// "ticket_ids": [1234, 2345], +// "force": false // } // func handleClose(ctx context.Context, rt *runtime.Runtime, r *http.Request, l *models.HTTPLogger) (interface{}, int, error) { @@ -41,7 +43,7 @@ func handleClose(ctx context.Context, rt *runtime.Runtime, r *http.Request, l *m return nil, http.StatusBadRequest, errors.Wrapf(err, "error loading tickets for org: %d", request.OrgID) } - evts, err := models.CloseTickets(ctx, rt.DB, oa, request.UserID, tickets, true, l) + evts, err := models.CloseTickets(ctx, rt.DB, oa, request.UserID, tickets, true, request.Force, l) if err != nil { return nil, http.StatusInternalServerError, errors.Wrap(err, "error closing tickets") } diff --git a/web/ticket/testdata/close.json b/web/ticket/testdata/close.json index 9b3916e13..c644b9e4f 100644 --- a/web/ticket/testdata/close.json +++ b/web/ticket/testdata/close.json @@ -27,7 +27,7 @@ "db_assertions": [ { "query": "SELECT count(*) FROM tickets_ticket WHERE status = 'O'", - "count": 1 + "count": 2 }, { "query": "SELECT count(*) FROM tickets_ticket WHERE status = 'C'", @@ -55,8 +55,8 @@ "org_id": 1, "user_id": 3, "ticket_ids": [ - 1, - 2 + 2, + 3 ] }, "status": 200, @@ -68,7 +68,7 @@ "db_assertions": [ { "query": "SELECT count(*) FROM tickets_ticket WHERE status = 'O'", - "count": 0 + "count": 1 }, { "query": "SELECT count(*) FROM tickets_ticket WHERE status = 'C'", @@ -79,5 +79,76 @@ "count": 2 } ] + }, + { + "label": "error response and no closing if closing fails on zendesk side", + "http_mocks": { + "https://nyaruka.zendesk.com/api/v2/tickets/update_many.json?ids=21": [ + { + "status": 400, + "body": "{\"error\": \"oops\", \"description\": \"something went wrong\"}" + } + ] + }, + "method": "POST", + "path": "/mr/ticket/close", + "body": { + "org_id": 1, + "user_id": 3, + "ticket_ids": [ + 4 + ] + }, + "status": 500, + "response": { + "error": "error closing tickets: something went wrong" + }, + "db_assertions": [ + { + "query": "SELECT count(*) FROM tickets_ticket WHERE status = 'O'", + "count": 1 + }, + { + "query": "SELECT count(*) FROM tickets_ticket WHERE status = 'C'", + "count": 3 + } + ] + }, + { + "label": "unless force is true", + "http_mocks": { + "https://nyaruka.zendesk.com/api/v2/tickets/update_many.json?ids=21": [ + { + "status": 200, + "body": "{\"error\": \"oops\", \"description\": \"something went wrong\"}" + } + ] + }, + "method": "POST", + "path": "/mr/ticket/close", + "body": { + "org_id": 1, + "user_id": 3, + "ticket_ids": [ + 4 + ], + "force": true + }, + "status": 200, + "response": { + "changed_ids": [ + 4 + ] + }, + "db_assertions": [ + { + "query": "SELECT count(*) FROM tickets_ticket WHERE status = 'O'", + "count": 0 + }, + { + "query": "SELECT count(*) FROM tickets_ticket WHERE status = 'C'", + "count": 4 + } + ] } ] \ No newline at end of file