From 08dff13aa2dc28bfdf811339612dd95f33b8f70e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Boros?= Date: Mon, 18 Oct 2021 23:09:34 +0200 Subject: [PATCH] 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 --- CHANGELOG.md | 7 + cmd/root.go | 4 +- internal/pkg/client/client.go | 38 ++-- internal/pkg/client/client_test.go | 76 ++++--- internal/pkg/client/clockify/clockify.go | 108 ++++++---- internal/pkg/client/clockify/clockify_test.go | 12 +- internal/pkg/client/tempo/tempo.go | 17 +- internal/pkg/client/tempo/tempo_test.go | 16 +- .../pkg/client/timewarrior/timewarrior.go | 186 +++++++++--------- .../client/timewarrior/timewarrior_test.go | 12 +- internal/pkg/client/toggl/toggl.go | 37 ++-- internal/pkg/client/toggl/toggl_test.go | 12 +- 12 files changed, 319 insertions(+), 206 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f3ae3ea..62da026 100644 --- 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 index f1d3d2b..02dc0ab 100644 --- 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 index df51e68..9d1a8c4 100644 --- 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 index baa2ab5..38930b7 100644 --- 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 index d778115..0bf78c2 100644 --- 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++ } diff --git a/internal/pkg/client/clockify/clockify_test.go b/internal/pkg/client/clockify/clockify_test.go index bdc3687..6bcfc19 100644 --- a/internal/pkg/client/clockify/clockify_test.go +++ b/internal/pkg/client/clockify/clockify_test.go @@ -187,7 +187,7 @@ func TestClockifyClient_FetchEntries(t *testing.T) { }) defer mockServer.Close() - httpClientOpts := &client.HTTPClientOptions{ + httpClientOpts := &client.HTTPClientOpts{ HTTPClient: http.DefaultClient, BaseURL: mockServer.URL, Token: "t-o-k-e-n", @@ -196,7 +196,7 @@ func TestClockifyClient_FetchEntries(t *testing.T) { clockifyClient := clockify.NewClient(&clockify.ClientOpts{ BaseClientOpts: client.BaseClientOpts{ - HTTPClientOptions: *httpClientOpts, + HTTPClientOpts: *httpClientOpts, }, Workspace: "marvel-studios", }) @@ -354,7 +354,7 @@ func TestClockifyClient_FetchEntries_TasksAsTags(t *testing.T) { }) defer mockServer.Close() - httpClientOpts := &client.HTTPClientOptions{ + httpClientOpts := &client.HTTPClientOpts{ HTTPClient: http.DefaultClient, BaseURL: mockServer.URL, Token: "t-o-k-e-n", @@ -363,9 +363,9 @@ func TestClockifyClient_FetchEntries_TasksAsTags(t *testing.T) { clockifyClient := clockify.NewClient(&clockify.ClientOpts{ BaseClientOpts: client.BaseClientOpts{ - HTTPClientOptions: *httpClientOpts, - TagsAsTasks: true, - TagsAsTasksRegex: `^TASK\-\d+$`, + HTTPClientOpts: *httpClientOpts, + TagsAsTasks: true, + TagsAsTasksRegex: `^TASK\-\d+$`, }, Workspace: "marvel-studios", }) diff --git a/internal/pkg/client/tempo/tempo.go b/internal/pkg/client/tempo/tempo.go index a087fab..8772977 100644 --- a/internal/pkg/client/tempo/tempo.go +++ b/internal/pkg/client/tempo/tempo.go @@ -80,7 +80,13 @@ func (c *tempoClient) FetchEntries(ctx context.Context, opts *client.FetchOpts) Worker: opts.User, } - resp, err := client.SendRequest(ctx, http.MethodPost, PathWorklogSearch, searchParams, &c.opts.HTTPClientOptions) + resp, err := client.SendRequest(ctx, &client.SendRequestOpts{ + Method: http.MethodPost, + Path: PathWorklogSearch, + ClientOpts: &c.opts.HTTPClientOpts, + Data: searchParams, + }) + if err != nil { return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err) } @@ -154,7 +160,14 @@ func (c *tempoClient) uploadEntry(ctx context.Context, entries []worklog.Entry, Worker: opts.User, } - if _, err := client.SendRequest(ctx, http.MethodPost, PathWorklogCreate, uploadEntry, &c.opts.HTTPClientOptions); err != nil { + _, err := client.SendRequest(ctx, &client.SendRequestOpts{ + Method: http.MethodPost, + Path: PathWorklogCreate, + ClientOpts: &c.opts.HTTPClientOpts, + Data: uploadEntry, + }) + + if err != nil { if tracker != nil { tracker.MarkAsErrored() } diff --git a/internal/pkg/client/tempo/tempo_test.go b/internal/pkg/client/tempo/tempo_test.go index 2d62a4b..af8e6b0 100644 --- a/internal/pkg/client/tempo/tempo_test.go +++ b/internal/pkg/client/tempo/tempo_test.go @@ -236,7 +236,7 @@ func TestTempoClient_FetchEntries(t *testing.T) { }) defer mockServer.Close() - httpClientOpts := &client.HTTPClientOptions{ + httpClientOpts := &client.HTTPClientOpts{ HTTPClient: http.DefaultClient, BaseURL: mockServer.URL, Username: clientUsername, @@ -245,7 +245,7 @@ func TestTempoClient_FetchEntries(t *testing.T) { tempoClient := tempo.NewClient(&tempo.ClientOpts{ BaseClientOpts: client.BaseClientOpts{ - HTTPClientOptions: *httpClientOpts, + HTTPClientOpts: *httpClientOpts, }, }) @@ -335,7 +335,7 @@ func TestTempoClient_UploadEntries(t *testing.T) { }) defer mockServer.Close() - httpClientOpts := &client.HTTPClientOptions{ + httpClientOpts := &client.HTTPClientOpts{ HTTPClient: http.DefaultClient, BaseURL: mockServer.URL, Username: clientUsername, @@ -344,7 +344,7 @@ func TestTempoClient_UploadEntries(t *testing.T) { tempoClient := tempo.NewClient(&tempo.ClientOpts{ BaseClientOpts: client.BaseClientOpts{ - HTTPClientOptions: *httpClientOpts, + HTTPClientOpts: *httpClientOpts, }, }) @@ -433,7 +433,7 @@ func TestTempoClient_UploadEntries_TreatDurationAsBilled(t *testing.T) { }) defer mockServer.Close() - httpClientOpts := &client.HTTPClientOptions{ + httpClientOpts := &client.HTTPClientOpts{ HTTPClient: http.DefaultClient, BaseURL: mockServer.URL, Username: clientUsername, @@ -442,7 +442,7 @@ func TestTempoClient_UploadEntries_TreatDurationAsBilled(t *testing.T) { tempoClient := tempo.NewClient(&tempo.ClientOpts{ BaseClientOpts: client.BaseClientOpts{ - HTTPClientOptions: *httpClientOpts, + HTTPClientOpts: *httpClientOpts, }, }) @@ -591,7 +591,7 @@ func TestTempoClient_UploadEntries_RoundToClosestMinute(t *testing.T) { }) defer mockServer.Close() - httpClientOpts := &client.HTTPClientOptions{ + httpClientOpts := &client.HTTPClientOpts{ HTTPClient: http.DefaultClient, BaseURL: mockServer.URL, Username: clientUsername, @@ -600,7 +600,7 @@ func TestTempoClient_UploadEntries_RoundToClosestMinute(t *testing.T) { tempoClient := tempo.NewClient(&tempo.ClientOpts{ BaseClientOpts: client.BaseClientOpts{ - HTTPClientOptions: *httpClientOpts, + HTTPClientOpts: *httpClientOpts, }, }) diff --git a/internal/pkg/client/timewarrior/timewarrior.go b/internal/pkg/client/timewarrior/timewarrior.go index e9e53ee..091b9f3 100644 --- a/internal/pkg/client/timewarrior/timewarrior.go +++ b/internal/pkg/client/timewarrior/timewarrior.go @@ -28,7 +28,7 @@ type FetchEntry struct { // ClientOpts is the client specific options, extending client.BaseClientOpts. // Since Timewarrior is a CLI tool, hence it has no API we could call on HTTP. -// Although client.HTTPClientOptions is part of client.BaseClientOpts, we are +// Although client.HTTPClientOpts is part of client.BaseClientOpts, we are // not using that as part of this integration, instead we are defining the path // of the executable (Command) and the command arguments used for export // (CommandArguments). @@ -43,10 +43,13 @@ type ClientOpts struct { } type timewarriorClient struct { - opts *ClientOpts + opts *ClientOpts + clientTagRegex *regexp.Regexp + projectTagRegex *regexp.Regexp + tagsAsTasksRegex *regexp.Regexp } -func (c *timewarriorClient) assembleCommand(subcommand string, opts *client.FetchOpts) (string, []string) { +func (c *timewarriorClient) executeCommand(ctx context.Context, subcommand string, entries *[]FetchEntry, opts *client.FetchOpts) error { arguments := []string{subcommand} arguments = append( @@ -59,120 +62,127 @@ func (c *timewarriorClient) assembleCommand(subcommand string, opts *client.Fetc arguments = append(arguments, c.opts.CommandArguments...) - return c.opts.Command, arguments -} - -func (c *timewarriorClient) FetchEntries(ctx context.Context, opts *client.FetchOpts) ([]worklog.Entry, error) { - var fetchedEntries []FetchEntry - - command, arguments := c.assembleCommand("export", opts) - - out, err := c.opts.CommandCtxExecutor(ctx, command, arguments...).Output() // #nosec G204 + out, err := c.opts.CommandCtxExecutor(ctx, c.opts.Command, arguments...).Output() // #nosec G204 if err != nil { - return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err) + return err } - if err = json.Unmarshal(out, &fetchedEntries); err != nil { - return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err) + if err = json.Unmarshal(out, &entries); err != nil { + return err } - clientTagRegex, err := regexp.Compile(c.opts.ClientTagRegex) - if err != nil { - return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err) - } + return nil +} - projectTagRegex, err := regexp.Compile(c.opts.ProjectTagRegex) +func (c *timewarriorClient) parseEntry(entry FetchEntry) ([]worklog.Entry, error) { + var entries []worklog.Entry + + startDate, err := time.ParseInLocation(ParseDateFormat, entry.Start, time.Local) if err != nil { - return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err) + return nil, err } - tagsAsTasksRegex, err := regexp.Compile(c.opts.TagsAsTasksRegex) + endDate, err := time.ParseInLocation(ParseDateFormat, entry.End, time.Local) if err != nil { - return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err) + return nil, err } - var entries []worklog.Entry - for _, entry := range fetchedEntries { - var clientName string - var projectName string - var task worklog.IDNameField + worklogEntry := worklog.Entry{ + Summary: entry.Annotation, + Notes: entry.Annotation, + Start: startDate, + BillableDuration: endDate.Sub(startDate), + UnbillableDuration: 0, + } - startDate, err := time.ParseInLocation(ParseDateFormat, entry.Start, time.Local) - if err != nil { - return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err) + for _, tag := range entry.Tags { + if tag == c.opts.UnbillableTag { + worklogEntry.UnbillableDuration = worklogEntry.BillableDuration + worklogEntry.BillableDuration = 0 + } else if c.opts.ClientTagRegex != "" && c.clientTagRegex.MatchString(tag) { + worklogEntry.Client = worklog.IDNameField{ + ID: tag, + Name: tag, + } + } else if c.opts.ProjectTagRegex != "" && c.projectTagRegex.MatchString(tag) { + worklogEntry.Project = worklog.IDNameField{ + ID: tag, + Name: tag, + } + } else if c.opts.TagsAsTasksRegex != "" && c.tagsAsTasksRegex.MatchString(tag) { + worklogEntry.Task = worklog.IDNameField{ + ID: tag, + Name: tag, + } } + } - endDate, err := time.ParseInLocation(ParseDateFormat, entry.End, time.Local) - if err != nil { - return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err) + // If the task was not found in tags, make sure to set it to annotation + if !worklogEntry.Task.IsComplete() { + worklogEntry.Task = worklog.IDNameField{ + ID: entry.Annotation, + Name: entry.Annotation, } + } - billableDuration := endDate.Sub(startDate) - unbillableDuration := time.Duration(0) - + if c.opts.TagsAsTasks && len(entry.Tags) > 0 { + var tags []worklog.IDNameField for _, tag := range entry.Tags { - if tag == c.opts.UnbillableTag { - unbillableDuration = billableDuration - billableDuration = 0 - } else if c.opts.ClientTagRegex != "" && clientTagRegex.MatchString(tag) { - clientName = tag - } else if c.opts.ProjectTagRegex != "" && projectTagRegex.MatchString(tag) { - projectName = tag - } else if c.opts.TagsAsTasksRegex != "" && tagsAsTasksRegex.MatchString(tag) { - task = worklog.IDNameField{ - ID: tag, - Name: tag, - } - } + tags = append(tags, worklog.IDNameField{ + ID: tag, + Name: tag, + }) } - // If the task was not found in tags, make sure to set it to annotation - if !task.IsComplete() { - task = worklog.IDNameField{ - ID: entry.Annotation, - Name: entry.Annotation, - } - } + splitEntries := worklogEntry.SplitByTagsAsTasks(worklogEntry.Summary, c.tagsAsTasksRegex, tags) + entries = append(entries, splitEntries...) + } else { + entries = append(entries, worklogEntry) + } - worklogEntry := worklog.Entry{ - Client: worklog.IDNameField{ - ID: clientName, - Name: clientName, - }, - Project: worklog.IDNameField{ - ID: projectName, - Name: projectName, - }, - Task: task, - Summary: entry.Annotation, - Notes: entry.Annotation, - Start: startDate, - BillableDuration: billableDuration, - UnbillableDuration: unbillableDuration, - } + return entries, nil +} - if c.opts.TagsAsTasks && len(entry.Tags) > 0 { - var tags []worklog.IDNameField - for _, tag := range entry.Tags { - tags = append(tags, worklog.IDNameField{ - ID: tag, - Name: tag, - }) - } +func (c *timewarriorClient) FetchEntries(ctx context.Context, opts *client.FetchOpts) ([]worklog.Entry, error) { + var fetchedEntries []FetchEntry + if err := c.executeCommand(ctx, "export", &fetchedEntries, opts); err != nil { + return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err) + } - splitEntries := worklogEntry.SplitByTagsAsTasks(worklogEntry.Summary, tagsAsTasksRegex, tags) - entries = append(entries, splitEntries...) - } else { - entries = append(entries, worklogEntry) + var entries []worklog.Entry + for _, entry := range fetchedEntries { + parsedEntries, err := c.parseEntry(entry) + if err != nil { + return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err) } + + entries = append(entries, parsedEntries...) } return entries, nil } // NewClient returns a new Timewarrior client. -func NewClient(opts *ClientOpts) client.Fetcher { - return &timewarriorClient{ - opts: opts, +func NewClient(opts *ClientOpts) (client.Fetcher, error) { + clientTagRegex, err := regexp.Compile(opts.ClientTagRegex) + if err != nil { + return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err) + } + + projectTagRegex, err := regexp.Compile(opts.ProjectTagRegex) + if err != nil { + return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err) } + + tagsAsTasksRegex, err := regexp.Compile(opts.TagsAsTasksRegex) + if err != nil { + return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err) + } + + return &timewarriorClient{ + opts: opts, + clientTagRegex: clientTagRegex, + projectTagRegex: projectTagRegex, + tagsAsTasksRegex: tagsAsTasksRegex, + }, nil } diff --git a/internal/pkg/client/timewarrior/timewarrior_test.go b/internal/pkg/client/timewarrior/timewarrior_test.go index b68fe1b..19993d1 100644 --- a/internal/pkg/client/timewarrior/timewarrior_test.go +++ b/internal/pkg/client/timewarrior/timewarrior_test.go @@ -117,7 +117,7 @@ func TestTimewarriorClient_FetchEntries(t *testing.T) { }, } - timewarriorClient := timewarrior.NewClient(&timewarrior.ClientOpts{ + timewarriorClient, err := timewarrior.NewClient(&timewarrior.ClientOpts{ BaseClientOpts: client.BaseClientOpts{}, Command: "timewarrior-command", CommandArguments: []string{}, @@ -127,6 +127,8 @@ func TestTimewarriorClient_FetchEntries(t *testing.T) { ProjectTagRegex: "^(project)$", }) + require.Nil(t, err) + entries, err := timewarriorClient.FetchEntries(context.Background(), &client.FetchOpts{ Start: start, End: end, @@ -207,7 +209,7 @@ func TestTimewarriorClient_FetchEntries_TagsAsTasksRegex_NoSplit(t *testing.T) { }, } - timewarriorClient := timewarrior.NewClient(&timewarrior.ClientOpts{ + timewarriorClient, err := timewarrior.NewClient(&timewarrior.ClientOpts{ BaseClientOpts: client.BaseClientOpts{ TagsAsTasks: false, TagsAsTasksRegex: `^TASK\-\d+$`, @@ -220,6 +222,8 @@ func TestTimewarriorClient_FetchEntries_TagsAsTasksRegex_NoSplit(t *testing.T) { ProjectTagRegex: "^(project)$", }) + require.Nil(t, err) + entries, err := timewarriorClient.FetchEntries(context.Background(), &client.FetchOpts{ Start: start, End: end, @@ -319,7 +323,7 @@ func TestTimewarriorClient_FetchEntries_TagsAsTasks(t *testing.T) { }, } - timewarriorClient := timewarrior.NewClient(&timewarrior.ClientOpts{ + timewarriorClient, err := timewarrior.NewClient(&timewarrior.ClientOpts{ BaseClientOpts: client.BaseClientOpts{ TagsAsTasks: true, TagsAsTasksRegex: `^TASK\-\d+$`, @@ -332,6 +336,8 @@ func TestTimewarriorClient_FetchEntries_TagsAsTasks(t *testing.T) { ProjectTagRegex: "^(project)$", }) + require.Nil(t, err) + entries, err := timewarriorClient.FetchEntries(context.Background(), &client.FetchOpts{ Start: start, End: end, diff --git a/internal/pkg/client/toggl/toggl.go b/internal/pkg/client/toggl/toggl.go index 30cbde1..d9aa129 100644 --- a/internal/pkg/client/toggl/toggl.go +++ b/internal/pkg/client/toggl/toggl.go @@ -130,6 +130,31 @@ func (c *togglClient) parseEntries(fetchedEntries []FetchEntry, tagsAsTasksRegex return entries, nil } +func (c *togglClient) fetchEntries(ctx context.Context, path string, tagsAsTasksRegex *regexp.Regexp) ([]worklog.Entry, *PaginatedResponse, error) { + resp, err := client.SendRequest(ctx, &client.SendRequestOpts{ + Method: http.MethodGet, + Path: path, + ClientOpts: &c.opts.HTTPClientOpts, + Data: nil, + }) + + if err != nil { + return nil, nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err) + } + + var paginatedResponse PaginatedResponse + if err = json.NewDecoder(resp.Body).Decode(&paginatedResponse); err != nil { + return nil, nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err) + } + + parsedEntries, err := c.parseEntries(paginatedResponse.Data, tagsAsTasksRegex) + if err != nil { + return nil, nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err) + } + + return parsedEntries, &paginatedResponse, err +} + func (c *togglClient) FetchEntries(ctx context.Context, opts *client.FetchOpts) ([]worklog.Entry, error) { var err error var entries []worklog.Entry @@ -159,21 +184,11 @@ func (c *togglClient) FetchEntries(ctx context.Context, opts *client.FetchOpts) return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err) } - resp, err := client.SendRequest(ctx, http.MethodGet, searchURL, nil, &c.opts.HTTPClientOptions) + parsedEntries, paginatedResponse, err := c.fetchEntries(ctx, searchURL, tagsAsTasksRegex) if err != nil { return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err) } - var paginatedResponse PaginatedResponse - if err = json.NewDecoder(resp.Body).Decode(&paginatedResponse); err != nil { - return nil, fmt.Errorf("%v: %v", client.ErrFetchEntries, err) - } - - parsedEntries, err := c.parseEntries(paginatedResponse.Data, tagsAsTasksRegex) - if err != nil { - return nil, err - } - entries = append(entries, parsedEntries...) pageSize = paginatedResponse.PerPage diff --git a/internal/pkg/client/toggl/toggl_test.go b/internal/pkg/client/toggl/toggl_test.go index 0e45fbd..513289d 100644 --- a/internal/pkg/client/toggl/toggl_test.go +++ b/internal/pkg/client/toggl/toggl_test.go @@ -147,7 +147,7 @@ func TestTogglClient_FetchEntries(t *testing.T) { }) defer mockServer.Close() - httpClientOpts := &client.HTTPClientOptions{ + httpClientOpts := &client.HTTPClientOpts{ HTTPClient: http.DefaultClient, BaseURL: mockServer.URL, Username: clientUsername, @@ -156,7 +156,7 @@ func TestTogglClient_FetchEntries(t *testing.T) { togglClient := toggl.NewClient(&toggl.ClientOpts{ BaseClientOpts: client.BaseClientOpts{ - HTTPClientOptions: *httpClientOpts, + HTTPClientOpts: *httpClientOpts, }, Workspace: 123456789, }) @@ -286,7 +286,7 @@ func TestTogglClient_FetchEntries_TagsAsTasks(t *testing.T) { }) defer mockServer.Close() - httpClientOpts := &client.HTTPClientOptions{ + httpClientOpts := &client.HTTPClientOpts{ HTTPClient: http.DefaultClient, BaseURL: mockServer.URL, Username: clientUsername, @@ -295,9 +295,9 @@ func TestTogglClient_FetchEntries_TagsAsTasks(t *testing.T) { togglClient := toggl.NewClient(&toggl.ClientOpts{ BaseClientOpts: client.BaseClientOpts{ - HTTPClientOptions: *httpClientOpts, - TagsAsTasks: true, - TagsAsTasksRegex: `^CPT\-\w+$`, + HTTPClientOpts: *httpClientOpts, + TagsAsTasks: true, + TagsAsTasksRegex: `^CPT\-\w+$`, }, Workspace: 123456789, })