GH-3039 - Backport notify fixes (#3045)

* Don't warn when  appears in card followed by text that is not a username (#2747)
* ensure bot is member of team when notifiying
* fix notify subscriber

.gitignore

@@ -45,6 +45,7 @@ __debug_bin
 files
 octo*.db
 focalboard*.db
+*.boardarchive
 .eslintcache
 .vscode/settings.json
 # config.json is copied from app-config.json in the Makefile
@@ -66,3 +67,5 @@ mattermost-plugin/dist
 .idea
 docker/certs
 docker/data
+server/**/*.coverage
+mattermost-plugin/**/*.coverage

mattermost-plugin/server/notifications.go

@@ -115,3 +115,7 @@ func (da *pluginAPIAdapter) GetChannelMember(channelID string, userID string) (*
 func (da *pluginAPIAdapter) IsErrNotFound(err error) bool {
 	return errors.Is(err, pluginapi.ErrNotFound)
 }
+
+func (da *pluginAPIAdapter) CreateMember(teamID string, userID string) (*model.TeamMember, error) {
+	return da.client.Team.CreateMember(teamID, userID)
+}

server/services/notify/notifymentions/mentions_backend.go

@@ -110,6 +110,14 @@ func (b *Backend) BlockChanged(evt notify.BlockChangeEvent) error {
 			merr.Append(fmt.Errorf("cannot deliver notification for @%s: %w", username, err))
 		}
 
+		if userID == "" {
+			// was a `@` followed by something other than a username.
+			b.logger.Debug("Mention notification skipped; not a known username",
+				mlog.String("username", username),
+			)
+			continue
+		}
+
 		b.logger.Debug("Mention notification delivered",
 			mlog.String("user", username),
 			mlog.Int("listener_count", len(listeners)),

server/services/notify/notifysubscriptions/delivery.go

@@ -12,6 +12,8 @@ import (
 // SubscriptionDelivery provides an interface for delivering subscription notifications to other systems, such as
 // channels server via plugin API.
 type SubscriptionDelivery interface {
-	SubscriptionDeliverSlackAttachments(workspaceID string, subscriberID string, subscriberType model.SubscriberType,
+	SubscriptionDeliverSlackAttachments(workspaceID string, teamID string, subscriberID string, subscriberType model.SubscriberType,
 		attachments []*mm_model.SlackAttachment) error
+
+	GetTeamIDForWorkspace(workspaceID string) (string, error)
 }

server/services/notify/notifysubscriptions/notifier.go

@@ -167,6 +167,12 @@ func (n *notifier) notifySubscribers(hint *model.NotificationHint) error {
 		return fmt.Errorf("could not get board & card for block %s: %w", hint.BlockID, err)
 	}
 
+	// need the team ID
+	teamID, err := n.delivery.GetTeamIDForWorkspace(c.WorkspaceID)
+	if err != nil {
+		return fmt.Errorf("could not get team ID for workspace %s: %w", c.WorkspaceID, err)
+	}
+
 	n.logger.Debug("notifySubscribers - subscribers",
 		mlog.Any("hint", hint),
 		mlog.String("board_id", board.ID),
@@ -236,7 +242,7 @@ func (n *notifier) notifySubscribers(hint *model.NotificationHint) error {
 				mlog.String("subscriber_type", string(sub.SubscriberType)),
 			)
 
-			if err = n.delivery.SubscriptionDeliverSlackAttachments(hint.WorkspaceID, sub.SubscriberID, sub.SubscriberType, attachments); err != nil {
+			if err = n.delivery.SubscriptionDeliverSlackAttachments(hint.WorkspaceID, teamID, sub.SubscriberID, sub.SubscriberType, attachments); err != nil {
 				merr.Append(fmt.Errorf("cannot deliver notification to subscriber %s [%s]: %w",
 					sub.SubscriberID, sub.SubscriberType, err))
 			}

server/services/notify/notifysubscriptions/subscriptions_backend.go

@@ -123,7 +123,7 @@ func (b *Backend) BlockChanged(evt notify.BlockChangeEvent) error {
 	if err != nil {
 		merr.Append(fmt.Errorf("cannot fetch subscribers for board %s: %w", evt.Board.ID, err))
 	}
-	if err = b.notifySubscribers(subs, evt.Board, evt.ModifiedByID); err != nil {
+	if err = b.notifySubscribers(c, subs, evt.Board.ID, model.TypeBoard, evt.ModifiedByID); err != nil {
 		merr.Append(fmt.Errorf("cannot notify board subscribers for board %s: %w", evt.Board.ID, err))
 	}
 
@@ -136,7 +136,7 @@ func (b *Backend) BlockChanged(evt notify.BlockChangeEvent) error {
 	if err != nil {
 		merr.Append(fmt.Errorf("cannot fetch subscribers for card %s: %w", evt.Card.ID, err))
 	}
-	if err = b.notifySubscribers(subs, evt.Card, evt.ModifiedByID); err != nil {
+	if err = b.notifySubscribers(c, subs, evt.Card.ID, model.TypeCard, evt.ModifiedByID); err != nil {
 		merr.Append(fmt.Errorf("cannot notify card subscribers for card %s: %w", evt.Card.ID, err))
 	}
 
@@ -146,7 +146,7 @@ func (b *Backend) BlockChanged(evt notify.BlockChangeEvent) error {
 		if err != nil {
 			merr.Append(fmt.Errorf("cannot fetch subscribers for block %s: %w", evt.BlockChanged.ID, err))
 		}
-		if err := b.notifySubscribers(subs, evt.BlockChanged, evt.ModifiedByID); err != nil {
+		if err := b.notifySubscribers(c, subs, evt.BlockChanged.ID, evt.BlockChanged.Type, evt.ModifiedByID); err != nil {
 			merr.Append(fmt.Errorf("cannot notify block subscribers for block %s: %w", evt.BlockChanged.ID, err))
 		}
 	}
@@ -154,24 +154,27 @@ func (b *Backend) BlockChanged(evt notify.BlockChangeEvent) error {
 }
 
 // notifySubscribers triggers a change notification for subscribers by writing a notification hint to the database.
-func (b *Backend) notifySubscribers(subs []*model.Subscriber, block *model.Block, modifiedByID string) error {
+func (b *Backend) notifySubscribers(c store.Container, subs []*model.Subscriber, blockID string, idType model.BlockType, modifiedByID string) error {
 	if len(subs) == 0 {
 		return nil
 	}
 
 	hint := &model.NotificationHint{
-		BlockType:    block.Type,
-		BlockID:      block.ID,
-		WorkspaceID:  block.WorkspaceID,
+		WorkspaceID:  c.WorkspaceID,
+		BlockType:    idType,
+		BlockID:      blockID,
 		ModifiedByID: modifiedByID,
 	}
 
-	hint, err := b.store.UpsertNotificationHint(hint, b.getBlockUpdateFreq(block.Type))
+	hint, err := b.store.UpsertNotificationHint(hint, b.getBlockUpdateFreq(idType))
 	if err != nil {
 		return fmt.Errorf("cannot upsert notification hint: %w", err)
 	}
+	if err := b.notifier.onNotifyHint(hint); err != nil {
+		return err
+	}
 
-	return b.notifier.onNotifyHint(hint)
+	return nil
 }
 
 // OnMention satisfies the `MentionListener` interface and is called whenever a @mention notification

server/services/notify/plugindelivery/mention_deliver.go

@@ -15,7 +15,7 @@ import (
 // MentionDeliver notifies a user they have been mentioned in a block.
 func (pd *PluginDelivery) MentionDeliver(mentionUsername string, extract string, evt notify.BlockChangeEvent) (string, error) {
 	// determine which team the workspace is associated with
-	teamID, err := pd.getTeamID(evt)
+	teamID, err := pd.GetTeamIDForWorkspace(evt.Workspace)
 	if err != nil {
 		return "", fmt.Errorf("cannot determine teamID for block change notification: %w", err)
 	}
@@ -45,7 +45,7 @@ func (pd *PluginDelivery) MentionDeliver(mentionUsername string, extract string,
 		return "", fmt.Errorf("cannot find user: %w", err)
 	}
 
-	channel, err := pd.api.GetDirectChannel(member.UserId, pd.botID)
+	channel, err := pd.getDirectChannel(teamID, member.UserId, pd.botID)
 	if err != nil {
 		return "", fmt.Errorf("cannot get direct channel: %w", err)
 	}

server/services/notify/plugindelivery/plugin_delivery.go

@@ -4,8 +4,6 @@
 package plugindelivery
 
 import (
-	"github.com/mattermost/focalboard/server/services/notify"
-
 	mm_model "github.com/mattermost/mattermost-server/v6/model"
 )
 
@@ -35,6 +33,9 @@ type PluginAPI interface {
 	// IsErrNotFound returns true if `err` or one of its wrapped children are the `ErrNotFound`
 	// as defined in the plugin API.
 	IsErrNotFound(err error) bool
+	// CreateMember adds a user to the specified team. Safe to call if the user is
+	// already a member of the team.
+	CreateMember(teamID string, userID string) (*mm_model.TeamMember, error)
 }
 
 // PluginDelivery provides ability to send notifications to direct message channels via Mattermost plugin API.
@@ -44,6 +45,7 @@ type PluginDelivery struct {
 	api        PluginAPI
 }
 
+// New creates a PluginDelivery instance.
 func New(botID string, serverRoot string, api PluginAPI) *PluginDelivery {
 	return &PluginDelivery{
 		botID:      botID,
@@ -52,9 +54,9 @@ func New(botID string, serverRoot string, api PluginAPI) *PluginDelivery {
 	}
 }
 
-func (pd *PluginDelivery) getTeamID(evt notify.BlockChangeEvent) (string, error) {
+func (pd *PluginDelivery) GetTeamIDForWorkspace(workspaceID string) (string, error) {
 	// for now, the workspace ID is also the channel ID
-	channel, err := pd.api.GetChannelByID(evt.Workspace)
+	channel, err := pd.api.GetChannelByID(workspaceID)
 	if err != nil {
 		return "", err
 	}

server/services/notify/plugindelivery/subscription_deliver.go

@@ -17,8 +17,8 @@ var (
 )
 
 // SubscriptionDeliverSlashAttachments notifies a user that changes were made to a block they are subscribed to.
-func (pd *PluginDelivery) SubscriptionDeliverSlackAttachments(workspaceID string, subscriberID string, subscriptionType model.SubscriberType,
-	attachments []*mm_model.SlackAttachment) error {
+func (pd *PluginDelivery) SubscriptionDeliverSlackAttachments(workspaceID string, teamID string, subscriberID string,
+	subscriptionType model.SubscriberType, attachments []*mm_model.SlackAttachment) error {
 	// check subscriber is member of channel
 	_, err := pd.api.GetChannelMember(workspaceID, subscriberID)
 	if err != nil {
@@ -29,7 +29,7 @@ func (pd *PluginDelivery) SubscriptionDeliverSlackAttachments(workspaceID string
 		return fmt.Errorf("cannot fetch channel member for user %s: %w", subscriberID, err)
 	}
 
-	channelID, err := pd.getDirectChannelID(subscriberID, subscriptionType, pd.botID)
+	channelID, err := pd.getDirectChannelID(teamID, subscriberID, subscriptionType, pd.botID)
 	if err != nil {
 		return err
 	}
@@ -44,14 +44,14 @@ func (pd *PluginDelivery) SubscriptionDeliverSlackAttachments(workspaceID string
 	return pd.api.CreatePost(post)
 }
 
-func (pd *PluginDelivery) getDirectChannelID(subscriberID string, subscriberType model.SubscriberType, botID string) (string, error) {
+func (pd *PluginDelivery) getDirectChannelID(teamID string, subscriberID string, subscriberType model.SubscriberType, botID string) (string, error) {
 	switch subscriberType {
 	case model.SubTypeUser:
 		user, err := pd.api.GetUserByID(subscriberID)
 		if err != nil {
 			return "", fmt.Errorf("cannot find user: %w", err)
 		}
-		channel, err := pd.api.GetDirectChannel(user.Id, botID)
+		channel, err := pd.getDirectChannel(teamID, user.Id, botID)
 		if err != nil {
 			return "", fmt.Errorf("cannot get direct channel: %w", err)
 		}
@@ -62,3 +62,12 @@ func (pd *PluginDelivery) getDirectChannelID(subscriberID string, subscriberType
 		return "", ErrUnsupportedSubscriberType
 	}
 }
+
+func (pd *PluginDelivery) getDirectChannel(teamID string, userID string, botID string) (*mm_model.Channel, error) {
+	// first ensure the bot is a member of the team.
+	_, err := pd.api.CreateMember(teamID, botID)
+	if err != nil {
+		return nil, fmt.Errorf("cannot add bot to team %s: %w", teamID, err)
+	}
+	return pd.api.GetDirectChannel(userID, botID)
+}

server/services/notify/plugindelivery/user_test.go

@@ -153,3 +153,11 @@ type ErrNotFound struct{}
 func (e ErrNotFound) Error() string {
 	return "not found"
 }
+
+func (m pluginAPIMock) CreateMember(teamID string, userID string) (*mm_model.TeamMember, error) {
+	member := &mm_model.TeamMember{
+		UserId: userID,
+		TeamId: teamID,
+	}
+	return member, nil
+}