From 17b9a937b17b950a69d6523169209a6bb34b534c Mon Sep 17 00:00:00 2001
From: tobi <31960611+tsmethurst@users.noreply.github.com>
Date: Mon, 15 May 2023 12:52:40 +0200
Subject: [PATCH] [bugfix] Fix duplicating fields on profile edit (#1788)

* [bugfix] Fix duplicating fields on profile edit

* test non-duplicate fields
---
 internal/processing/account/update.go      |   1 +
 internal/processing/account/update_test.go | 332 ++++++++++++---------
 testrig/testmodels.go                      |   1 +
 3 files changed, 198 insertions(+), 136 deletions(-)

diff --git a/internal/processing/account/update.go b/internal/processing/account/update.go
index 0baeebb6a..d7018367d 100644
--- a/internal/processing/account/update.go
+++ b/internal/processing/account/update.go
@@ -155,6 +155,7 @@ func (p *Processor) Update(ctx context.Context, account *gtsmodel.Account, form
 		}
 
 		// Process the raw fields we stored earlier.
+		account.Fields = make([]*gtsmodel.Field, 0, len(account.FieldsRaw))
 		for _, fieldRaw := range account.FieldsRaw {
 			field := &gtsmodel.Field{}
 
diff --git a/internal/processing/account/update_test.go b/internal/processing/account/update_test.go
index 03a3d8703..87b4ebd50 100644
--- a/internal/processing/account/update_test.go
+++ b/internal/processing/account/update_test.go
@@ -24,208 +24,268 @@ import (
 	"github.com/stretchr/testify/suite"
 	"github.com/superseriousbusiness/gotosocial/internal/ap"
 	apimodel "github.com/superseriousbusiness/gotosocial/internal/api/model"
+	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
 )
 
 type AccountUpdateTestSuite struct {
 	AccountStandardTestSuite
 }
 
+func (suite *AccountStandardTestSuite) checkClientAPIChan(accountID string) {
+	msg := <-suite.fromClientAPIChan
+
+	// Profile update.
+	suite.Equal(ap.ActivityUpdate, msg.APActivityType)
+	suite.Equal(ap.ObjectProfile, msg.APObjectType)
+
+	// Correct account updated.
+	if msg.OriginAccount == nil {
+		suite.FailNow("expected msg.OriginAccount not to be nil")
+	}
+	suite.Equal(accountID, msg.OriginAccount.ID)
+}
+
 func (suite *AccountUpdateTestSuite) TestAccountUpdateSimple() {
-	testAccount := suite.testAccounts["local_account_1"]
+	testAccount := &gtsmodel.Account{}
+	*testAccount = *suite.testAccounts["local_account_1"]
 
-	locked := true
-	displayName := "new display name"
-	note := "#hello here i am!"
+	var (
+		ctx          = context.Background()
+		locked       = true
+		displayName  = "new display name"
+		note         = "#hello here i am!"
+		noteExpected = `<p><a href="http://localhost:8080/tags/hello" class="mention hashtag" rel="tag nofollow noreferrer noopener" target="_blank">#<span>hello</span></a> here i am!</p>`
+	)
 
-	form := &apimodel.UpdateCredentialsRequest{
+	// Call update function.
+	apiAccount, errWithCode := suite.accountProcessor.Update(ctx, testAccount, &apimodel.UpdateCredentialsRequest{
 		DisplayName: &displayName,
 		Locked:      &locked,
 		Note:        &note,
+	})
+	if errWithCode != nil {
+		suite.FailNow(errWithCode.Error())
 	}
 
-	// should get no error from the update function, and an api model account returned
-	apiAccount, errWithCode := suite.accountProcessor.Update(context.Background(), testAccount, form)
-	suite.NoError(errWithCode)
-	suite.NotNil(apiAccount)
-
-	// fields on the profile should be updated
+	// Returned profile should be updated.
 	suite.True(apiAccount.Locked)
 	suite.Equal(displayName, apiAccount.DisplayName)
-	suite.Equal(`<p><a href="http://localhost:8080/tags/hello" class="mention hashtag" rel="tag nofollow noreferrer noopener" target="_blank">#<span>hello</span></a> here i am!</p>`, apiAccount.Note)
+	suite.Equal(noteExpected, apiAccount.Note)
 
-	// we should have an update in the client api channel
-	msg := <-suite.fromClientAPIChan
-	suite.Equal(ap.ActivityUpdate, msg.APActivityType)
-	suite.Equal(ap.ObjectProfile, msg.APObjectType)
-	suite.NotNil(msg.OriginAccount)
-	suite.Equal(testAccount.ID, msg.OriginAccount.ID)
-	suite.Nil(msg.TargetAccount)
+	// We should have an update in the client api channel.
+	suite.checkClientAPIChan(testAccount.ID)
 
-	// fields should be updated in the database as well
-	dbAccount, err := suite.db.GetAccountByID(context.Background(), testAccount.ID)
-	suite.NoError(err)
+	// Check database model of account as well.
+	dbAccount, err := suite.db.GetAccountByID(ctx, testAccount.ID)
+	if err != nil {
+		suite.FailNow(err.Error())
+	}
 	suite.True(*dbAccount.Locked)
 	suite.Equal(displayName, dbAccount.DisplayName)
-	suite.Equal(`<p><a href="http://localhost:8080/tags/hello" class="mention hashtag" rel="tag nofollow noreferrer noopener" target="_blank">#<span>hello</span></a> here i am!</p>`, dbAccount.Note)
+	suite.Equal(noteExpected, dbAccount.Note)
 }
 
 func (suite *AccountUpdateTestSuite) TestAccountUpdateWithMention() {
-	testAccount := suite.testAccounts["local_account_1"]
+	testAccount := &gtsmodel.Account{}
+	*testAccount = *suite.testAccounts["local_account_1"]
 
 	var (
+		ctx          = context.Background()
 		locked       = true
 		displayName  = "new display name"
 		note         = "#hello here i am!\n\ngo check out @1happyturtle, they have a cool account!"
 		noteExpected = "<p><a href=\"http://localhost:8080/tags/hello\" class=\"mention hashtag\" rel=\"tag nofollow noreferrer noopener\" target=\"_blank\">#<span>hello</span></a> here i am!<br><br>go check out <span class=\"h-card\"><a href=\"http://localhost:8080/@1happyturtle\" class=\"u-url mention\" rel=\"nofollow noreferrer noopener\" target=\"_blank\">@<span>1happyturtle</span></a></span>, they have a cool account!</p>"
 	)
 
-	form := &apimodel.UpdateCredentialsRequest{
+	// Call update function.
+	apiAccount, errWithCode := suite.accountProcessor.Update(ctx, testAccount, &apimodel.UpdateCredentialsRequest{
 		DisplayName: &displayName,
 		Locked:      &locked,
 		Note:        &note,
+	})
+	if errWithCode != nil {
+		suite.FailNow(errWithCode.Error())
 	}
 
-	// should get no error from the update function, and an api model account returned
-	apiAccount, errWithCode := suite.accountProcessor.Update(context.Background(), testAccount, form)
-	suite.NoError(errWithCode)
-	suite.NotNil(apiAccount)
-
-	// fields on the profile should be updated
+	// Returned profile should be updated.
 	suite.True(apiAccount.Locked)
 	suite.Equal(displayName, apiAccount.DisplayName)
 	suite.Equal(noteExpected, apiAccount.Note)
 
-	// we should have an update in the client api channel
-	msg := <-suite.fromClientAPIChan
-	suite.Equal(ap.ActivityUpdate, msg.APActivityType)
-	suite.Equal(ap.ObjectProfile, msg.APObjectType)
-	suite.NotNil(msg.OriginAccount)
-	suite.Equal(testAccount.ID, msg.OriginAccount.ID)
-	suite.Nil(msg.TargetAccount)
+	// We should have an update in the client api channel.
+	suite.checkClientAPIChan(testAccount.ID)
 
-	// fields should be updated in the database as well
-	dbAccount, err := suite.db.GetAccountByID(context.Background(), testAccount.ID)
-	suite.NoError(err)
+	// Check database model of account as well.
+	dbAccount, err := suite.db.GetAccountByID(ctx, testAccount.ID)
+	if err != nil {
+		suite.FailNow(err.Error())
+	}
 	suite.True(*dbAccount.Locked)
 	suite.Equal(displayName, dbAccount.DisplayName)
 	suite.Equal(noteExpected, dbAccount.Note)
 }
 
 func (suite *AccountUpdateTestSuite) TestAccountUpdateWithMarkdownNote() {
-	testAccount := suite.testAccounts["local_account_1"]
+	testAccount := &gtsmodel.Account{}
+	*testAccount = *suite.testAccounts["local_account_1"]
 
-	note := "*hello* ~~here~~ i am!"
-	expectedNote := `<p><em>hello</em> <del>here</del> i am!</p>`
+	var (
+		ctx          = context.Background()
+		note         = "*hello* ~~here~~ i am!"
+		noteExpected = `<p><em>hello</em> <del>here</del> i am!</p>`
+	)
 
-	form := &apimodel.UpdateCredentialsRequest{
-		Note: &note,
+	// Set status content type of account 1 to markdown for this test.
+	testAccount.StatusContentType = "text/markdown"
+	if err := suite.db.UpdateAccount(ctx, testAccount, "status_content_type"); err != nil {
+		suite.FailNow(err.Error())
 	}
 
-	// set default post content type of account 1 to markdown
-	testAccount.StatusContentType = "text/markdown"
+	// Call update function.
+	apiAccount, errWithCode := suite.accountProcessor.Update(ctx, testAccount, &apimodel.UpdateCredentialsRequest{
+		Note: &note,
+	})
+	if errWithCode != nil {
+		suite.FailNow(errWithCode.Error())
+	}
 
-	// should get no error from the update function, and an api model account returned
-	apiAccount, errWithCode := suite.accountProcessor.Update(context.Background(), testAccount, form)
-	// reset test account to avoid breaking other tests
-	testAccount.StatusContentType = "text/plain"
-	suite.NoError(errWithCode)
-	suite.NotNil(apiAccount)
+	// Returned profile should be updated.
+	suite.Equal(noteExpected, apiAccount.Note)
 
-	// fields on the profile should be updated
-	suite.Equal(expectedNote, apiAccount.Note)
+	// We should have an update in the client api channel.
+	suite.checkClientAPIChan(testAccount.ID)
 
-	// we should have an update in the client api channel
-	msg := <-suite.fromClientAPIChan
-	suite.Equal(ap.ActivityUpdate, msg.APActivityType)
-	suite.Equal(ap.ObjectProfile, msg.APObjectType)
-	suite.NotNil(msg.OriginAccount)
-	suite.Equal(testAccount.ID, msg.OriginAccount.ID)
-	suite.Nil(msg.TargetAccount)
-
-	// fields should be updated in the database as well
-	dbAccount, err := suite.db.GetAccountByID(context.Background(), testAccount.ID)
+	// Check database model of account as well.
+	dbAccount, err := suite.db.GetAccountByID(ctx, testAccount.ID)
+	if err != nil {
+		suite.FailNow(err.Error())
+	}
 	suite.NoError(err)
-	suite.Equal(expectedNote, dbAccount.Note)
+	suite.Equal(noteExpected, dbAccount.Note)
 }
 
 func (suite *AccountUpdateTestSuite) TestAccountUpdateWithFields() {
-	testAccount := suite.testAccounts["local_account_1"]
+	testAccount := &gtsmodel.Account{}
+	*testAccount = *suite.testAccounts["local_account_1"]
 
-	updateFields := []apimodel.UpdateField{
-		{
-			Name:  func() *string { s := "favourite emoji"; return &s }(),
-			Value: func() *string { s := ":rainbow:"; return &s }(),
-		},
-		{
-			Name:  func() *string { s := "my website"; return &s }(),
-			Value: func() *string { s := "https://example.org"; return &s }(),
-		},
-	}
+	var (
+		ctx          = context.Background()
+		updateFields = []apimodel.UpdateField{
+			{
+				Name:  func() *string { s := "favourite emoji"; return &s }(),
+				Value: func() *string { s := ":rainbow:"; return &s }(),
+			},
+			{
+				Name:  func() *string { s := "my website"; return &s }(),
+				Value: func() *string { s := "https://example.org"; return &s }(),
+			},
+		}
+		fieldsExpectedRaw = []apimodel.Field{
+			{
+				Name:       "favourite emoji",
+				Value:      ":rainbow:",
+				VerifiedAt: (*string)(nil),
+			},
+			{
+				Name:       "my website",
+				Value:      "https://example.org",
+				VerifiedAt: (*string)(nil),
+			},
+		}
+		fieldsExpectedParsed = []apimodel.Field{
+			{
+				Name:       "favourite emoji",
+				Value:      ":rainbow:",
+				VerifiedAt: (*string)(nil),
+			},
+			{
+				Name:       "my website",
+				Value:      "<a href=\"https://example.org\" rel=\"nofollow noreferrer noopener\" target=\"_blank\">https://example.org</a>",
+				VerifiedAt: (*string)(nil),
+			},
+		}
+		emojisExpected = []apimodel.Emoji{
+			{
+				Shortcode:       "rainbow",
+				URL:             "http://localhost:8080/fileserver/01AY6P665V14JJR0AFVRT7311Y/emoji/original/01F8MH9H8E4VG3KDYJR9EGPXCQ.png",
+				StaticURL:       "http://localhost:8080/fileserver/01AY6P665V14JJR0AFVRT7311Y/emoji/static/01F8MH9H8E4VG3KDYJR9EGPXCQ.png",
+				VisibleInPicker: true,
+				Category:        "reactions",
+			},
+		}
+	)
 
-	form := &apimodel.UpdateCredentialsRequest{
+	apiAccount, errWithCode := suite.accountProcessor.Update(ctx, testAccount, &apimodel.UpdateCredentialsRequest{
 		FieldsAttributes: &updateFields,
+	})
+	if errWithCode != nil {
+		suite.FailNow(errWithCode.Error())
 	}
 
-	// should get no error from the update function, and an api model account returned
-	apiAccount, errWithCode := suite.accountProcessor.Update(context.Background(), testAccount, form)
+	// Returned profile should be updated.
+	suite.EqualValues(fieldsExpectedRaw, apiAccount.Source.Fields)
+	suite.EqualValues(fieldsExpectedParsed, apiAccount.Fields)
+	suite.EqualValues(emojisExpected, apiAccount.Emojis)
 
-	// reset test account to avoid breaking other tests
-	testAccount.StatusContentType = "text/plain"
-	suite.NoError(errWithCode)
-	suite.NotNil(apiAccount)
-	suite.EqualValues([]apimodel.Field{
-		{
-			Name:       "favourite emoji",
-			Value:      ":rainbow:",
-			VerifiedAt: (*string)(nil),
-		},
-		{
-			Name:       "my website",
-			Value:      "<a href=\"https://example.org\" rel=\"nofollow noreferrer noopener\" target=\"_blank\">https://example.org</a>",
-			VerifiedAt: (*string)(nil),
-		},
-	}, apiAccount.Fields)
-	suite.EqualValues([]apimodel.Field{
-		{
-			Name:       "favourite emoji",
-			Value:      ":rainbow:",
-			VerifiedAt: (*string)(nil),
-		},
-		{
-			Name:       "my website",
-			Value:      "https://example.org",
-			VerifiedAt: (*string)(nil),
-		},
-	}, apiAccount.Source.Fields)
-	suite.EqualValues([]apimodel.Emoji{
-		{
-			Shortcode:       "rainbow",
-			URL:             "http://localhost:8080/fileserver/01AY6P665V14JJR0AFVRT7311Y/emoji/original/01F8MH9H8E4VG3KDYJR9EGPXCQ.png",
-			StaticURL:       "http://localhost:8080/fileserver/01AY6P665V14JJR0AFVRT7311Y/emoji/static/01F8MH9H8E4VG3KDYJR9EGPXCQ.png",
-			VisibleInPicker: true,
-			Category:        "reactions",
-		},
-	}, apiAccount.Emojis)
+	// We should have an update in the client api channel.
+	suite.checkClientAPIChan(testAccount.ID)
 
-	// we should have an update in the client api channel
-	msg := <-suite.fromClientAPIChan
-	suite.Equal(ap.ActivityUpdate, msg.APActivityType)
-	suite.Equal(ap.ObjectProfile, msg.APObjectType)
-	suite.NotNil(msg.OriginAccount)
-	suite.Equal(testAccount.ID, msg.OriginAccount.ID)
-	suite.Nil(msg.TargetAccount)
+	// Check database model of account as well.
+	dbAccount, err := suite.db.GetAccountByID(ctx, testAccount.ID)
+	if err != nil {
+		suite.FailNow(err.Error())
+	}
+	suite.Equal(fieldsExpectedParsed[0].Name, dbAccount.Fields[0].Name)
+	suite.Equal(fieldsExpectedParsed[0].Value, dbAccount.Fields[0].Value)
+	suite.Equal(fieldsExpectedParsed[1].Name, dbAccount.Fields[1].Name)
+	suite.Equal(fieldsExpectedParsed[1].Value, dbAccount.Fields[1].Value)
+	suite.Equal(fieldsExpectedRaw[0].Name, dbAccount.FieldsRaw[0].Name)
+	suite.Equal(fieldsExpectedRaw[0].Value, dbAccount.FieldsRaw[0].Value)
+	suite.Equal(fieldsExpectedRaw[1].Name, dbAccount.FieldsRaw[1].Name)
+	suite.Equal(fieldsExpectedRaw[1].Value, dbAccount.FieldsRaw[1].Value)
+}
 
-	// fields should be updated in the database as well
-	dbAccount, err := suite.db.GetAccountByID(context.Background(), testAccount.ID)
-	suite.NoError(err)
-	suite.Equal("favourite emoji", dbAccount.Fields[0].Name)
-	suite.Equal(":rainbow:", dbAccount.Fields[0].Value)
-	suite.Equal("my website", dbAccount.Fields[1].Name)
-	suite.Equal("<a href=\"https://example.org\" rel=\"nofollow noreferrer noopener\" target=\"_blank\">https://example.org</a>", dbAccount.Fields[1].Value)
-	suite.Equal("favourite emoji", dbAccount.FieldsRaw[0].Name)
-	suite.Equal(":rainbow:", dbAccount.FieldsRaw[0].Value)
-	suite.Equal("my website", dbAccount.FieldsRaw[1].Name)
-	suite.Equal("https://example.org", dbAccount.FieldsRaw[1].Value)
+func (suite *AccountUpdateTestSuite) TestAccountUpdateNoteNotFields() {
+	// local_account_2 already has some fields set.
+	// We want to ensure that the fields don't change
+	// even if the account note is updated.
+	testAccount := &gtsmodel.Account{}
+	*testAccount = *suite.testAccounts["local_account_2"]
+
+	var (
+		ctx             = context.Background()
+		fieldsRawBefore = len(testAccount.FieldsRaw)
+		fieldsBefore    = len(testAccount.Fields)
+		note            = "#hello here i am!"
+		noteExpected    = `<p><a href="http://localhost:8080/tags/hello" class="mention hashtag" rel="tag nofollow noreferrer noopener" target="_blank">#<span>hello</span></a> here i am!</p>`
+	)
+
+	// Call update function.
+	apiAccount, errWithCode := suite.accountProcessor.Update(ctx, testAccount, &apimodel.UpdateCredentialsRequest{
+		Note: &note,
+	})
+	if errWithCode != nil {
+		suite.FailNow(errWithCode.Error())
+	}
+
+	// Returned profile should be updated.
+	suite.True(apiAccount.Locked)
+	suite.Equal(noteExpected, apiAccount.Note)
+	suite.Equal(fieldsRawBefore, len(apiAccount.Source.Fields))
+	suite.Equal(fieldsBefore, len(apiAccount.Fields))
+
+	// We should have an update in the client api channel.
+	suite.checkClientAPIChan(testAccount.ID)
+
+	// Check database model of account as well.
+	dbAccount, err := suite.db.GetAccountByID(ctx, testAccount.ID)
+	if err != nil {
+		suite.FailNow(err.Error())
+	}
+	suite.True(*dbAccount.Locked)
+	suite.Equal(noteExpected, dbAccount.Note)
+	suite.Equal(fieldsRawBefore, len(dbAccount.FieldsRaw))
+	suite.Equal(fieldsBefore, len(dbAccount.Fields))
 }
 
 func TestAccountUpdateTestSuite(t *testing.T) {
diff --git a/testrig/testmodels.go b/testrig/testmodels.go
index 6337004ff..15e204f85 100644
--- a/testrig/testmodels.go
+++ b/testrig/testmodels.go
@@ -526,6 +526,7 @@ func NewTestAccounts() map[string]*gtsmodel.Account {
 			SuspendedAt:           time.Time{},
 			HideCollections:       FalseBool(),
 			SuspensionOrigin:      "",
+			EnableRSS:             FalseBool(),
 		},
 		"remote_account_1": {
 			ID:                    "01F8MH5ZK5VRH73AKHQM6Y9VNX",