[MM-14846] Update EditAt for FileIds and Attachment in Post + Ignore FileIds Updates (#10540)

* Set EditAt for FileIds and Attachments; Disallow update/patch of FileIds in API Handler

* Add custom comparison methods for StringArray and Post Attachments

* gofmt

* Split up comparison function to child structs

* Naming consistency

* gofmt

api4/post.go

@@ -437,6 +437,9 @@ func updatePost(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// Updating the file_ids of a post is not a supported operation and will be ignored
+	post.FileIds = nil
+
 	if !c.App.SessionHasPermissionToChannelByPost(c.App.Session, c.Params.PostId, model.PERMISSION_EDIT_POST) {
 		c.SetPermissionError(model.PERMISSION_EDIT_POST)
 		return
@@ -479,6 +482,9 @@ func patchPost(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// Updating the file_ids of a post is not a supported operation and will be ignored
+	post.FileIds = nil
+
 	if !c.App.SessionHasPermissionToChannelByPost(c.App.Session, c.Params.PostId, model.PERMISSION_EDIT_POST) {
 		c.SetPermissionError(model.PERMISSION_EDIT_POST)
 		return

api4/post_test.go

@@ -580,6 +580,29 @@ func TestUpdatePost(t *testing.T) {
 	_, resp = Client.UpdatePost(rpost2.Id, up2)
 	CheckBadRequestStatus(t, resp)
 
+	rpost3, err := th.App.CreatePost(&model.Post{ChannelId: channel.Id, Message: "zz" + model.NewId() + "a", UserId: th.BasicUser.Id}, channel, false)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	fileIds := model.StringArray{"abcdef", "geh"}
+	up3 := &model.Post{Id: rpost3.Id, ChannelId: channel.Id, Message: "zz" + model.NewId() + " update post 3", FileIds: fileIds}
+	rrupost3, resp := Client.UpdatePost(rpost3.Id, up3)
+	CheckNoError(t, resp)
+	assert.Empty(t, rrupost3.FileIds)
+
+	up4 := &model.Post{Id: rpost3.Id, ChannelId: channel.Id, Message: "zz" + model.NewId() + " update post 3"}
+	up4.AddProp("attachments", []model.SlackAttachment{
+		{
+			Text: "Hello World",
+		},
+	})
+	rrupost3, resp = Client.UpdatePost(rpost3.Id, up4)
+	CheckNoError(t, resp)
+	assert.NotEqual(t, rpost3.EditAt, rrupost3.EditAt)
+	assert.NotEqual(t, rpost3.Attachments(), rrupost3.Attachments())
+
+
 	Client.Logout()
 	_, resp = Client.UpdatePost(rpost.Id, rpost)
 	CheckUnauthorizedStatus(t, resp)
@@ -671,16 +694,30 @@ func TestPatchPost(t *testing.T) {
 	if rpost.Hashtags != "#otherhashtag" {
 		t.Fatal("Message did not update properly")
 	}
-	if len(rpost.FileIds) != 3 {
-		t.Fatal("FileIds did not update properly")
+	if len(rpost.FileIds) == 3 {
+		t.Fatal("FileIds should not update properly")
 	}
-	if !reflect.DeepEqual(rpost.FileIds, *patch.FileIds) {
-		t.Fatal("FileIds did not update properly")
+	if reflect.DeepEqual(rpost.FileIds, *patch.FileIds) {
+		t.Fatal("FileIds should not update")
 	}
 	if rpost.HasReactions {
 		t.Fatal("HasReactions did not update properly")
 	}
 
+	patch2 := &model.PostPatch{}
+	attachments := []model.SlackAttachment{
+		{
+			Text: "Hello World",
+		},
+	}
+	patch2.Props = new(model.StringInterface)
+	*patch2.Props = model.StringInterface{"attachments": attachments}
+
+	rpost2, resp := Client.PatchPost(post.Id, patch2)
+	CheckNoError(t, resp)
+	assert.NotEmpty(t, rpost2.Props["attachments"])
+	assert.NotEqual(t, rpost.EditAt, rpost2.EditAt)
+
 	if r, err := Client.DoApiPut("/posts/"+post.Id+"/patch", "garbage"); err == nil {
 		t.Fatal("should have errored")
 	} else {

app/post.go

@@ -506,6 +506,11 @@ func (a *App) UpdatePost(post *model.Post, safeUpdate bool) (*model.Post, *model
 		newPost.Props = post.Props
 	}
 
+	// Avoid deep-equal checks if EditAt was already modified through message change
+	if newPost.EditAt == oldPost.EditAt && (!oldPost.FileIds.Equals(newPost.FileIds) || !oldPost.AttachmentsEqual(newPost)) {
+		newPost.EditAt = model.GetMillis()
+	}
+
 	if err := a.FillInPostProps(post, nil); err != nil {
 		return nil, err
 	}

model/integration_action.go

@@ -58,6 +58,66 @@ type PostAction struct {
 	Cookie      string                 `json:"cookie,omitempty" db:"-"`
 }
 
+func (p *PostAction) Equals(input *PostAction) bool {
+	if p.Id != input.Id {
+		return false
+	}
+
+	if p.Type != input.Type {
+		return false
+	}
+
+	if p.Name != input.Name {
+		return false
+	}
+
+	if p.DataSource != input.DataSource {
+		return false
+	}
+
+	if p.Cookie != input.Cookie {
+		return false
+	}
+
+	// Compare PostActionOptions
+	if len(p.Options) != len(input.Options) {
+		return false
+	}
+
+	for k := range p.Options {
+		if p.Options[k].Text != input.Options[k].Text {
+			return false
+		}
+
+		if p.Options[k].Value != input.Options[k].Value {
+			return false
+		}
+	}
+
+	// Compare PostActionIntegration
+	if p.Integration.URL != input.Integration.URL {
+		return false
+	}
+
+	if len(p.Integration.Context) != len(input.Integration.Context) {
+		return false
+	}
+
+	for key, value := range p.Integration.Context {
+		inputValue, ok := input.Integration.Context[key]
+
+		if !ok {
+			return false
+		}
+
+		if value != inputValue {
+			return false
+		}
+	}
+
+	return true
+}
+
 // PostActionCookie is set by the server, serialized and encrypted into
 // PostAction.Cookie. The clients should hold on to it, and include it with
 // subsequent DoPostAction requests.  This allows the server to access the

model/post.go

@@ -403,6 +403,23 @@ func (o *Post) Attachments() []*SlackAttachment {
 	return ret
 }
 
+func (o *Post) AttachmentsEqual(input *Post) bool {
+	attachments := o.Attachments()
+	inputAttachments := input.Attachments()
+
+	if len(attachments) != len(inputAttachments) {
+		return false
+	}
+
+	for i := range attachments {
+		if !attachments[i].Equals(inputAttachments[i]) {
+			return false
+		}
+	}
+
+	return true
+}
+
 var markdownDestinationEscaper = strings.NewReplacer(
 	`\`, `\\`,
 	`<`, `\<`,

model/post_test.go

@@ -180,6 +180,233 @@ func TestPostSanitizeProps(t *testing.T) {
 	}
 }
 
+func TestPost_AttachmentsEqual(t *testing.T) {
+	post1 := &Post {
+	}
+	post2 := &Post {
+	}
+	for name, tc := range map[string]struct {
+		Attachments1 []*SlackAttachment
+		Attachments2 []*SlackAttachment
+		Expected bool
+	}{
+		"Empty": {
+			nil,
+			nil,
+			true,
+		},
+		"DifferentLength": {
+			[]*SlackAttachment{
+				{
+					Text: "Hello World",
+				},
+			},
+			nil,
+			false,
+		},
+		"EqualText": {
+			[]*SlackAttachment{
+				{
+					Text: "Hello World",
+				},
+			},
+			[]*SlackAttachment{
+				{
+					Text: "Hello World",
+				},
+			},
+			true,
+		},
+		"DifferentText": {
+			[]*SlackAttachment{
+				{
+					Text: "Hello World",
+				},
+			},
+			[]*SlackAttachment{
+				{
+					Text: "Hello World 2",
+				},
+			},
+			false,
+		},
+		"DifferentColor": {
+			[]*SlackAttachment{
+				{
+					Text: "Hello World",
+					Color: "#152313",
+				},
+			},
+			[]*SlackAttachment{
+				{
+					Text: "Hello World 2",
+				},
+			},
+			false,
+		},
+		"EqualFields": {
+			[]*SlackAttachment{
+				{
+					Fields: []*SlackAttachmentField {
+						{
+							Title: "Hello World",
+							Value: "FooBar",
+						},
+						{
+							Title: "Hello World2",
+							Value: "FooBar2",
+						},
+					},
+				},
+			},
+			[]*SlackAttachment{
+				{
+					Fields: []*SlackAttachmentField {
+						{
+							Title: "Hello World",
+							Value: "FooBar",
+						},
+						{
+							Title: "Hello World2",
+							Value: "FooBar2",
+						},
+					},
+				},
+			},
+			true,
+		},
+		"DifferentFields": {
+			[]*SlackAttachment{
+				{
+					Fields: []*SlackAttachmentField {
+						{
+							Title: "Hello World",
+							Value: "FooBar",
+						},
+					},
+				},
+			},
+			[]*SlackAttachment{
+				{
+					Fields: []*SlackAttachmentField {
+						{
+							Title: "Hello World",
+							Value: "FooBar",
+							Short: false,
+						},
+						{
+							Title: "Hello World2",
+							Value: "FooBar2",
+							Short: true,
+						},
+					},
+				},
+			},
+			false,
+		},
+		"EqualActions": {
+			[]*SlackAttachment{
+				{
+					Actions: []*PostAction{
+						{
+							Name: "FooBar",
+							Options: []*PostActionOptions {
+								{
+									Text: "abcdef",
+									Value: "abcdef",
+								},
+							},
+							Integration: &PostActionIntegration{
+								URL: "http://localhost",
+								Context: map[string]interface{}{
+									"context": "foobar",
+									"test": 123,
+								},
+							},
+						},
+					},
+				},
+			},
+			[]*SlackAttachment{
+				{
+					Actions: []*PostAction{
+						{
+							Name: "FooBar",
+							Options: []*PostActionOptions {
+								{
+									Text: "abcdef",
+									Value: "abcdef",
+								},
+							},
+							Integration: &PostActionIntegration{
+								URL: "http://localhost",
+								Context: map[string]interface{}{
+									"context": "foobar",
+									"test": 123,
+								},
+							},
+						},
+					},
+				},
+			},
+			true,
+		},
+		"DifferentActions": {
+			[]*SlackAttachment{
+				{
+					Actions: []*PostAction{
+						{
+							Name: "FooBar",
+							Options: []*PostActionOptions {
+								{
+									Text: "abcdef",
+									Value: "abcdef",
+								},
+							},
+							Integration: &PostActionIntegration{
+								URL: "http://localhost",
+								Context: map[string]interface{}{
+									"context": "foobar",
+									"test": "mattermost",
+								},
+							},
+						},
+					},
+				},
+			},
+			[]*SlackAttachment{
+				{
+					Actions: []*PostAction{
+						{
+							Name: "FooBar",
+							Options: []*PostActionOptions {
+								{
+									Text: "abcdef",
+									Value: "abcdef",
+								},
+							},
+							Integration: &PostActionIntegration{
+								URL: "http://localhost",
+								Context: map[string]interface{}{
+									"context": "foobar",
+									"test": 123,
+								},
+							},
+						},
+					},
+				},
+			},
+			false,
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			post1.AddProp("attachments", tc.Attachments1)
+			post2.AddProp("attachments", tc.Attachments2)
+			assert.Equal(t, tc.Expected, post1.AttachmentsEqual(post2))
+		})
+	}
+}
+
 var markdownSample, markdownSampleWithRewrittenImageURLs string
 
 func init() {

model/slack_attachment.go

@@ -30,12 +30,116 @@ type SlackAttachment struct {
 	Actions    []*PostAction           `json:"actions,omitempty"`
 }
 
+func (s *SlackAttachment) Equals(input *SlackAttachment) bool {
+	// Direct comparison of simple types
+
+	if s.Id != input.Id {
+		return false
+	}
+
+	if s.Fallback != input.Fallback {
+		return false
+	}
+
+	if s.Color != input.Color {
+		return false
+	}
+
+	if s.Pretext != input.Pretext {
+		return false
+	}
+
+	if s.AuthorName != input.AuthorName {
+		return false
+	}
+
+	if s.AuthorLink != input.AuthorLink {
+		return false
+	}
+
+	if s.AuthorIcon != input.AuthorIcon {
+		return false
+	}
+
+	if s.Title != input.Title {
+		return false
+	}
+
+	if s.TitleLink != input.TitleLink {
+		return false
+	}
+
+	if s.Text != input.Text {
+		return false
+	}
+
+	if s.ImageURL != input.ImageURL {
+		return false
+	}
+
+	if s.ThumbURL != input.ThumbURL {
+		return false
+	}
+
+	if s.Footer != input.Footer {
+		return false
+	}
+
+	if s.FooterIcon != input.FooterIcon {
+		return false
+	}
+
+	// Compare length & slice values of fields
+	if len(s.Fields) != len(input.Fields) {
+		return false
+	}
+
+	for j := range s.Fields {
+		if !s.Fields[j].Equals(input.Fields[j]) {
+			return false
+		}
+	}
+
+	// Compare length & slice values of actions
+	if len(s.Actions) != len(input.Actions) {
+		return false
+	}
+
+	for j := range s.Actions {
+		if !s.Actions[j].Equals(input.Actions[j]) {
+			return false
+		}
+	}
+
+	if s.Timestamp != input.Timestamp {
+		return false
+	}
+
+	return true
+}
+
 type SlackAttachmentField struct {
 	Title string              `json:"title"`
 	Value interface{}         `json:"value"`
 	Short SlackCompatibleBool `json:"short"`
 }
 
+func (s *SlackAttachmentField) Equals(input *SlackAttachmentField) bool {
+	if s.Title != input.Title {
+		return false
+	}
+
+	if s.Value != input.Value {
+		return false
+	}
+
+	if s.Short != input.Short {
+		return false
+	}
+
+	return true
+}
+
 func StringifySlackFieldValue(a []*SlackAttachment) []*SlackAttachment {
 	var nonNilAttachments []*SlackAttachment
 	for _, attachment := range a {

model/utils.go

@@ -36,6 +36,22 @@ type StringInterface map[string]interface{}
 type StringMap map[string]string
 type StringArray []string
 
+func (sa StringArray) Equals(input StringArray) bool {
+
+	if len(sa) != len(input) {
+		return false
+	}
+
+	for index := range sa {
+
+		if sa[index] != input[index] {
+			return false
+		}
+	}
+
+	return true
+}
+
 var translateFunc goi18n.TranslateFunc = nil
 
 func AppErrorInit(t goi18n.TranslateFunc) {

model/utils_test.go

@@ -250,6 +250,49 @@ var hashtags = map[string]string{
 	"foo#bar":         "",
 }
 
+func TestStringArray_Equal(t *testing.T) {
+	for name, tc := range map[string]struct {
+		Array1 StringArray
+		Array2 StringArray
+		Expected bool
+	}{
+		"Empty": {
+			nil,
+			nil,
+			true,
+		},
+		"EqualLength_EqualValue": {
+			StringArray{"123"},
+			StringArray{"123"},
+			true,
+		},
+		"DifferentLength": {
+			StringArray{"123"},
+			StringArray{"123", "abc"},
+			false,
+		},
+		"DifferentValues_EqualLength": {
+			StringArray{"123"},
+			StringArray{"abc"},
+			false,
+		},
+		"EqualLength_EqualValues": {
+			StringArray{"123", "abc"},
+			StringArray{"123", "abc"},
+			true,
+		},
+		"EqualLength_EqualValues_DifferentOrder": {
+			StringArray{"abc", "123"},
+			StringArray{"123", "abc"},
+			false,
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			assert.Equal(t, tc.Expected, tc.Array1.Equals(tc.Array2))
+		})
+	}
+}
+
 func TestParseHashtags(t *testing.T) {
 	for input, output := range hashtags {
 		if o, _ := ParseHashtags(input); o != output {