List all authors of a card diff in the notification (#2055)

server/model/notification.go

@@ -24,7 +24,6 @@ type NotificationHint struct {
 
 	// ModifiedByID is the id of the user who made the block change
 	ModifiedByID string `json:"modified_by_id"`
-	Username     string `json:"-"`
 
 	// CreatedAt is the timestamp this notification hint was created
 	// required: true

server/services/notify/notifysubscriptions/diff.go

@@ -15,9 +15,9 @@ import (
 
 // Diff represents a difference between two versions of a block.
 type Diff struct {
-	Board    *model.Block
-	Card     *model.Block
-	Username string
+	Board   *model.Block
+	Card    *model.Block
+	Authors StringMap
 
 	BlockType model.BlockType
 	OldBlock  *model.Block
@@ -76,16 +76,6 @@ func (dg *diffGenerator) generateDiffs() ([]*Diff, error) {
 		return nil, fmt.Errorf("cannot generate diff for block %s; must have a valid board and card: %w", dg.hint.BlockID, err)
 	}
 
-	user, err := dg.store.GetUserByID(dg.hint.ModifiedByID)
-	if err != nil {
-		return nil, fmt.Errorf("could not lookup user %s: %w", dg.hint.ModifiedByID, err)
-	}
-	if user != nil {
-		dg.hint.Username = user.Username
-	} else {
-		dg.hint.Username = "unknown user" // TODO: localize this when server gets i18n
-	}
-
 	// parse board's property schema here so it only happens once.
 	schema, err := model.ParsePropertySchema(dg.board)
 	if err != nil {
@@ -163,6 +153,8 @@ func (dg *diffGenerator) generateDiffsForCard(card *model.Block, schema model.Pr
 		return nil, fmt.Errorf("could not get subtree for card %s: %w", card.ID, err)
 	}
 
+	authors := make(StringMap)
+
 	// walk child blocks
 	var childDiffs []*Diff
 	for i := range blocks {
@@ -176,11 +168,14 @@ func (dg *diffGenerator) generateDiffsForCard(card *model.Block, schema model.Pr
 		}
 		if blockDiff != nil {
 			childDiffs = append(childDiffs, blockDiff)
+			authors.Append(blockDiff.Authors)
 		}
 	}
 
 	dg.logger.Debug("generateDiffsForCard",
+		mlog.Bool("has_top_changes", cardDiff != nil),
 		mlog.Int("subtree", len(blocks)),
+		mlog.Array("author_names", authors.Values()),
 		mlog.Int("child_diffs", len(childDiffs)),
 	)
 
@@ -189,7 +184,7 @@ func (dg *diffGenerator) generateDiffsForCard(card *model.Block, schema model.Pr
 			cardDiff = &Diff{
 				Board:       dg.board,
 				Card:        card,
-				Username:    dg.hint.Username,
+				Authors:     make(StringMap),
 				BlockType:   card.Type,
 				OldBlock:    card,
 				NewBlock:    card,
@@ -200,13 +195,22 @@ func (dg *diffGenerator) generateDiffsForCard(card *model.Block, schema model.Pr
 		}
 		cardDiff.Diffs = childDiffs
 	}
+	cardDiff.Authors.Append(authors)
+
 	return cardDiff, nil
 }
 
 func (dg *diffGenerator) generateDiffForBlock(newBlock *model.Block, schema model.PropSchema) (*Diff, error) {
+	dg.logger.Debug("generateDiffForBlock - new block",
+		mlog.String("block_id", newBlock.ID),
+		mlog.String("block_type", string(newBlock.Type)),
+		mlog.String("modified_by", newBlock.ModifiedBy),
+		mlog.Int64("update_at", newBlock.UpdateAt),
+	)
+
 	// find the version of the block as it was at the time of last notify.
 	opts := model.QueryBlockHistoryOptions{
-		BeforeUpdateAt: dg.lastNotifyAt,
+		BeforeUpdateAt: dg.lastNotifyAt + 1,
 		Limit:          1,
 		Descending:     true,
 	}
@@ -218,13 +222,52 @@ func (dg *diffGenerator) generateDiffForBlock(newBlock *model.Block, schema mode
 	var oldBlock *model.Block
 	if len(history) != 0 {
 		oldBlock = &history[0]
+
+		dg.logger.Debug("generateDiffForBlock - old block",
+			mlog.String("block_id", oldBlock.ID),
+			mlog.String("block_type", string(oldBlock.Type)),
+			mlog.Int64("before_update_at", dg.lastNotifyAt),
+			mlog.String("modified_by", oldBlock.ModifiedBy),
+			mlog.Int64("update_at", oldBlock.UpdateAt),
+		)
+	}
+
+	// find all the versions of the blocks that changed so we can gather all the author usernames.
+	opts = model.QueryBlockHistoryOptions{
+		AfterUpdateAt: dg.lastNotifyAt,
+		Descending:    true,
+	}
+	chgBlocks, err := dg.store.GetBlockHistory(dg.container, newBlock.ID, opts)
+	if err != nil {
+		return nil, fmt.Errorf("error getting block history for block %s: %w", newBlock.ID, err)
+	}
+	authors := make(StringMap)
+
+	dg.logger.Debug("generateDiffForBlock - authors",
+		mlog.Int64("after_update_at", dg.lastNotifyAt),
+		mlog.Int("history_count", len(chgBlocks)),
+	)
+
+	// have to loop through history slice because GetBlockHistory does not return pointers.
+	for _, b := range chgBlocks {
+		user, err := dg.store.GetUserByID(b.ModifiedBy)
+		if err != nil || user == nil {
+			dg.logger.Error("could not fetch username for block",
+				mlog.String("modified_by", b.ModifiedBy),
+				mlog.Err(err),
+			)
+			authors.Add(b.ModifiedBy, "unknown_user") // todo: localize this when server has i18n
+		} else {
+			authors.Add(user.ID, user.Username)
+		}
 	}
 
 	propDiffs := dg.generatePropDiffs(oldBlock, newBlock, schema)
 
 	dg.logger.Debug("generateDiffForBlock - results",
 		mlog.String("block_id", newBlock.ID),
-		mlog.Int64("before_update_at", opts.BeforeUpdateAt),
+		mlog.String("block_type", string(newBlock.Type)),
+		mlog.Array("author_names", authors.Values()),
 		mlog.Int("history_count", len(history)),
 		mlog.Int("prop_diff_count", len(propDiffs)),
 	)
@@ -232,7 +275,7 @@ func (dg *diffGenerator) generateDiffForBlock(newBlock *model.Block, schema mode
 	diff := &Diff{
 		Board:       dg.board,
 		Card:        dg.card,
-		Username:    dg.hint.Username,
+		Authors:     authors,
 		BlockType:   newBlock.Type,
 		OldBlock:    oldBlock,
 		NewBlock:    newBlock,

server/services/notify/notifysubscriptions/diff2slackattachments.go

@@ -20,9 +20,9 @@ import (
 
 const (
 	// card change notifications.
-	defAddCardNotify    = "@{{.Username}} has added the card {{.NewBlock | makeLink}}\n"
-	defModifyCardNotify = "###### @{{.Username}} has modified the card {{.Card | makeLink}}\n"
-	defDeleteCardNotify = "@{{.Username}} has deleted the card {{.Card | makeLink}}\n"
+	defAddCardNotify    = "{{.Authors | printAuthors \"unknown_user\" }} has added the card {{.NewBlock | makeLink}}\n"
+	defModifyCardNotify = "###### {{.Authors | printAuthors \"unknown_user\" }} has modified the card {{.Card | makeLink}}\n"
+	defDeleteCardNotify = "{{.Authors | printAuthors \"unknown_user\" }} has deleted the card {{.Card | makeLink}}\n"
 )
 
 var (
@@ -57,6 +57,9 @@ func getTemplate(name string, opts DiffConvOpts, def string) (*template.Template
 			"stripNewlines": func(s string) string {
 				return strings.TrimSpace(strings.ReplaceAll(s, "\n", "¶ "))
 			},
+			"printAuthors": func(empty string, authors StringMap) string {
+				return makeAuthorsList(authors, empty)
+			},
 		}
 		t.Funcs(myFuncs)
 
@@ -70,6 +73,21 @@ func getTemplate(name string, opts DiffConvOpts, def string) (*template.Template
 	return t, nil
 }
 
+func makeAuthorsList(authors StringMap, empty string) string {
+	if len(authors) == 0 {
+		return empty
+	}
+	prefix := ""
+	sb := &strings.Builder{}
+	for _, name := range authors.Values() {
+		sb.WriteString(prefix)
+		sb.WriteString("@")
+		sb.WriteString(strings.TrimSpace(name))
+		prefix = ", "
+	}
+	return sb.String()
+}
+
 // execTemplate executes the named template corresponding to the template name and language specified.
 func execTemplate(w io.Writer, name string, opts DiffConvOpts, def string, data interface{}) error {
 	t, err := getTemplate(name, opts, def)
@@ -191,7 +209,7 @@ func cardDiff2SlackAttachment(cardDiff *Diff, opts DiffConvOpts) (*mm_model.Slac
 			if format != "" {
 				attachment.Fields = append(attachment.Fields, &mm_model.SlackAttachmentField{
 					Short: false,
-					Title: "Comment",
+					Title: "Comment by " + makeAuthorsList(child.Authors, "unknown_user"), // todo:  localize this when server has i18n
 					Value: fmt.Sprintf(format, stripNewlines(block.Title)),
 				})
 			}

server/services/notify/notifysubscriptions/notifier.go

@@ -191,6 +191,11 @@ func (n *notifier) notifySubscribers(hint *model.NotificationHint) error {
 		return nil
 	}
 
+	diffAuthors := make(StringMap)
+	for _, d := range diffs {
+		diffAuthors.Append(d.Authors)
+	}
+
 	opts := DiffConvOpts{
 		Language: "en", // TODO: use correct language with i18n available on server.
 		MakeCardLink: func(block *model.Block) string {
@@ -208,11 +213,12 @@ func (n *notifier) notifySubscribers(hint *model.NotificationHint) error {
 	if len(attachments) > 0 {
 		for _, sub := range subs {
 			// don't notify the author of their own changes.
-			if sub.SubscriberID == hint.ModifiedByID {
+			authorName, isAuthor := diffAuthors[sub.SubscriberID]
+			if isAuthor && len(diffAuthors) == 1 {
 				n.logger.Debug("notifySubscribers - deliver, skipping author",
 					mlog.Any("hint", hint),
-					mlog.String("modified_by_id", hint.ModifiedByID),
-					mlog.String("modified_by_username", hint.Username),
+					mlog.String("author_id", sub.SubscriberID),
+					mlog.String("author_username", authorName),
 				)
 				continue
 			}

server/services/notify/notifysubscriptions/util.go

@@ -30,3 +30,31 @@ func getBoardDescription(board *model.Block) string {
 func stripNewlines(s string) string {
 	return strings.TrimSpace(strings.ReplaceAll(s, "\n", "¶ "))
 }
+
+type StringMap map[string]string
+
+func (sm StringMap) Add(k string, v string) {
+	sm[k] = v
+}
+
+func (sm StringMap) Append(m StringMap) {
+	for k, v := range m {
+		sm[k] = v
+	}
+}
+
+func (sm StringMap) Keys() []string {
+	keys := make([]string, 0, len(sm))
+	for k := range sm {
+		keys = append(keys, k)
+	}
+	return keys
+}
+
+func (sm StringMap) Values() []string {
+	values := make([]string, 0, len(sm))
+	for _, v := range sm {
+		values = append(values, v)
+	}
+	return values
+}

server/services/store/sqlstore/blocks.go

@@ -588,11 +588,11 @@ func (s *SQLStore) getBlockHistory(db sq.BaseRunner, c store.Container, blockID
 		OrderBy("insert_at" + order)
 
 	if opts.BeforeUpdateAt != 0 {
-		query = query.Where(sq.LtOrEq{"update_at": opts.BeforeUpdateAt})
+		query = query.Where(sq.Lt{"update_at": opts.BeforeUpdateAt})
 	}
 
 	if opts.AfterUpdateAt != 0 {
-		query = query.Where(sq.GtOrEq{"update_at": opts.AfterUpdateAt})
+		query = query.Where(sq.Gt{"update_at": opts.AfterUpdateAt})
 	}
 
 	if opts.Limit != 0 {

server/services/store/sqlstore/migrations/bindata.go

@@ -694,7 +694,7 @@ func _000015_blocks_history_no_nullsUpSql() (*asset, error) {
 		return nil, err
 	}
 
-	info := bindataFileInfo{name: "000015_blocks_history_no_nulls.up.sql", size: 3143, mode: os.FileMode(436), modTime: time.Unix(1639144692, 0)}
+	info := bindataFileInfo{name: "000015_blocks_history_no_nulls.up.sql", size: 3143, mode: os.FileMode(436), modTime: time.Unix(1639153400, 0)}
 	a := &asset{bytes: bytes, info: info}
 	return a, nil
 }
@@ -714,7 +714,7 @@ func _000016_subscriptions_tableDownSql() (*asset, error) {
 		return nil, err
 	}
 
-	info := bindataFileInfo{name: "000016_subscriptions_table.down.sql", size: 79, mode: os.FileMode(436), modTime: time.Unix(1639144681, 0)}
+	info := bindataFileInfo{name: "000016_subscriptions_table.down.sql", size: 79, mode: os.FileMode(436), modTime: time.Unix(1639153400, 0)}
 	a := &asset{bytes: bytes, info: info}
 	return a, nil
 }
@@ -734,7 +734,7 @@ func _000016_subscriptions_tableUpSql() (*asset, error) {
 		return nil, err
 	}
 
-	info := bindataFileInfo{name: "000016_subscriptions_table.up.sql", size: 618, mode: os.FileMode(436), modTime: time.Unix(1639144681, 0)}
+	info := bindataFileInfo{name: "000016_subscriptions_table.up.sql", size: 618, mode: os.FileMode(436), modTime: time.Unix(1639153400, 0)}
 	a := &asset{bytes: bytes, info: info}
 	return a, nil
 }