refactor: fix multiple quality issues (#27)

* refactor: rename HTTPClientOptions to HTTPClientOpts * refactor: move SendRequest parameters to its own struct * refactor: return explicit `nil` for err in SendRequest when no error happened * refactor(clockify): split FetchEntries to smaller pieces * refactor(toggl): split FetchEntries to smaller pieces * refactor(timewarrior): split FetchEntries to smaller pieces * chore(changelog): update changelog
gabor-boros · Oct 18, 2021 · 08dff13 · 08dff13
1 parent 76f99b6
commit 08dff13
Show file tree

Hide file tree

Showing 12 changed files with 319 additions and 206 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -43,6 +43,7 @@ All notable changes to this project will be documented in this file.
 - Add virtualenv to gitignore ([466aa6d](https://github.com/gabor-boros/minutes/commit/466aa6d7d3cba1aba26185873c606d16c3e59483))
 - Refactor and add badges ([72f091f](https://github.com/gabor-boros/minutes/commit/72f091f8fcfb18584e51e9064d7691de2abc5217))
 - Add pull request template ([21ce60a](https://github.com/gabor-boros/minutes/commit/21ce60a68125fe3bf22e6505becda6249b9cdcdf))
+- Create PR welcome messages ([76f99b6](https://github.com/gabor-boros/minutes/commit/76f99b635f0ced3bfe64012454138a9fe5a75cf9))
 
 **Refactor**
 
@@ -60,6 +61,12 @@ All notable changes to this project will be documented in this file.
 - Use outsourced entry duration splitting ([7be81c2](https://github.com/gabor-boros/minutes/commit/7be81c2431468679a753547a2a225c3b9560c8fb))
 - Wrap errors into client.ErrFetchEntries ([90f3f2b](https://github.com/gabor-boros/minutes/commit/90f3f2bfe008e8c1d6e82ef0d8255dd50ba4ed0f))
 - Simplify worklog creation ([15bdad7](https://github.com/gabor-boros/minutes/commit/15bdad721f648586f1175b403ca987daa114f400))
+- Rename HTTPClientOptions to HTTPClientOpts ([a2342ff](https://github.com/gabor-boros/minutes/commit/a2342ff0b555629e3c7ff75a3654a147ce669c66))
+- Move SendRequest parameters to its own struct ([e486f6e](https://github.com/gabor-boros/minutes/commit/e486f6e3e82f384fc1a2b0b7730e1b8a86942da6))
+- Return explicit `nil` for err in SendRequest when no error happened ([7d0ea5f](https://github.com/gabor-boros/minutes/commit/7d0ea5fcf7bfcc7b52abb6f29352ddc9fa2a6be3))
+- Split FetchEntries to smaller pieces ([e135044](https://github.com/gabor-boros/minutes/commit/e135044b3a72e708f8842d5b5eaffec30ee9d0e7))
+- Split FetchEntries to smaller pieces ([1678fd1](https://github.com/gabor-boros/minutes/commit/1678fd126c254db16c5f4d2a0ab4b18112ebe495))
+- Split FetchEntries to smaller pieces ([4d6c222](https://github.com/gabor-boros/minutes/commit/4d6c2222f9474d4a82cac76ea648c02cf912c542))
 
 **Testing**
 

diff --git a/cmd/root.go b/cmd/root.go
@@ -230,7 +230,7 @@ func validateFlags() {
 
 func getClientOpts(urlFlag string, usernameFlag string, passwordFlag string, tokenFlag string, tokenHeader string) (*client.BaseClientOpts, error) {
 	opts := &client.BaseClientOpts{
-		HTTPClientOptions: client.HTTPClientOptions{
+		HTTPClientOpts: client.HTTPClientOpts{
 			HTTPClient:  http.DefaultClient,
 			TokenHeader: tokenHeader,
 		},
@@ -309,7 +309,7 @@ func getFetcher() (client.Fetcher, error) {
 			UnbillableTag:      viper.GetString("timewarrior-unbillable-tag"),
 			ClientTagRegex:     viper.GetString("timewarrior-client-tag-regex"),
 			ProjectTagRegex:    viper.GetString("timewarrior-project-tag-regex"),
-		}), nil
+		})
 	case "toggl":
 		opts, err := getClientOpts(
 			"toggl-url",

diff --git a/internal/pkg/client/client.go b/internal/pkg/client/client.go
@@ -23,8 +23,8 @@ var (
 	ErrUploadEntries = errors.New("failed to upload entries")
 )
 
-// HTTPClientOptions specifies all options that are required for HTTP clients.
-type HTTPClientOptions struct {
+// HTTPClientOpts specifies all options that are required for HTTP clients.
+type HTTPClientOpts struct {
 	HTTPClient *http.Client
 	// BaseURL for the API, without a trailing slash.
 	BaseURL string
@@ -47,7 +47,7 @@ type HTTPClientOptions struct {
 // When a client needs other options as well, it composes a new set of options
 // using BaseClientOpts.
 type BaseClientOpts struct {
-	HTTPClientOptions
+	HTTPClientOpts
 	// TagsAsTasks defines to use tag names to determine the task.
 	// Using TagsAsTasks can be useful if the user's workflow involves
 	// splitting activity across multiple tasks, or when the user has no option
@@ -119,44 +119,54 @@ type FetchUploader interface {
 	Uploader
 }
 
+// SendRequestOpts represents the parameters needed for sending a request.
+// Since SendRequest is for sending requests to HTTP based APIs, it receives
+// the HTTPClientOpts as well for its options.
+type SendRequestOpts struct {
+	Method     string
+	Path       string
+	ClientOpts *HTTPClientOpts
+	Data       interface{}
+}
+
 // SendRequest is a helper for any Fetcher and Uploader that must APIs.
 // The SendRequest function prepares a new HTTP request, sends it and returns
 // the response for further parsing. If the response status is not 200 or 201,
 // the function returns an error.
-func SendRequest(ctx context.Context, method string, path string, data interface{}, opts *HTTPClientOptions) (*http.Response, error) {
+func SendRequest(ctx context.Context, opts *SendRequestOpts) (*http.Response, error) {
 	var err error
 	var marshalledData []byte
 
-	requestURL, err := url.Parse(opts.BaseURL + path)
+	requestURL, err := url.Parse(opts.ClientOpts.BaseURL + opts.Path)
 	if err != nil {
 		return nil, err
 	}
 
-	if data != nil {
-		marshalledData, err = json.Marshal(data)
+	if opts.Data != nil {
+		marshalledData, err = json.Marshal(opts.Data)
 		if err != nil {
 			return nil, err
 		}
 	}
 
-	req, err := http.NewRequestWithContext(ctx, method, requestURL.String(), bytes.NewBuffer(marshalledData))
+	req, err := http.NewRequestWithContext(ctx, opts.Method, requestURL.String(), bytes.NewBuffer(marshalledData))
 	if err != nil {
 		return nil, err
 	}
 
 	req.Header.Add("Content-Type", "application/json")
 
-	if opts.Token != "" {
-		if opts.TokenHeader == "" {
+	if opts.ClientOpts.Token != "" {
+		if opts.ClientOpts.TokenHeader == "" {
 			return nil, errors.New("no token header name")
 		}
 
-		req.Header.Add(opts.TokenHeader, opts.Token)
+		req.Header.Add(opts.ClientOpts.TokenHeader, opts.ClientOpts.Token)
 	} else {
-		req.SetBasicAuth(opts.Username, opts.Password)
+		req.SetBasicAuth(opts.ClientOpts.Username, opts.ClientOpts.Password)
 	}
 
-	resp, err := opts.HTTPClient.Do(req)
+	resp, err := opts.ClientOpts.HTTPClient.Do(req)
 	if err != nil {
 		return nil, err
 	}
@@ -170,5 +180,5 @@ func SendRequest(ctx context.Context, method string, path string, data interface
 		return nil, fmt.Errorf("%d: %s", resp.StatusCode, string(errBody))
 	}
 
-	return resp, err
+	return resp, nil
 }
diff --git a/internal/pkg/client/client_test.go b/internal/pkg/client/client_test.go
@@ -87,9 +87,14 @@ func TestSendRequest_GET(t *testing.T) {
 	})
 	defer mockServer.Close()
 
-	resp, err := client.SendRequest(context.Background(), http.MethodGet, "/endpoint", nil, &client.HTTPClientOptions{
-		HTTPClient: http.DefaultClient,
-		BaseURL:    mockServer.URL,
+	resp, err := client.SendRequest(context.Background(), &client.SendRequestOpts{
+		Method: http.MethodGet,
+		Path:   "/endpoint",
+		ClientOpts: &client.HTTPClientOpts{
+			HTTPClient: http.DefaultClient,
+			BaseURL:    mockServer.URL,
+		},
+		Data: nil,
 	})
 
 	require.Nil(t, err, "request failed")
@@ -109,12 +114,17 @@ func TestSendRequest_POST(t *testing.T) {
 	})
 	defer mockServer.Close()
 
-	requestOpts := &client.HTTPClientOptions{
+	requestOpts := &client.HTTPClientOpts{
 		HTTPClient: http.DefaultClient,
 		BaseURL:    mockServer.URL,
 	}
 
-	resp, err := client.SendRequest(context.Background(), http.MethodPost, "/endpoint", data, requestOpts)
+	resp, err := client.SendRequest(context.Background(), &client.SendRequestOpts{
+		Method:     http.MethodPost,
+		Path:       "/endpoint",
+		ClientOpts: requestOpts,
+		Data:       data,
+	})
 
 	require.Nil(t, err, "request failed")
 	require.Equal(t, http.StatusOK, resp.StatusCode)
@@ -130,11 +140,16 @@ func TestSendRequest_BasicAuth(t *testing.T) {
 	})
 	defer mockServer.Close()
 
-	resp, err := client.SendRequest(context.Background(), http.MethodGet, "/endpoint", nil, &client.HTTPClientOptions{
-		HTTPClient: http.DefaultClient,
-		BaseURL:    mockServer.URL,
-		Username:   "Thor",
-		Password:   "The strongest Avenger",
+	resp, err := client.SendRequest(context.Background(), &client.SendRequestOpts{
+		Method: http.MethodGet,
+		Path:   "/endpoint",
+		ClientOpts: &client.HTTPClientOpts{
+			HTTPClient: http.DefaultClient,
+			BaseURL:    mockServer.URL,
+			Username:   "Thor",
+			Password:   "The strongest Avenger",
+		},
+		Data: nil,
 	})
 
 	require.Nil(t, err, "request failed")
@@ -151,11 +166,16 @@ func TestSendRequest_TokenAuth(t *testing.T) {
 	})
 	defer mockServer.Close()
 
-	resp, err := client.SendRequest(context.Background(), http.MethodGet, "/endpoint", nil, &client.HTTPClientOptions{
-		HTTPClient:  http.DefaultClient,
-		BaseURL:     mockServer.URL,
-		Token:       "t-o-k-e-n",
-		TokenHeader: "X-API-Token",
+	resp, err := client.SendRequest(context.Background(), &client.SendRequestOpts{
+		Method: http.MethodGet,
+		Path:   "/endpoint",
+		ClientOpts: &client.HTTPClientOpts{
+			HTTPClient:  http.DefaultClient,
+			BaseURL:     mockServer.URL,
+			Token:       "t-o-k-e-n",
+			TokenHeader: "X-API-Token",
+		},
+		Data: nil,
 	})
 
 	require.Nil(t, err, "request failed")
@@ -170,11 +190,16 @@ func TestSendRequest_TokenAuth_NoHeader(t *testing.T) {
 	})
 	defer mockServer.Close()
 
-	resp, err := client.SendRequest(context.Background(), http.MethodGet, "/endpoint", nil, &client.HTTPClientOptions{
-		HTTPClient:  http.DefaultClient,
-		BaseURL:     mockServer.URL,
-		Token:       "t-o-k-e-n",
-		TokenHeader: "",
+	resp, err := client.SendRequest(context.Background(), &client.SendRequestOpts{
+		Method: http.MethodGet,
+		Path:   "/endpoint",
+		ClientOpts: &client.HTTPClientOpts{
+			HTTPClient:  http.DefaultClient,
+			BaseURL:     mockServer.URL,
+			Token:       "t-o-k-e-n",
+			TokenHeader: "",
+		},
+		Data: nil,
 	})
 
 	require.Nil(t, resp, "request unexpectedly sent")
@@ -189,9 +214,14 @@ func TestSendRequest_Error(t *testing.T) {
 	})
 	defer mockServer.Close()
 
-	resp, err := client.SendRequest(context.Background(), http.MethodGet, "/endpoint", nil, &client.HTTPClientOptions{
-		HTTPClient: http.DefaultClient,
-		BaseURL:    mockServer.URL,
+	resp, err := client.SendRequest(context.Background(), &client.SendRequestOpts{
+		Method: http.MethodGet,
+		Path:   "/endpoint",
+		ClientOpts: &client.HTTPClientOpts{
+			HTTPClient: http.DefaultClient,
+			BaseURL:    mockServer.URL,
+		},
+		Data: nil,
 	})
 
 	require.Nil(t, resp, "response unexpectedly succeeded")

diff --git a/internal/pkg/client/clockify/clockify.go b/internal/pkg/client/clockify/clockify.go
@@ -88,6 +88,69 @@ func (c *clockifyClient) getSearchURL(user string, params *WorklogSearchParams)
 	return fmt.Sprintf("%s?%s", worklogURL.Path, worklogURL.Query().Encode()), nil
 }
 
+func (c *clockifyClient) fetchEntries(ctx context.Context, path string) ([]FetchEntry, error) {
+	resp, err := client.SendRequest(ctx, &client.SendRequestOpts{
+		Method:     http.MethodGet,
+		Path:       path,
+		ClientOpts: &c.opts.HTTPClientOpts,
+		Data:       nil,
+	})
+
+	if err != nil {
+		return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err)
+	}
+
+	var fetchedEntries []FetchEntry
+	if err = json.NewDecoder(resp.Body).Decode(&fetchedEntries); err != nil {
+		return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err)
+	}
+
+	return fetchedEntries, err
+}
+
+func (c *clockifyClient) parseEntries(fetchedEntries []FetchEntry, tagsAsTasksRegex *regexp.Regexp) []worklog.Entry {
+	var entries []worklog.Entry
+
+	for _, entry := range fetchedEntries {
+		billableDuration := entry.TimeInterval.End.Sub(entry.TimeInterval.Start)
+		unbillableDuration := time.Duration(0)
+
+		if !entry.Billable {
+			unbillableDuration = billableDuration
+			billableDuration = 0
+		}
+
+		worklogEntry := worklog.Entry{
+			Client: worklog.IDNameField{
+				ID:   entry.Project.ClientID,
+				Name: entry.Project.ClientName,
+			},
+			Project: worklog.IDNameField{
+				ID:   entry.Project.ID,
+				Name: entry.Project.Name,
+			},
+			Task: worklog.IDNameField{
+				ID:   entry.Task.ID,
+				Name: entry.Task.Name,
+			},
+			Summary:            entry.Task.Name,
+			Notes:              entry.Description,
+			Start:              entry.TimeInterval.Start,
+			BillableDuration:   billableDuration,
+			UnbillableDuration: unbillableDuration,
+		}
+
+		if c.opts.TagsAsTasks && len(entry.Tags) > 0 {
+			pageEntries := worklogEntry.SplitByTagsAsTasks(entry.Description, tagsAsTasksRegex, entry.Tags)
+			entries = append(entries, pageEntries...)
+		} else {
+			entries = append(entries, worklogEntry)
+		}
+	}
+
+	return entries
+}
+
 func (c *clockifyClient) FetchEntries(ctx context.Context, opts *client.FetchOpts) ([]worklog.Entry, error) {
 	var err error
 	var entries []worklog.Entry
@@ -118,58 +181,17 @@ func (c *clockifyClient) FetchEntries(ctx context.Context, opts *client.FetchOpt
 			return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err)
 		}
 
-		resp, err := client.SendRequest(ctx, http.MethodGet, searchURL, nil, &c.opts.HTTPClientOptions)
+		fetchedEntries, err := c.fetchEntries(ctx, searchURL)
 		if err != nil {
 			return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err)
 		}
 
-		var fetchedEntries []FetchEntry
-		if err = json.NewDecoder(resp.Body).Decode(&fetchedEntries); err != nil {
-			return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err)
-		}
-
 		// The API returned no entries, meaning no entries left
 		if len(fetchedEntries) == 0 {
 			break
 		}
 
-		for _, entry := range fetchedEntries {
-			billableDuration := entry.TimeInterval.End.Sub(entry.TimeInterval.Start)
-			unbillableDuration := time.Duration(0)
-
-			if !entry.Billable {
-				unbillableDuration = billableDuration
-				billableDuration = 0
-			}
-
-			worklogEntry := worklog.Entry{
-				Client: worklog.IDNameField{
-					ID:   entry.Project.ClientID,
-					Name: entry.Project.ClientName,
-				},
-				Project: worklog.IDNameField{
-					ID:   entry.Project.ID,
-					Name: entry.Project.Name,
-				},
-				Task: worklog.IDNameField{
-					ID:   entry.Task.ID,
-					Name: entry.Task.Name,
-				},
-				Summary:            entry.Task.Name,
-				Notes:              entry.Description,
-				Start:              entry.TimeInterval.Start,
-				BillableDuration:   billableDuration,
-				UnbillableDuration: unbillableDuration,
-			}
-
-			if c.opts.TagsAsTasks && len(entry.Tags) > 0 {
-				pageEntries := worklogEntry.SplitByTagsAsTasks(entry.Description, tagsAsTasksRegex, entry.Tags)
-				entries = append(entries, pageEntries...)
-			} else {
-				entries = append(entries, worklogEntry)
-			}
-		}
-
+		entries = append(entries, c.parseEntries(fetchedEntries, tagsAsTasksRegex)...)
 		currentPage++
 	}