Skip to content

Commit

Permalink
Add force param to close tickets endpoint to let us ignore errors on …
Browse files Browse the repository at this point in the history
…external ticket service when removing a ticketer
  • Loading branch information
rowanseymour committed Sep 7, 2021
1 parent 39b36f9 commit 6bc8d2a
Show file tree
Hide file tree
Showing 9 changed files with 94 additions and 21 deletions.
4 changes: 2 additions & 2 deletions core/models/tickets.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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
}
}
Expand Down
4 changes: 2 additions & 2 deletions core/models/tickets_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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())
Expand Down
6 changes: 3 additions & 3 deletions services/tickets/rocketchat/testdata/event_callback.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
"text": "We can help"
}
},
"status": 404,
"status": 401,
"response": {
"status": "unauthorized"
}
Expand All @@ -54,7 +54,7 @@
"text": "We can help"
}
},
"status": 404,
"status": 401,
"response": {
"status": "unauthorized"
}
Expand Down Expand Up @@ -97,7 +97,7 @@
"text": "We can help"
}
},
"status": 400,
"status": 404,
"response": {
"error": "no such ticket 88bfa1dc-be33-45c2-b469-294ecb0eba90"
}
Expand Down
2 changes: 1 addition & 1 deletion services/tickets/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down
10 changes: 4 additions & 6 deletions web/testing.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,16 +65,15 @@ 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)

for key, value := range substitutions {
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)))
Expand All @@ -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")
Expand Down
1 change: 1 addition & 0 deletions web/ticket/base.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions web/ticket/base_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
8 changes: 5 additions & 3 deletions web/ticket/close.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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")
}
Expand Down
79 changes: 75 additions & 4 deletions web/ticket/testdata/close.json
Original file line number Diff line number Diff line change
Expand Up @@ -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'",
Expand Down Expand Up @@ -55,8 +55,8 @@
"org_id": 1,
"user_id": 3,
"ticket_ids": [
1,
2
2,
3
]
},
"status": 200,
Expand All @@ -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'",
Expand All @@ -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
}
]
}
]

0 comments on commit 6bc8d2a

Please sign in to comment.