From 33715e7ece0cba10fbc68f029ff2c51e016bb187 Mon Sep 17 00:00:00 2001
From: Arthur Shvarts <ashvarts@gmail.com>
Date: Wed, 23 Oct 2024 09:34:40 -0400
Subject: [PATCH] Improve promotion comment. (#31)

* Improve promotion comment.

Promotion comment used to list all the values in the 'targetPaths' key,
ignoring the promotionAllow and promotionBlock lists. This PR limits
the comment to only paths that are being promoted.

* Use testify library for assert

* return nil, use assert

* Add t.Parallel()
---
 internal/pkg/githubapi/github.go              | 28 +++++++-
 internal/pkg/githubapi/github_test.go         | 72 +++++++++++++++++++
 .../pkg/githubapi/testdata/pr_body.golden.md  |  1 -
 3 files changed, 99 insertions(+), 2 deletions(-)

diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go
index 29430cb3..ec533e2d 100644
--- a/internal/pkg/githubapi/github.go
+++ b/internal/pkg/githubapi/github.go
@@ -1109,7 +1109,7 @@ func prBody(keys []int, newPrMetadata prMetadata, newPrBody string) string {
 
 	for i, k := range keys {
 		sp = newPrMetadata.PreviousPromotionMetadata[k].SourcePath
-		x := newPrMetadata.PreviousPromotionMetadata[k].TargetPaths
+		x := identifyCommonPaths(newPrMetadata.PromotedPaths, newPrMetadata.PreviousPromotionMetadata[k].TargetPaths)
 		tp = strings.Join(x, fmt.Sprintf("`  \n%s`", strings.Repeat(mkTab, i+1)))
 		newPrBody = newPrBody + fmt.Sprintf("%s↘️  #%d  `%s` ➡️  \n%s`%s`  \n", strings.Repeat(mkTab, i), k, sp, strings.Repeat(mkTab, i+1), tp)
 	}
@@ -1117,6 +1117,32 @@ func prBody(keys []int, newPrMetadata prMetadata, newPrBody string) string {
 	return newPrBody
 }
 
+// identifyCommonPaths takes a slice of promotion paths and target paths and
+// returns a slice containing paths in common.
+func identifyCommonPaths(promotionPaths []string, targetPaths []string) []string {
+	if (len(promotionPaths) == 0) || (len(targetPaths) == 0) {
+		return nil
+	}
+	var commonPaths []string
+	for _, pp := range promotionPaths {
+		if pp == "" {
+			continue
+		}
+		for _, tp := range targetPaths {
+			if tp == "" {
+				continue
+			}
+			// strings.HasPrefix is used to check that the target path and promotion path match instead of
+			// using 'pp ==  tp' because the promotion path is targetPath + component.
+			if strings.HasPrefix(pp, tp) {
+				commonPaths = append(commonPaths, tp)
+			}
+		}
+	}
+
+	return commonPaths
+}
+
 func createPrObject(ghPrClientDetails GhPrClientDetails, newBranchRef string, newPrTitle string, newPrBody string, defaultBranch string, assignee string) (*github.PullRequest, error) {
 	newPrConfig := &github.NewPullRequest{
 		Body:  github.String(newPrBody),
diff --git a/internal/pkg/githubapi/github_test.go b/internal/pkg/githubapi/github_test.go
index 8284258b..1a5d40d8 100644
--- a/internal/pkg/githubapi/github_test.go
+++ b/internal/pkg/githubapi/github_test.go
@@ -235,6 +235,9 @@ func TestPrBody(t *testing.T) {
 	t.Parallel()
 	keys := []int{1, 2, 3}
 	newPrMetadata := prMetadata{
+		// note: "targetPath3" is missing from the list of promoted paths, so it should not
+		// be included in the new PR body.
+		PromotedPaths: []string{"targetPath1", "targetPath2", "targetPath4", "targetPath5", "targetPath6"},
 		PreviousPromotionMetadata: map[int]promotionInstanceMetaData{
 			1: {
 				SourcePath:  "sourcePath1",
@@ -338,3 +341,72 @@ func TestCommitStatusTargetURL(t *testing.T) {
 		})
 	}
 }
+
+func Test_identifyCommonPaths(t *testing.T) {
+	t.Parallel()
+	type args struct {
+		promoPaths  []string
+		targetPaths []string
+	}
+	tests := []struct {
+		name string
+		args args
+		want []string
+	}{
+		{
+			name: "same paths",
+			args: args{
+				promoPaths:  []string{"path1/component/path", "path2/component/path", "path3/component/path"},
+				targetPaths: []string{"path1", "path2", "path3"},
+			},
+			want: []string{"path1", "path2", "path3"},
+		},
+		{
+			name: "paths1 is empty",
+			args: args{
+				promoPaths:  []string{},
+				targetPaths: []string{"path1", "path2", "path3"},
+			},
+			want: nil,
+		},
+		{
+			name: "paths2 is empty",
+			args: args{
+				promoPaths:  []string{"path1/component/some", "path2/some/other", "path3"},
+				targetPaths: []string{},
+			},
+			want: nil,
+		},
+		{
+			name: "paths2 missing elements",
+			args: args{
+				promoPaths:  []string{"path1", "path2", "path3"},
+				targetPaths: []string{""},
+			},
+			want: nil,
+		},
+		{
+			name: "path1 missing elements",
+			args: args{
+				promoPaths:  []string{""},
+				targetPaths: []string{"path1", "path2"},
+			},
+			want: nil,
+		},
+		{
+			name: "path1 and path2 common elements",
+			args: args{
+				promoPaths:  []string{"path1/component/path", "path3/component/also"},
+				targetPaths: []string{"path1", "path2", "path3"},
+			},
+			want: []string{"path1", "path3"},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			got := identifyCommonPaths(tt.args.promoPaths, tt.args.targetPaths)
+			assert.Equal(t, got, tt.want)
+		})
+	}
+}
diff --git a/internal/pkg/githubapi/testdata/pr_body.golden.md b/internal/pkg/githubapi/testdata/pr_body.golden.md
index ef3693fe..a01e4f2e 100644
--- a/internal/pkg/githubapi/testdata/pr_body.golden.md
+++ b/internal/pkg/githubapi/testdata/pr_body.golden.md
@@ -2,7 +2,6 @@
 &nbsp;&nbsp;&nbsp;&nbsp;`targetPath1`  
 &nbsp;&nbsp;&nbsp;&nbsp;`targetPath2`  
 &nbsp;&nbsp;&nbsp;&nbsp;↘️  #2  `sourcePath2` ➡️  
-&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;`targetPath3`  
 &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;`targetPath4`  
 &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;↘️  #3  `sourcePath3` ➡️  
 &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;`targetPath5`