handle errors in HandleCommandResponse method (#9888)

app/command.go

@@ -41,13 +41,6 @@ func (a *App) CreateCommandPost(post *model.Post, teamId string, response *model
 	post.Message = model.ParseSlackLinksToMarkdown(response.Text)
 	post.CreateAt = model.GetMillis()
 
-	_, err := a.GetChannelMember(post.ChannelId, post.UserId)
-	if err != nil {
-		err = model.NewAppError("CreateCommandPost", "api.command.command_post.forbidden.app_error", nil, err.Error(), http.StatusForbidden)
-		mlog.Error(err.Error())
-		return nil, err
-	}
-
 	if strings.HasPrefix(post.Type, model.POST_SYSTEM_MESSAGE_PREFIX) {
 		err := model.NewAppError("CreateCommandPost", "api.context.invalid_param.app_error", map[string]interface{}{"Name": "post.type"}, "", http.StatusBadRequest)
 		return nil, err
@@ -297,14 +290,36 @@ func (a *App) ExecuteCommand(args *model.CommandArgs) (*model.CommandResponse, *
 }
 
 func (a *App) HandleCommandResponse(command *model.Command, args *model.CommandArgs, response *model.CommandResponse, builtIn bool) (*model.CommandResponse, *model.AppError) {
-	a.HandleCommandResponsePost(command, args, response, builtIn)
+	trigger := ""
+	if len(args.Command) != 0 {
+		parts := strings.Split(args.Command, " ")
+		trigger = parts[0][1:]
+		trigger = strings.ToLower(trigger)
+	}
+
+	var lastError *model.AppError
+	_, err := a.HandleCommandResponsePost(command, args, response, builtIn)
+
+	if err != nil {
+		mlog.Error(err.Error())
+		lastError = err
+	}
 
 	if response.ExtraResponses != nil {
 		for _, resp := range response.ExtraResponses {
-			a.HandleCommandResponsePost(command, args, resp, builtIn)
+			_, err := a.HandleCommandResponsePost(command, args, resp, builtIn)
+
+			if err != nil {
+				mlog.Error(err.Error())
+				lastError = err
+			}
 		}
 	}
 
+	if lastError != nil {
+		return response, model.NewAppError("command", "api.command.execute_command.create_post_failed.app_error", map[string]interface{}{"Trigger": trigger}, "", http.StatusInternalServerError)
+	}
+
 	return response, nil
 }
 
@@ -318,6 +333,11 @@ func (a *App) HandleCommandResponsePost(command *model.Command, args *model.Comm
 	post.Props = response.Props
 
 	if len(response.ChannelId) != 0 {
+		_, err := a.GetChannelMember(response.ChannelId, args.UserId)
+		if err != nil {
+			err = model.NewAppError("HandleCommandResponsePost", "api.command.command_post.forbidden.app_error", nil, err.Error(), http.StatusForbidden)
+			return nil, err
+		}
 		post.ChannelId = response.ChannelId
 	}
 
@@ -354,7 +374,7 @@ func (a *App) HandleCommandResponsePost(command *model.Command, args *model.Comm
 	response.Attachments = a.ProcessSlackAttachments(response.Attachments)
 
 	if _, err := a.CreateCommandPost(post, args.TeamId, response); err != nil {
-		mlog.Error(err.Error())
+		return post, err
 	}
 
 	return post, nil

app/command_test.go

@@ -64,18 +64,6 @@ func TestCreateCommandPost(t *testing.T) {
 	if err == nil || err.Id != "api.context.invalid_param.app_error" {
 		t.Fatal("should have failed - bad post type")
 	}
-
-	channel := th.CreateChannel(th.BasicTeam)
-
-	post = &model.Post{
-		ChannelId: th.BasicChannel.Id,
-		UserId:    channel.Id,
-	}
-
-	_, err = th.App.CreateCommandPost(post, th.BasicTeam.Id, resp)
-	if err == nil || err.Id != "api.command.command_post.forbidden.app_error" {
-		t.Fatal("should have failed - forbidden channel post")
-	}
 }
 
 func TestHandleCommandResponsePost(t *testing.T) {
@@ -85,16 +73,17 @@ func TestHandleCommandResponsePost(t *testing.T) {
 	command := &model.Command{}
 	args := &model.CommandArgs{
 		ChannelId: th.BasicChannel.Id,
-		TeamId: th.BasicTeam.Id,
-		UserId: th.BasicUser.Id,
-		RootId: "root_id",
-		ParentId: "parent_id",
+		TeamId:    th.BasicTeam.Id,
+		UserId:    th.BasicUser.Id,
+		RootId:    "",
+		ParentId:  "",
 	}
 
 	resp := &model.CommandResponse{
-		Type: model.POST_EPHEMERAL,
-		Props: model.StringInterface{"some_key": "some value"},
-		Text: "some message",
+		Type:         model.POST_DEFAULT,
+		ResponseType: model.COMMAND_RESPONSE_TYPE_IN_CHANNEL,
+		Props:        model.StringInterface{"some_key": "some value"},
+		Text:         "some message",
 	}
 
 	builtIn := true
@@ -199,4 +188,72 @@ func TestHandleCommandResponsePost(t *testing.T) {
 	post, err = th.App.HandleCommandResponsePost(command, args, resp, builtIn)
 	assert.Nil(t, err)
 	assert.Equal(t, "@here", resp.Attachments[0].Text)
+
+	channel = th.CreatePrivateChannel(th.BasicTeam)
+	resp.ChannelId = channel.Id
+	args.UserId = th.BasicUser2.Id
+	post, err = th.App.HandleCommandResponsePost(command, args, resp, builtIn)
+
+	if err == nil || err.Id != "api.command.command_post.forbidden.app_error" {
+		t.Fatal("should have failed - forbidden channel post")
+	}
+}
+func TestHandleCommandResponse(t *testing.T) {
+	th := Setup().InitBasic()
+	defer th.TearDown()
+
+	command := &model.Command{}
+
+	args := &model.CommandArgs{
+		Command:   "/invite username",
+		UserId:    th.BasicUser.Id,
+		ChannelId: th.BasicChannel.Id,
+	}
+
+	resp := &model.CommandResponse{
+		Text: "message 1",
+		Type: model.POST_SYSTEM_GENERIC,
+	}
+
+	builtIn := true
+
+	_, err := th.App.HandleCommandResponse(command, args, resp, builtIn)
+	if err == nil || err.Id != "api.command.execute_command.create_post_failed.app_error" {
+		t.Fatal("should have failed - invalid post type")
+	}
+
+	resp = &model.CommandResponse{
+		Text: "message 1",
+	}
+
+	_, err = th.App.HandleCommandResponse(command, args, resp, builtIn)
+	assert.Nil(t, err)
+
+	resp = &model.CommandResponse{
+		Text: "message 1",
+		ExtraResponses: []*model.CommandResponse{
+			&model.CommandResponse{
+				Text: "message 2",
+			},
+			&model.CommandResponse{
+				Type: model.POST_SYSTEM_GENERIC,
+				Text: "message 3",
+			},
+		},
+	}
+
+	_, err = th.App.HandleCommandResponse(command, args, resp, builtIn)
+	if err == nil || err.Id != "api.command.execute_command.create_post_failed.app_error" {
+		t.Fatal("should have failed - invalid post type on extra response")
+	}
+
+	resp = &model.CommandResponse{
+		ExtraResponses: []*model.CommandResponse{
+			&model.CommandResponse{},
+			&model.CommandResponse{},
+		},
+	}
+
+	_, err = th.App.HandleCommandResponse(command, args, resp, builtIn)
+	assert.Nil(t, err)
 }

i18n/en.json

@@ -359,6 +359,10 @@
     "id": "api.command.duplicate_trigger.app_error",
     "translation": "This trigger word is already in use. Please choose another word."
   },
+  {
+    "id": "api.command.execute_command.create_post_failed.app_error",
+    "translation": "Command '{{.Trigger}}' failed to post response. Please contact your System Administrator."
+  },
   {
     "id": "api.command.execute_command.debug",
     "translation": "Executing cmd=%v userId=%v"