diff --git a/server/model/notification.go b/server/model/notification.go index 3426cb09d..2ab80e0f2 100644 --- a/server/model/notification.go +++ b/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 diff --git a/server/services/notify/notifysubscriptions/diff.go b/server/services/notify/notifysubscriptions/diff.go index f4f9d7adf..a7a8e01d7 100644 --- a/server/services/notify/notifysubscriptions/diff.go +++ b/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, diff --git a/server/services/notify/notifysubscriptions/diff2slackattachments.go b/server/services/notify/notifysubscriptions/diff2slackattachments.go index 751d83a3f..ec4ee56e9 100644 --- a/server/services/notify/notifysubscriptions/diff2slackattachments.go +++ b/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)), }) } diff --git a/server/services/notify/notifysubscriptions/notifier.go b/server/services/notify/notifysubscriptions/notifier.go index 61625a97b..1ba7b053c 100644 --- a/server/services/notify/notifysubscriptions/notifier.go +++ b/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 } diff --git a/server/services/notify/notifysubscriptions/util.go b/server/services/notify/notifysubscriptions/util.go index 800640a63..bc1449ce4 100644 --- a/server/services/notify/notifysubscriptions/util.go +++ b/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 +} diff --git a/server/services/store/sqlstore/blocks.go b/server/services/store/sqlstore/blocks.go index d6794297a..f77479cf3 100644 --- a/server/services/store/sqlstore/blocks.go +++ b/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 { diff --git a/server/services/store/sqlstore/migrations/bindata.go b/server/services/store/sqlstore/migrations/bindata.go index 81f0ad65f..76593d234 100644 --- a/server/services/store/sqlstore/migrations/bindata.go +++ b/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 }