[bugfix] Fix no notification if mention edited into status (#4102)

This pull request adds mention notifications if a mention was edited into a status after its initial publication.

Closes https://codeberg.org/superseriousbusiness/gotosocial/issues/3869

Reviewed-on: https://codeberg.org/superseriousbusiness/gotosocial/pulls/4102
Co-authored-by: tobi <tobi.smethurst@protonmail.com>
Co-committed-by: tobi <tobi.smethurst@protonmail.com>

internal/cache/db.go

@@ -1083,6 +1083,12 @@ func (c *Caches) initMention() {
 		m2.OriginAccount = nil
 		m2.TargetAccount = nil
 
+		// Zero non-db fields.
+		m2.NameString = ""
+		m2.IsNew = false
+		m2.TargetAccountURI = ""
+		m2.TargetAccountURL = ""
+
 		return m2
 	}
 

internal/cache/size.go

@@ -549,9 +549,7 @@ func sizeofMention() uintptr {
 		OriginAccountID:  exampleURI,
 		OriginAccountURI: exampleURI,
 		TargetAccountID:  exampleID,
-		NameString:       exampleUsername,
+		Silent:           util.Ptr(false),
-		TargetAccountURI: exampleURI,
-		TargetAccountURL: exampleURI,
 	}))
 }
 

internal/db/bundb/mention.go

@@ -64,6 +64,39 @@ func (m *mentionDB) GetMention(ctx context.Context, id string) (*gtsmodel.Mentio
 	return mention, nil
 }
 
+func (m *mentionDB) GetMentionByTargetAcctStatus(
+	ctx context.Context,
+	targetAcctID string,
+	statusID string,
+) (*gtsmodel.Mention, error) {
+	// Get the status first.
+	status, err := m.state.DB.GetStatusByID(ctx, statusID)
+	if err != nil {
+		return nil, err
+	}
+
+	// Populate mentions if necessary.
+	if !status.MentionsPopulated() {
+		status.Mentions, err = m.GetMentions(ctx, status.MentionIDs)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	// See if the mention is there.
+	mention, ok := status.GetMentionByTargetID(targetAcctID)
+	if !ok {
+		return nil, db.ErrNoEntries
+	}
+
+	// Further populate the mention fields where applicable.
+	if err := m.PopulateMention(ctx, mention); err != nil {
+		return nil, err
+	}
+
+	return mention, nil
+}
+
 func (m *mentionDB) GetMentions(ctx context.Context, ids []string) ([]*gtsmodel.Mention, error) {
 	// Load all mention IDs via cache loader callbacks.
 	mentions, err := m.state.Caches.DB.Mention.LoadIDs("ID",

internal/db/mention.go

@@ -28,6 +28,9 @@ type Mention interface {
 	// GetMention gets a single mention by ID
 	GetMention(ctx context.Context, id string) (*gtsmodel.Mention, error)
 
+	// GetMentionByTargetAcctStatus returns a mention by targetAccountID and statusID.
+	GetMentionByTargetAcctStatus(ctx context.Context, targetAcctID string, statusID string) (*gtsmodel.Mention, error)
+
 	// GetMentions gets multiple mentions.
 	GetMentions(ctx context.Context, ids []string) ([]*gtsmodel.Mention, error)
 

internal/federation/dereferencing/status.go

@@ -708,6 +708,7 @@ func (d *Dereferencer) fetchStatusMentions(
 		mention.TargetAccountURL = mention.TargetAccount.URL
 		mention.StatusID = status.ID
 		mention.Status = status
+		mention.IsNew = true
 
 		// Place the new mention into the database.
 		if err := d.state.DB.PutMention(ctx, mention); err != nil {

internal/gtsmodel/mention.go

@@ -49,6 +49,12 @@ type Mention struct {
 	// This will not be put in the database, it's just for convenience.
 	NameString string `bun:"-"`
 
+	// IsNew indicates whether this mention is "new" in the sense
+	// that it has not previously been inserted into the database.
+	//
+	// This will not be put in the database, it's just for convenience.
+	IsNew bool `bun:"-"`
+
 	// TargetAccountURI is the AP ID (uri) of the user mentioned.
 	//
 	// This will not be put in the database, it's just for convenience.

internal/gtsmodel/status.go

@@ -210,6 +210,16 @@ func (s *Status) GetMentionByTargetURI(uri string) (*Mention, bool) {
 	return nil, false
 }
 
+// GetMentionByTargetID searches status for Mention{} with target ID.
+func (s *Status) GetMentionByTargetID(id string) (*Mention, bool) {
+	for _, mention := range s.Mentions {
+		if mention.TargetAccountID == id {
+			return mention, true
+		}
+	}
+	return nil, false
+}
+
 // GetMentionByUsernameDomain fetches the Mention associated with given
 // username and domains, typically extracted from a mention Namestring.
 func (s *Status) GetMentionByUsernameDomain(username, domain string) (*Mention, bool) {

internal/processing/parsemention.go

@@ -19,9 +19,11 @@ package processing
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"code.superseriousbusiness.org/gotosocial/internal/config"
+	"code.superseriousbusiness.org/gotosocial/internal/db"
 	"code.superseriousbusiness.org/gotosocial/internal/federation"
 	"code.superseriousbusiness.org/gotosocial/internal/gtscontext"
 	"code.superseriousbusiness.org/gotosocial/internal/gtsmodel"
@@ -100,7 +102,29 @@ func GetParseMentionFunc(state *state.State, federator *federation.Federator) gt
 			}
 		}
 
-		// Return mention with useful populated fields,
+		// Check if the mention was
+		// in the database already.
+		if statusID != "" {
+			mention, err := state.DB.GetMentionByTargetAcctStatus(ctx, targetAcct.ID, statusID)
+			if err != nil && !errors.Is(err, db.ErrNoEntries) {
+				return nil, fmt.Errorf(
+					"db error checking for existing mention: %w",
+					err,
+				)
+			}
+
+			if mention != nil {
+				// We had it, return this rather
+				// than creating a new one.
+				mention.NameString = namestring
+				mention.OriginAccountURI = originAcct.URI
+				mention.TargetAccountURI = targetAcct.URI
+				mention.TargetAccountURL = targetAcct.URL
+				return mention, nil
+			}
+		}
+
+		// Return new mention with useful populated fields,
 		// but *don't* store it in the database; that's
 		// up to the calling function to do, if they want.
 		return &gtsmodel.Mention{
@@ -114,6 +138,10 @@ func GetParseMentionFunc(state *state.State, federator *federation.Federator) gt
 			TargetAccountURL: targetAcct.URL,
 			TargetAccount:    targetAcct,
 			NameString:       namestring,
+
+			// Mention wasn't
+			// stored in the db.
+			IsNew: true,
 		}, nil
 	}
 }

internal/processing/workers/fromclientapi.go

@@ -729,6 +729,25 @@ func (p *clientAPI) UpdateStatus(ctx context.Context, cMsg *messages.FromClientA
 		}
 	}
 
+	// Notify any *new* mentions added
+	// to this status by the editor.
+	for _, mention := range status.Mentions {
+		// Check if we've seen
+		// this mention already.
+		if !mention.IsNew {
+			// Already seen
+			// it, skip.
+			continue
+		}
+
+		// Haven't seen this mention
+		// yet, notify it if necessary.
+		mention.Status = status
+		if err := p.surface.notifyMention(ctx, mention); err != nil {
+			log.Errorf(ctx, "error notifying mention: %v", err)
+		}
+	}
+
 	// Push message that the status has been edited to streams.
 	if err := p.surface.timelineStatusUpdate(ctx, status); err != nil {
 		log.Errorf(ctx, "error streaming status edit: %v", err)

internal/processing/workers/fromclientapi_test.go

@@ -89,6 +89,7 @@ func (suite *FromClientAPITestSuite) newStatus(
 			OriginAccountID:  account.ID,
 			OriginAccountURI: account.URI,
 			TargetAccountID:  replyToStatus.AccountID,
+			IsNew:            true,
 		}
 
 		if err := state.DB.PutMention(ctx, mention); err != nil {
@@ -117,6 +118,7 @@ func (suite *FromClientAPITestSuite) newStatus(
 			TargetAccountID:  mentionedAccount.ID,
 			TargetAccount:    mentionedAccount,
 			Silent:           util.Ptr(false),
+			IsNew:            true,
 		}
 
 		newStatus.Mentions = append(newStatus.Mentions, newMention)

internal/processing/workers/fromfediapi.go

@@ -996,7 +996,26 @@ func (p *fediAPI) UpdateStatus(ctx context.Context, fMsg *messages.FromFediAPI)
 		log.Errorf(ctx, "error streaming status edit: %v", err)
 	}
 
-	// Status representation was refetched, uncache from timelines.
+	// Notify any *new* mentions added
+	// to this status by the editor.
+	for _, mention := range status.Mentions {
+		// Check if we've seen
+		// this mention already.
+		if !mention.IsNew {
+			// Already seen
+			// it, skip.
+			continue
+		}
+
+		// Haven't seen this mention
+		// yet, notify it if necessary.
+		mention.Status = status
+		if err := p.surface.notifyMention(ctx, mention); err != nil {
+			log.Errorf(ctx, "error notifying mention: %v", err)
+		}
+	}
+
+	// Status representation changed, uncache from timelines.
 	p.surface.invalidateStatusFromTimelines(status.ID)
 
 	return nil

internal/processing/workers/surfacenotify.go

@@ -99,56 +99,77 @@ func (s *Surface) notifyMentions(
 
 	for _, mention := range status.Mentions {
 		// Set status on the mention (stops
-		// the below function populating it).
+		// notifyMention having to populate it).
 		mention.Status = status
 
-		// Beforehand, ensure the passed mention is fully populated.
+		// Do the thing.
-		if err := s.State.DB.PopulateMention(ctx, mention); err != nil {
+		if err := s.notifyMention(ctx, mention); err != nil {
-			errs.Appendf("error populating mention %s: %w", mention.ID, err)
+			errs = append(errs, err)
-			continue
-		}
-
-		if mention.TargetAccount.IsRemote() {
-			// no need to notify
-			// remote accounts.
-			continue
-		}
-
-		// Ensure thread not muted
-		// by mentioned account.
-		muted, err := s.State.DB.IsThreadMutedByAccount(
-			ctx,
-			status.ThreadID,
-			mention.TargetAccountID,
-		)
-		if err != nil {
-			errs.Appendf("error checking status thread mute %s: %w", status.ThreadID, err)
-			continue
-		}
-
-		if muted {
-			// This mentioned account
-			// has muted the thread.
-			// Don't pester them.
-			continue
-		}
-
-		// notify mentioned
-		// by status author.
-		if err := s.Notify(ctx,
-			gtsmodel.NotificationMention,
-			mention.TargetAccount,
-			mention.OriginAccount,
-			mention.StatusID,
-		); err != nil {
-			errs.Appendf("error notifying mention target %s: %w", mention.TargetAccountID, err)
-			continue
 		}
 	}
 
 	return errs.Combine()
 }
 
+// notifyMention notifies the target
+// of the given mention that they've
+// been mentioned in a status.
+func (s *Surface) notifyMention(
+	ctx context.Context,
+	mention *gtsmodel.Mention,
+) error {
+	// Beforehand, ensure the passed mention is fully populated.
+	if err := s.State.DB.PopulateMention(ctx, mention); err != nil {
+		return gtserror.Newf(
+			"error populating mention %s: %w",
+			mention.ID, err,
+		)
+	}
+
+	if mention.TargetAccount.IsRemote() {
+		// no need to notify
+		// remote accounts.
+		return nil
+	}
+
+	// Ensure thread not muted
+	// by mentioned account.
+	muted, err := s.State.DB.IsThreadMutedByAccount(
+		ctx,
+		mention.Status.ThreadID,
+		mention.TargetAccountID,
+	)
+	if err != nil {
+		return gtserror.Newf(
+			"error checking status thread mute %s: %w",
+			mention.Status.ThreadID, err,
+		)
+	}
+
+	if muted {
+		// This mentioned account
+		// has muted the thread.
+		// Don't pester them.
+		return nil
+	}
+
+	// Notify mentioned
+	// by status author.
+	if err := s.Notify(ctx,
+		gtsmodel.NotificationMention,
+		mention.TargetAccount,
+		mention.OriginAccount,
+		mention.StatusID,
+	); err != nil {
+		return gtserror.Newf(
+			"error notifying mention target %s: %w",
+			mention.TargetAccountID, err,
+		)
+	}
+
+	return nil
+}
+
 // notifyFollowRequest notifies the target of the given
 // follow request that they have a new follow request.
 func (s *Surface) notifyFollowRequest(

internal/text/goldmark_custom_renderer.go

@@ -156,7 +156,9 @@ func (cr *customRenderer) handleMention(text string) string {
 		return text
 	}
 
-	if cr.statusID != "" {
+	// Store mention if it's from a
+	// status and wasn't stored before.
+	if cr.statusID != "" && mention.IsNew {
 		if err := cr.db.PutMention(cr.ctx, mention); err != nil {
 			log.Errorf(cr.ctx, "error putting mention in db: %s", err)
 			return text