Cleanup of comment creation (#42)

Refactor of comment creation code

cmd/approve.go

@@ -3,21 +3,9 @@ package main
 import (
 	"encoding/json"
 	"errors"
-	"fmt"
 	"net/http"
 )
 
-func (c *Client) Approve() (string, int, error) {
-
-	_, res, err := c.git.MergeRequestApprovals.ApproveMergeRequest(c.projectId, c.mergeId, nil, nil)
-
-	if err != nil {
-		return "", res.Response.StatusCode, fmt.Errorf("Approving MR failed: %w", err)
-	}
-
-	return "Success! Approved MR.", http.StatusOK, nil
-}
-
 func ApproveHandler(w http.ResponseWriter, r *http.Request) {
 	w.Header().Set("Content-Type", "application/json")
 	c := r.Context().Value("client").(Client)
@@ -26,7 +14,8 @@ func ApproveHandler(w http.ResponseWriter, r *http.Request) {
 		c.handleError(w, errors.New("Invalid request type"), "That request type is not allowed", http.StatusMethodNotAllowed)
 		return
 	}
-	msg, status, err := c.Approve()
+
+	_, res, err := c.git.MergeRequestApprovals.ApproveMergeRequest(c.projectId, c.mergeId, nil, nil)
 
 	if err != nil {
 		c.handleError(w, err, "Could not approve MR", http.StatusBadRequest)
@@ -34,9 +23,9 @@ func ApproveHandler(w http.ResponseWriter, r *http.Request) {
 	}
 
 	/* TODO: Check for non-200 status codes */
-	w.WriteHeader(status)
+	w.WriteHeader(res.StatusCode)
 	response := SuccessResponse{
-		Message: msg,
+		Message: "Success! Approved MR.",
 		Status:  http.StatusOK,
 	}
 

cmd/comment.go

@@ -1,36 +1,24 @@
 package main
 
 import (
-	"bytes"
 	"encoding/json"
 	"errors"
-	"fmt"
 	"io"
-	"io/ioutil"
-	"mime/multipart"
 	"net/http"
-	"time"
 
 	"github.com/xanzy/go-gitlab"
 )
 
 const mrVersionsUrl = "%s/api/v4/projects/%s/merge_requests/%d/versions"
 
-type MRVersion struct {
+type PostCommentRequest struct {
-	ID             int       `json:"id"`
+	Comment        string `json:"comment"`
+	FileName       string `json:"file_name"`
+	LineNumber     int    `json:"line_number"`
 	HeadCommitSHA  string `json:"head_commit_sha"`
 	BaseCommitSHA  string `json:"base_commit_sha"`
 	StartCommitSHA string `json:"start_commit_sha"`
-	CreatedAt      time.Time `json:"created_at"`
+	Type           string `json:"type"`
-	MergeRequestID int       `json:"merge_request_id"`
-	State          string    `json:"state"`
-	RealSize       string    `json:"real_size"`
-}
-
-type PostCommentRequest struct {
-	LineNumber int    `json:"line_number"`
-	FileName   string `json:"file_name"`
-	Comment    string `json:"comment"`
 }
 
 type DeleteCommentRequest struct {
@@ -91,7 +79,6 @@ func DeleteComment(w http.ResponseWriter, r *http.Request) {
 
 	/* TODO: Check status code */
 	w.WriteHeader(http.StatusOK)
-
 	response := SuccessResponse{
 		Message: "Comment deleted succesfully",
 		Status:  http.StatusOK,
@@ -119,22 +106,46 @@ func PostComment(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	res, err := c.PostComment(postCommentRequest)
+	position := &gitlab.NotePosition{
-	for k, v := range res.Header {
+		PositionType: "text",
-		w.Header().Set(k, v[0])
+		StartSHA:     postCommentRequest.StartCommitSHA,
+		HeadSHA:      postCommentRequest.HeadCommitSHA,
+		BaseSHA:      postCommentRequest.BaseCommitSHA,
+		NewPath:      postCommentRequest.FileName,
+		OldPath:      postCommentRequest.FileName,
 	}
 
+	/* TODO: This switch statement relates to #25, for now we are just sending both
+	the old line and new line but we will eventually have to fix this */
+	switch postCommentRequest.Type {
+	case "addition":
+		position.NewLine = postCommentRequest.LineNumber
+	case "subtraction":
+		position.OldLine = postCommentRequest.LineNumber
+	case "modification":
+		position.NewLine = postCommentRequest.LineNumber
+		position.OldLine = postCommentRequest.LineNumber
+	}
+
+	discussion, _, err := c.git.Discussions.CreateMergeRequestDiscussion(
+		c.projectId,
+		c.mergeId,
+		&gitlab.CreateMergeRequestDiscussionOptions{
+			Body:     &postCommentRequest.Comment,
+			Position: position,
+		})
+
 	if err != nil {
-		c.handleError(w, err, "Could not post comment", res.StatusCode)
+		c.handleError(w, err, "Could not create comment", http.StatusBadRequest)
 		return
 	}
 
-	w.WriteHeader(res.StatusCode)
+	response := CommentResponse{
-
+		SuccessResponse: SuccessResponse{
-	/* TODO: Check for bad status codes */
+			Message: "Comment updated succesfully",
-	response := SuccessResponse{
-		Message: "Comment created succesfully",
 			Status:  http.StatusOK,
+		},
+		Comment: discussion.Notes[0],
 	}
 
 	json.NewEncoder(w).Encode(response)
@@ -194,124 +205,3 @@ func EditComment(w http.ResponseWriter, r *http.Request) {
 
 	json.NewEncoder(w).Encode(response)
 }
-
-func (c *Client) PostComment(cr PostCommentRequest) (*http.Response, error) {
-
-	err, response := getMRVersions(c.gitlabInstance, c.projectId, c.mergeId, c.authToken)
-	if err != nil {
-		return nil, fmt.Errorf("Error making diff thread: %e", err)
-	}
-
-	defer response.Body.Close()
-
-	body, err := ioutil.ReadAll(response.Body)
-
-	var diffVersionInfo []MRVersion
-	err = json.Unmarshal(body, &diffVersionInfo)
-	if err != nil {
-		return nil, fmt.Errorf("Error unmarshalling version info JSON: %w", err)
-	}
-
-	/* This is necessary since we do not know whether the comment is on a line that
-	   has been changed or not. Making all three of these API calls will let us leave
-	   the comment regardless. I ran these in sequence vai a Sync.WaitGroup, but
-	   it was successfully posting a comment to a modified twice, so now I'm running
-	   them in sequence.
-
-	   To clean this up we might try to detect more information about the change in our
-	   Lua code and pass it to the Go code.
-
-	   See the Gitlab documentation: https://docs.gitlab.com/ee/api/discussions.html#create-a-new-thread-in-the-merge-request-diff */
-	for i := 0; i < 3; i++ {
-		ii := i
-		res, err := c.CommentOnDeletion(cr.LineNumber, cr.FileName, cr.Comment, diffVersionInfo[0], ii)
-
-		if err == nil && res.StatusCode >= 200 && res.StatusCode <= 299 {
-			return res, nil
-		}
-	}
-
-	return nil, fmt.Errorf("Could not leave comment")
-
-}
-
-func min(a int, b int) int {
-	if a < b {
-		return a
-	}
-	return b
-}
-
-/* Gets the latest merge request revision data */
-func getMRVersions(gitlabInstance string, projectId string, mergeId int, authToken string) (e error, response *http.Response) {
-	url := fmt.Sprintf(mrVersionsUrl, gitlabInstance, projectId, mergeId)
-
-	req, err := http.NewRequest(http.MethodGet, url, nil)
-
-	req.Header.Add("PRIVATE-TOKEN", authToken)
-
-	if err != nil {
-		return err, nil
-	}
-
-	client := &http.Client{}
-	response, err = client.Do(req)
-
-	if err != nil {
-		return err, nil
-	}
-
-	if response.StatusCode != 200 {
-		return errors.New("Non-200 status code: " + response.Status), nil
-	}
-
-	return nil, response
-}
-
-/*
-Creates a new merge request discussion https://docs.gitlab.com/ee/api/discussions.html#create-new-merge-request-thread
-The go-gitlab client was not working for this API specifically 😢
-*/
-func (c *Client) CommentOnDeletion(lineNumber int, fileName string, comment string, diffVersionInfo MRVersion, i int) (*http.Response, error) {
-
-	deletionDiscussionUrl := fmt.Sprintf(c.gitlabInstance+"/api/v4/projects/%s/merge_requests/%d/discussions", c.projectId, c.mergeId)
-
-	payload := &bytes.Buffer{}
-	writer := multipart.NewWriter(payload)
-	_ = writer.WriteField("body", comment)
-	_ = writer.WriteField("position[base_sha]", diffVersionInfo.BaseCommitSHA)
-	_ = writer.WriteField("position[start_sha]", diffVersionInfo.StartCommitSHA)
-	_ = writer.WriteField("position[head_sha]", diffVersionInfo.HeadCommitSHA)
-	_ = writer.WriteField("position[position_type]", "text")
-
-	/* We need to set these properties differently depending on whether we're commenting on a deleted line,
-	a modified line, an added line, or an unmodified line */
-	_ = writer.WriteField("position[old_path]", fileName)
-	_ = writer.WriteField("position[new_path]", fileName)
-	if i == 0 {
-		_ = writer.WriteField("position[old_line]", fmt.Sprintf("%d", lineNumber))
-	} else if i == 1 {
-		_ = writer.WriteField("position[new_line]", fmt.Sprintf("%d", lineNumber))
-	} else {
-		_ = writer.WriteField("position[old_line]", fmt.Sprintf("%d", lineNumber))
-		_ = writer.WriteField("position[new_line]", fmt.Sprintf("%d", lineNumber))
-	}
-
-	err := writer.Close()
-	if err != nil {
-		return nil, fmt.Errorf("Error making form data: %w", err)
-	}
-
-	client := &http.Client{}
-	req, err := http.NewRequest(http.MethodPost, deletionDiscussionUrl, payload)
-
-	if err != nil {
-		return nil, fmt.Errorf("Error building request: %w", err)
-	}
-	req.Header.Add("PRIVATE-TOKEN", c.authToken)
-	req.Header.Set("Content-Type", writer.FormDataContentType())
-
-	res, err := client.Do(req)
-
-	return res, err
-}

cmd/main.go

@@ -31,6 +31,7 @@ func main() {
 	m := http.NewServeMux()
 	m.Handle("/mr/description", withGitlabContext(http.HandlerFunc(DescriptionHandler), c))
 	m.Handle("/mr/reviewer", withGitlabContext(http.HandlerFunc(ReviewersHandler), c))
+	m.Handle("/mr/revisions", withGitlabContext(http.HandlerFunc(RevisionsHandler), c))
 	m.Handle("/mr/assignee", withGitlabContext(http.HandlerFunc(AssigneesHandler), c))
 	m.Handle("/approve", withGitlabContext(http.HandlerFunc(ApproveHandler), c))
 	m.Handle("/revoke", withGitlabContext(http.HandlerFunc(RevokeHandler), c))

cmd/revisions.go

@@ -0,0 +1,42 @@
+package main
+
+import (
+	"encoding/json"
+	"errors"
+	"net/http"
+
+	"github.com/xanzy/go-gitlab"
+)
+
+type RevisionsResponse struct {
+	SuccessResponse
+	Revisions []*gitlab.MergeRequestDiffVersion
+}
+
+func RevisionsHandler(w http.ResponseWriter, r *http.Request) {
+	c := r.Context().Value("client").(Client)
+	w.Header().Set("Content-Type", "application/json")
+
+	if r.Method != http.MethodGet {
+		c.handleError(w, errors.New("Invalid request type"), "That request type is not allowed", http.StatusMethodNotAllowed)
+		w.WriteHeader(http.StatusMethodNotAllowed)
+		return
+	}
+
+	versionInfo, _, err := c.git.MergeRequests.GetMergeRequestDiffVersions(c.projectId, c.mergeId, &gitlab.GetMergeRequestDiffVersionsOptions{})
+	if err != nil {
+		c.handleError(w, err, "Could not get diff version info", http.StatusBadRequest)
+	}
+
+	w.WriteHeader(http.StatusOK)
+	response := RevisionsResponse{
+		SuccessResponse: SuccessResponse{
+			Message: "Revisions fetched successfully",
+			Status:  http.StatusOK,
+		},
+		Revisions: versionInfo,
+	}
+
+	json.NewEncoder(w).Encode(response)
+
+}

cmd/revoke.go

@@ -3,22 +3,9 @@ package main
 import (
 	"encoding/json"
 	"errors"
-	"fmt"
 	"net/http"
 )
 
-func (c *Client) Revoke() (string, int, error) {
-
-	res, err := c.git.MergeRequestApprovals.UnapproveMergeRequest(c.projectId, c.mergeId, nil, nil)
-
-	if err != nil {
-		return "", res.Response.StatusCode, fmt.Errorf("Revoking approval failed: %w", err)
-	}
-
-	return "Success! Revoked MR approval.", http.StatusOK, nil
-
-}
-
 func RevokeHandler(w http.ResponseWriter, r *http.Request) {
 	c := r.Context().Value("client").(Client)
 	w.Header().Set("Content-Type", "application/json")
@@ -29,16 +16,17 @@ func RevokeHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	msg, status, err := c.Revoke()
+	res, err := c.git.MergeRequestApprovals.UnapproveMergeRequest(c.projectId, c.mergeId, nil, nil)
 
 	if err != nil {
 		c.handleError(w, err, "Could not revoke approval", http.StatusBadRequest)
 		return
 	}
 
-	w.WriteHeader(status)
+	/* TODO: Check for non-200 status codes */
+	w.WriteHeader(res.StatusCode)
 	response := SuccessResponse{
-		Message: msg,
+		Message: "Success! Revoked MR approval.",
 		Status:  http.StatusOK,
 	}
 

lua/gitlab/comment.lua

@@ -35,7 +35,22 @@ M.confirm_create_comment = function(text)
     end
   end
 
-  local jsonTable = { line_number = current_line_number, file_name = relative_file_path, comment = text }
+  -- TODO: How can we know whether to specify that the comment is on a line that has been modified,
+  -- added, or deleted? Additionally, how will we know which line number to send?
+  -- We need an intelligent way of getting this information so that we can send it to the comment
+  -- creation endpoint, relates to Issue #25: https://github.com/harrisoncramer/gitlab.nvim/issues/25
+
+  local revision = state.MR_REVISIONS[1]
+  local jsonTable = {
+    comment = text,
+    file_name = relative_file_path,
+    line_number = current_line_number,
+    base_commit_sha = revision.base_commit_sha,
+    start_commit_sha = revision.start_commit_sha,
+    head_commit_sha = revision.head_commit_sha,
+    type = "modification"
+  }
+
   local json = vim.json.encode(jsonTable)
 
   job.run_job("comment", "POST", json, function(data)

lua/gitlab/init.lua

@@ -103,6 +103,19 @@ M.ensureProjectMembers   = function(callback)
   end
 end
 
+M.ensureRevisions        = function(callback)
+  return function()
+    if type(state.MR_REVISIONS) ~= "table" then
+      job.run_job("mr/revisions", "GET", nil, function(data)
+        state.MR_REVISIONS = data.Revisions
+        callback()
+      end)
+    else
+      callback()
+    end
+  end
+end
+
 -- Builds the Go binary
 M.build                  = function()
   local command = string.format("cd %s && make", state.BIN_PATH)
@@ -173,7 +186,7 @@ M.summary                = M.ensureState(summary.summary)
 M.approve                = M.ensureState(function() job.run_job("approve", "POST") end)
 M.revoke                 = M.ensureState(function() job.run_job("revoke", "POST") end)
 M.list_discussions       = M.ensureState(discussions.list_discussions)
-M.create_comment         = M.ensureState(comment.create_comment)
+M.create_comment         = M.ensureState(M.ensureRevisions(comment.create_comment))
 M.edit_comment           = M.ensureState(comment.edit_comment)
 M.delete_comment         = M.ensureState(comment.delete_comment)
 M.toggle_resolved        = M.ensureState(comment.toggle_resolved)

lua/gitlab/state.lua

@@ -1,14 +1,5 @@
 local M   = {}
 
--- This is the global state that can be set from
--- various places in the plugin. It all begins as
--- uninitialized and is set by the setup/ensure function calls
-
-M.BIN_PATH   = nil -- Directory of the Go binary
-M.BIN        = nil -- Full path to the Go binary
-M.PROJECT_ID = nil -- Gitlab Project ID, set in .gitlab.nvim file
-M.INFO       = nil -- The basic information about the MR, set from "/info" endpoint
-
 -- These are the default keymaps for the plugin
 M.keymaps = {
   popup = {