Improved diff to markdown for card notifications (#2037)

* improved text diff for notifications

* fix panic selecting insert_at

* improvements wip

* simplified v1

* improved diff to markdown

* address review comments

Co-authored-by: Mattermod <mattermod@users.noreply.github.com>

server/go.mod

@@ -19,6 +19,7 @@ require (
 	github.com/pkg/errors v0.9.1
 	github.com/prometheus/client_golang v1.11.0
 	github.com/rudderlabs/analytics-go v3.3.1+incompatible
+	github.com/sergi/go-diff v1.0.0
 	github.com/spf13/afero v1.6.0 // indirect
 	github.com/spf13/cast v1.3.1 // indirect
 	github.com/spf13/jwalterweatherman v1.1.0 // indirect

server/go.sum

@@ -808,6 +808,7 @@ github.com/scylladb/termtables v0.0.0-20191203121021-c4c0b6d42ff4/go.mod h1:C1a7
 github.com/sean-/seed v0.0.0-20170313163322-e2103e2c3529/go.mod h1:DxrIzT+xaE7yg65j358z/aeFdxmN0P9QXhEzd20vsDc=
 github.com/segmentio/backo-go v0.0.0-20200129164019-23eae7c10bd3 h1:ZuhckGJ10ulaKkdvJtiAqsLTiPrLaXSdnVgXJKJkTxE=
 github.com/segmentio/backo-go v0.0.0-20200129164019-23eae7c10bd3/go.mod h1:9/Rh6yILuLysoQnZ2oNooD2g7aBnvM7r/fNVxRNWfBc=
+github.com/sergi/go-diff v1.0.0 h1:Kpca3qRNrduNnOQeazBd0ysaKrUJiIuISHxogkT9RPQ=
 github.com/sergi/go-diff v1.0.0/go.mod h1:0CfEIISq7TuYL3j771MWULgwwjU+GofnZX9QAmXWZgo=
 github.com/shopspring/decimal v0.0.0-20180709203117-cd690d0c9e24/go.mod h1:M+9NzErvs504Cn4c5DxATwIqPbtswREoFCre64PpcG4=
 github.com/shurcooL/component v0.0.0-20170202220835-f88ec8f54cc4/go.mod h1:XhFIlyj5a1fBNx5aJTbKoIq0mNaPvOagO+HjB3EtxrY=

server/services/notify/notifysubscriptions/diff2markdown.go

@@ -0,0 +1,181 @@
+package notifysubscriptions
+
+import (
+	"strings"
+
+	"github.com/sergi/go-diff/diffmatchpatch"
+
+	"github.com/mattermost/mattermost-server/v6/shared/mlog"
+)
+
+func generateMarkdownDiff(oldText string, newText string, logger *mlog.Logger) string {
+	oldTxtNorm := normalizeText(oldText)
+	newTxtNorm := normalizeText(newText)
+
+	dmp := diffmatchpatch.New()
+
+	diffs := dmp.DiffMain(oldTxtNorm, newTxtNorm, false)
+
+	diffs = dmp.DiffCleanupSemantic(diffs)
+	diffs = dmp.DiffCleanupEfficiency(diffs)
+
+	// check there is at least one insert or delete
+	var editFound bool
+	for _, d := range diffs {
+		if (d.Type == diffmatchpatch.DiffInsert || d.Type == diffmatchpatch.DiffDelete) && strings.TrimSpace(d.Text) != "" {
+			editFound = true
+			break
+		}
+	}
+
+	if !editFound {
+		logger.Debug("skipping notification for superficial diff")
+		return ""
+	}
+
+	cfg := markDownCfg{
+		insertOpen:  "`",
+		insertClose: "`",
+		deleteOpen:  "~~`",
+		deleteClose: "`~~",
+	}
+	markdown := generateMarkdown(diffs, cfg)
+	markdown = strings.ReplaceAll(markdown, "¶", "\n")
+
+	return markdown
+}
+
+const (
+	truncLenEquals  = 60
+	truncLenInserts = 120
+	truncLenDeletes = 80
+)
+
+type markDownCfg struct {
+	insertOpen  string
+	insertClose string
+	deleteOpen  string
+	deleteClose string
+}
+
+func generateMarkdown(diffs []diffmatchpatch.Diff, cfg markDownCfg) string {
+	sb := &strings.Builder{}
+
+	var first, last bool
+
+	for i, diff := range diffs {
+		first = i == 0
+		last = i == len(diffs)-1
+
+		switch diff.Type {
+		case diffmatchpatch.DiffInsert:
+			sb.WriteString(cfg.insertOpen)
+			sb.WriteString(truncate(diff.Text, truncLenInserts, first, last))
+			sb.WriteString(cfg.insertClose)
+
+		case diffmatchpatch.DiffDelete:
+			sb.WriteString(cfg.deleteOpen)
+			sb.WriteString(truncate(diff.Text, truncLenDeletes, first, last))
+			sb.WriteString(cfg.deleteClose)
+
+		case diffmatchpatch.DiffEqual:
+			sb.WriteString(truncate(diff.Text, truncLenEquals, first, last))
+		}
+	}
+	return sb.String()
+}
+
+func truncate(s string, maxLen int, first bool, last bool) string {
+	if len(s) < maxLen {
+		return s
+	}
+
+	var result string
+
+	switch {
+	case first:
+		// truncate left
+		result = " ... " + rightWords(s, maxLen)
+	case last:
+		// truncate right
+		result = leftWords(s, maxLen) + " ... "
+	default:
+		// truncate in the middle
+		half := len(s) / 2
+
+		left := leftWords(s[:half], maxLen/2)
+		right := rightWords(s[half:], maxLen/2)
+
+		result = left + " ... " + right
+	}
+
+	return strings.ReplaceAll(result, "¶", "↩")
+}
+
+func normalizeText(s string) string {
+	s = strings.ReplaceAll(s, "\t", " ")
+	s = strings.ReplaceAll(s, "  ", " ")
+	s = strings.ReplaceAll(s, "\n\n", "\n")
+	s = strings.ReplaceAll(s, "\n", "¶")
+	return s
+}
+
+// leftWords returns approximately maxLen characters from the left part of the source string by truncating on the right,
+// with best effort to include whole words.
+func leftWords(s string, maxLen int) string {
+	if len(s) < maxLen {
+		return s
+	}
+	fields := strings.Fields(s)
+	fields = words(fields, maxLen)
+
+	return strings.Join(fields, " ")
+}
+
+// rightWords returns approximately maxLen from the right part of the source string by truncating from the left,
+// with best effort to include whole words.
+func rightWords(s string, maxLen int) string {
+	if len(s) < maxLen {
+		return s
+	}
+	fields := strings.Fields(s)
+
+	// reverse the fields so that the right-most words end up at the beginning.
+	reverse(fields)
+
+	fields = words(fields, maxLen)
+
+	// reverse the fields again so that the original order is restored.
+	reverse(fields)
+
+	return strings.Join(fields, " ")
+}
+
+func reverse(ss []string) {
+	ssLen := len(ss)
+	for i := 0; i < ssLen/2; i++ {
+		ss[i], ss[ssLen-i-1] = ss[ssLen-i-1], ss[i]
+	}
+}
+
+// words returns a subslice containing approximately maxChars of characters. The last item may be truncated.
+func words(words []string, maxChars int) []string {
+	var count int
+	result := make([]string, 0, len(words))
+
+	for i, w := range words {
+		wordLen := len(w)
+		if wordLen+count > maxChars {
+			switch {
+			case i == 0:
+				result = append(result, w[:maxChars])
+			case wordLen < 8:
+				result = append(result, w)
+			}
+			return result
+		}
+		count += wordLen
+		result = append(result, w)
+	}
+	return result
+}

server/services/notify/notifysubscriptions/diff2markdown_test.go

@@ -0,0 +1,26 @@
+package notifysubscriptions
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_reverse(t *testing.T) {
+	tests := []struct {
+		name string
+		ss   []string
+		want []string
+	}{
+		{name: "even", ss: []string{"one", "two", "three", "four"}, want: []string{"four", "three", "two", "one"}},
+		{name: "odd", ss: []string{"one", "two", "three"}, want: []string{"three", "two", "one"}},
+		{name: "one", ss: []string{"one"}, want: []string{"one"}},
+		{name: "empty", ss: []string{}, want: []string{}},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			reverse(tt.ss)
+			assert.Equal(t, tt.want, tt.ss)
+		})
+	}
+}

server/services/notify/notifysubscriptions/diff2slackattachments.go

@@ -15,6 +15,7 @@ import (
 	"github.com/wiggin77/merror"
 
 	mm_model "github.com/mattermost/mattermost-server/v6/model"
+	"github.com/mattermost/mattermost-server/v6/shared/mlog"
 )
 
 const (
@@ -34,6 +35,7 @@ var (
 type DiffConvOpts struct {
 	Language     string
 	MakeCardLink func(block *model.Block) string
+	Logger       *mlog.Logger
 }
 
 // getTemplate returns a new or cached named template based on the language specified.
@@ -90,6 +92,9 @@ func Diffs2SlackAttachments(diffs []*Diff, opts DiffConvOpts) ([]*mm_model.Slack
 				merr.Append(err)
 				continue
 			}
+			if a == nil {
+				continue
+			}
 			attachments = append(attachments, a)
 		}
 	}
@@ -140,7 +145,7 @@ func cardDiff2SlackAttachment(cardDiff *Diff, opts DiffConvOpts) (*mm_model.Slac
 		attachment.Fields = append(attachment.Fields, &mm_model.SlackAttachmentField{
 			Short: false,
 			Title: "Title",
-			Value: fmt.Sprintf("%s  ~~`%s`~~", cardDiff.NewBlock.Title, cardDiff.OldBlock.Title),
+			Value: fmt.Sprintf("%s  ~~`%s`~~", stripNewlines(cardDiff.NewBlock.Title), stripNewlines(cardDiff.OldBlock.Title)),
 		})
 	}
 
@@ -153,7 +158,7 @@ func cardDiff2SlackAttachment(cardDiff *Diff, opts DiffConvOpts) (*mm_model.Slac
 
 			var val string
 			if propDiff.OldValue != "" {
-				val = fmt.Sprintf("%s  ~~`%s`~~", propDiff.NewValue, propDiff.OldValue)
+				val = fmt.Sprintf("%s  ~~`%s`~~", stripNewlines(propDiff.NewValue), stripNewlines(propDiff.OldValue))
 			} else {
 				val = propDiff.NewValue
 			}
@@ -208,33 +213,21 @@ func cardDiff2SlackAttachment(cardDiff *Diff, opts DiffConvOpts) (*mm_model.Slac
 				continue
 			}
 
-			/*
-				TODO: use diff lib for content changes which can be many paragraphs.
-				      Unfortunately `github.com/sergi/go-diff` is not suitable for
-					  markdown display. An alternate markdown friendly lib is being
-					  worked on at github.com/wiggin77/go-difflib and will be substituted
-					  here when ready.
-
-				newTxt := cleanBlockTitle(child.NewBlock)
-				oldTxt := cleanBlockTitle(child.OldBlock)
-
-				dmp := diffmatchpatch.New()
-				txtDiffs := dmp.DiffMain(oldTxt, newTxt, true)
-
-				_, _ = w.Write([]byte(dmp.DiffPrettyText(txtDiffs)))
-
-			*/
-
-			if oldTitle != "" {
-				oldTitle = fmt.Sprintf("\n~~`%s`~~", oldTitle)
+			markdown := generateMarkdownDiff(oldTitle, newTitle, opts.Logger)
+			if markdown == "" {
+				continue
 			}
 
 			attachment.Fields = append(attachment.Fields, &mm_model.SlackAttachmentField{
 				Short: false,
 				Title: "Description",
-				Value: newTitle + oldTitle,
+				Value: markdown,
 			})
 		}
 	}
+
+	if len(attachment.Fields) == 0 {
+		return nil, nil
+	}
 	return attachment, nil
 }

server/services/notify/notifysubscriptions/notifier.go

@@ -196,6 +196,7 @@ func (n *notifier) notifySubscribers(hint *model.NotificationHint) error {
 		MakeCardLink: func(block *model.Block) string {
 			return fmt.Sprintf("[%s](%s)", block.Title, utils.MakeCardLink(n.serverRoot, board.WorkspaceID, board.ID, card.ID))
 		},
+		Logger: n.logger,
 	}
 
 	attachments, err := Diffs2SlackAttachments(diffs, opts)
@@ -204,28 +205,35 @@ func (n *notifier) notifySubscribers(hint *model.NotificationHint) error {
 	}
 
 	merr := merror.New()
-	for _, sub := range subs {
-		// don't notify the author of their own changes.
-		if sub.SubscriberID == hint.ModifiedByID {
-			n.logger.Debug("notifySubscribers - deliver, skipping author",
+	if len(attachments) > 0 {
+		for _, sub := range subs {
+			// don't notify the author of their own changes.
+			if sub.SubscriberID == hint.ModifiedByID {
+				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),
+				)
+				continue
+			}
+
+			n.logger.Debug("notifySubscribers - deliver",
 				mlog.Any("hint", hint),
 				mlog.String("modified_by_id", hint.ModifiedByID),
-				mlog.String("modified_by_username", hint.Username),
+				mlog.String("subscriber_id", sub.SubscriberID),
+				mlog.String("subscriber_type", string(sub.SubscriberType)),
 			)
-			continue
-		}
 
-		n.logger.Debug("notifySubscribers - deliver",
+			if err = n.delivery.SubscriptionDeliverSlackAttachments(hint.WorkspaceID, sub.SubscriberID, sub.SubscriberType, attachments); err != nil {
+				merr.Append(fmt.Errorf("cannot deliver notification to subscriber %s [%s]: %w",
+					sub.SubscriberID, sub.SubscriberType, err))
+			}
+		}
+	} else {
+		n.logger.Debug("notifySubscribers - skip delivery; no chg",
 			mlog.Any("hint", hint),
 			mlog.String("modified_by_id", hint.ModifiedByID),
-			mlog.String("subscriber_id", sub.SubscriberID),
-			mlog.String("subscriber_type", string(sub.SubscriberType)),
 		)
-
-		if err = n.delivery.SubscriptionDeliverSlackAttachments(hint.WorkspaceID, sub.SubscriberID, sub.SubscriberType, attachments); err != nil {
-			merr.Append(fmt.Errorf("cannot deliver notification to subscriber %s [%s]: %w",
-				sub.SubscriberID, sub.SubscriberType, err))
-		}
 	}
 
 	// find the new NotifiedAt based on the newest diff.