codereview #4: fixes from testing

Josh Bialkowski created 5 years ago

* don't prefix imported title's with jira ID
* fix import new comment due to wrong variable name
* fix double import of comment edition due to improper err check
* fix JIRA cloud paginated changelog has a different JSON
  field then the embedded changelog in the JIRA server issue
  object
* fix splitting label strings yielded an empty string as a label
  value

Change summary

bridge/jira/client.go | 14 +++++++++++++-
bridge/jira/import.go | 37 +++++++++++++++++++++++++++++--------
2 files changed, 42 insertions(+), 9 deletions(-)

Detailed changes

bridge/jira/client.go

@@ -165,7 +165,9 @@ type ChangeLogPage struct {
 	StartAt    int              `json:"startAt"`
 	MaxResults int              `json:"maxResults"`
 	Total      int              `json:"total"`
+	IsLast     bool             `json:"isLast"` // Cloud-only
 	Entries    []ChangeLogEntry `json:"histories"`
+	Values     []ChangeLogEntry `json:"values"`
 }
 
 // NextStartAt return the index of the first item on the next page
@@ -175,6 +177,9 @@ func (self *ChangeLogPage) NextStartAt() int {
 
 // IsLastPage return true if there are no more items beyond this page
 func (self *ChangeLogPage) IsLastPage() bool {
+	// NOTE(josh): The "isLast" field is returned on JIRA cloud, but not on
+	// JIRA server. If we can distinguish which one we are working with, we can
+	// possibly rely on that instead.
 	return self.NextStartAt() >= self.Total
 }
 
@@ -804,7 +809,8 @@ func (client *Client) IterComments(
 // https://docs.atlassian.com/software/jira/docs/api/REST/8.2.6/#api/2/issue
 func (client *Client) GetChangeLog(
 	idOrKey string, maxResults int, startAt int) (*ChangeLogPage, error) {
-	url := fmt.Sprintf("%s/rest/api/2/issue/%s/changelog", client.serverURL, idOrKey)
+	url := fmt.Sprintf(
+		"%s/rest/api/2/issue/%s/changelog", client.serverURL, idOrKey)
 
 	request, err := http.NewRequest("GET", url, nil)
 	if err != nil {
@@ -865,6 +871,12 @@ func (client *Client) GetChangeLog(
 		return nil, err
 	}
 
+	// JIRA cloud returns changelog entries in the "values" list, whereas JIRA
+	// server returns them in the "histories" list when embedded in an issue
+	// object.
+	changelog.Entries = changelog.Values
+	changelog.Values = nil
+
 	return &changelog, nil
 }
 

bridge/jira/import.go

@@ -207,7 +207,7 @@ func (self *jiraImporter) ensureIssue(
 			return nil, err
 		}
 
-		title := fmt.Sprintf("[%s]: %s", issue.Key, issue.Fields.Summary)
+		title := issue.Fields.Summary
 		b, _, err = repo.NewBugRaw(
 			author,
 			issue.Fields.Created.Unix(),
@@ -278,6 +278,7 @@ func (self *jiraImporter) ensureComment(repo *cache.RepoCache,
 		}
 
 		self.out <- core.NewImportComment(op.Id())
+		targetOpID = op.Id()
 	}
 
 	// If there are no updates to this comment, then we are done
@@ -292,7 +293,12 @@ func (self *jiraImporter) ensureComment(repo *cache.RepoCache,
 	derivedID := getTimeDerivedID(item.ID, item.Updated)
 	_, err = b.ResolveOperationWithMetadata(
 		keyJiraID, derivedID)
-	if err != nil && err != cache.ErrNoMatchingOp {
+	if err == nil {
+		// Already imported this edition
+		return nil
+	}
+
+	if err != cache.ErrNoMatchingOp {
 		return err
 	}
 
@@ -310,7 +316,7 @@ func (self *jiraImporter) ensureComment(repo *cache.RepoCache,
 	op, err := b.EditCommentRaw(
 		editor,
 		item.Updated.Unix(),
-		target,
+		targetOpID,
 		cleanText,
 		map[string]string{
 			keyJiraID: derivedID,
@@ -397,8 +403,8 @@ func (self *jiraImporter) ensureChange(
 	for _, item := range entry.Items {
 		switch item.Field {
 		case "labels":
-			fromLabels := strings.Split(item.FromString, " ")
-			toLabels := strings.Split(item.ToString, " ")
+			fromLabels := removeEmpty(strings.Split(item.FromString, " "))
+			toLabels := removeEmpty(strings.Split(item.ToString, " "))
 			removedLabels, addedLabels, _ := setSymmetricDifference(
 				fromLabels, toLabels)
 
@@ -467,14 +473,15 @@ func (self *jiraImporter) ensureChange(
 		_, err := b.ResolveOperationWithMetadata(keyJiraOperationID, derivedID)
 		if err == nil {
 			continue
-		} else if err != cache.ErrNoMatchingOp {
+		}
+		if err != cache.ErrNoMatchingOp {
 			return err
 		}
 
 		switch item.Field {
 		case "labels":
-			fromLabels := strings.Split(item.FromString, " ")
-			toLabels := strings.Split(item.ToString, " ")
+			fromLabels := removeEmpty(strings.Split(item.FromString, " "))
+			toLabels := removeEmpty(strings.Split(item.ToString, " "))
 			removedLabels, addedLabels, _ := setSymmetricDifference(
 				fromLabels, toLabels)
 
@@ -562,6 +569,9 @@ func (self *jiraImporter) ensureChange(
 			}
 
 			self.out <- core.NewImportCommentEdition(op.Id())
+
+		default:
+			fmt.Printf("Unhandled changelog event %s\n", item.Field)
 		}
 
 		// Other Examples:
@@ -588,3 +598,14 @@ func getStatusMap(conf core.Configuration) (map[string]string, error) {
 	err := json.Unmarshal([]byte(mapStr), &statusMap)
 	return statusMap, err
 }
+
+func removeEmpty(values []string) []string {
+	output := make([]string, 0, len(values))
+	for _, value := range values {
+		value = strings.TrimSpace(value)
+		if value != "" {
+			output = append(output, value)
+		}
+	}
+	return output
+}