diff --git a/beeminder.go b/beeminder.go index 7ffa916..f49790f 100644 --- a/beeminder.go +++ b/beeminder.go @@ -329,27 +329,35 @@ func GetLastDatapointValue(config *Config, goalSlug string) (float64, error) { return result.LastDatapoint.Value, nil } -// CreateDatapoint submits a new datapoint to a Beeminder goal -func CreateDatapoint(config *Config, goalSlug, timestamp, value, comment string) error { +// CreateDatapoint submits a new datapoint to a Beeminder goal and returns the created datapoint +func CreateDatapoint(config *Config, goalSlug, timestamp, value, comment string) (*Datapoint, error) { baseURL := getBaseURL(config) - url := fmt.Sprintf("%s/api/v1/users/%s/goals/%s/datapoints.json", - baseURL, config.Username, goalSlug) + apiURL := fmt.Sprintf("%s/api/v1/users/%s/goals/%s/datapoints.json", + baseURL, config.Username, url.PathEscape(goalSlug)) - data := fmt.Sprintf("auth_token=%s×tamp=%s&value=%s&comment=%s", - config.AuthToken, timestamp, value, comment) + data := url.Values{} + data.Set("auth_token", config.AuthToken) + data.Set("timestamp", timestamp) + data.Set("value", value) + data.Set("comment", comment) - resp, err := http.Post(url, "application/x-www-form-urlencoded", - strings.NewReader(data)) + resp, err := http.Post(apiURL, "application/x-www-form-urlencoded", + strings.NewReader(data.Encode())) if err != nil { - return fmt.Errorf("failed to create datapoint: %w", err) + return nil, fmt.Errorf("failed to create datapoint: %w", err) } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return fmt.Errorf("API returned status %d", resp.StatusCode) + return nil, fmt.Errorf("API returned status %d", resp.StatusCode) + } + + var datapoint Datapoint + if err := json.NewDecoder(resp.Body).Decode(&datapoint); err != nil { + return nil, fmt.Errorf("failed to decode datapoint response: %w", err) } - return nil + return &datapoint, nil } // CreateCharge creates a new charge for the authenticated user and returns it @@ -419,10 +427,10 @@ func FetchGoal(config *Config, goalSlug string) (*Goal, error) { // FetchGoalWithDatapoints fetches goal details including recent datapoints func FetchGoalWithDatapoints(config *Config, goalSlug string) (*Goal, error) { baseURL := getBaseURL(config) - url := fmt.Sprintf("%s/api/v1/users/%s/goals/%s.json?auth_token=%s&datapoints=true", - baseURL, config.Username, goalSlug, config.AuthToken) + apiURL := fmt.Sprintf("%s/api/v1/users/%s/goals/%s.json?auth_token=%s&datapoints=true", + baseURL, config.Username, url.PathEscape(goalSlug), config.AuthToken) - resp, err := http.Get(url) + resp, err := http.Get(apiURL) if err != nil { return nil, fmt.Errorf("failed to fetch goal details: %w", err) } @@ -533,3 +541,60 @@ func RefreshGoal(config *Config, goalSlug string) (bool, error) { return result, nil } + +// WaitForDatapoint polls the goal's recent datapoints until the specified datapoint ID appears +// or the timeout is reached. Returns true if the datapoint was found, false if timeout occurred. +// The error return value is used to indicate permanent failures (e.g., authentication errors, +// non-existent goals) where continuing to poll would be futile. +// Note: A zero or negative timeout will result in at least one polling attempt before checking the deadline. +func WaitForDatapoint(config *Config, goalSlug, datapointID string, timeout, pollInterval time.Duration) (bool, error) { + deadline := time.Now().Add(timeout) + + for { + // Fetch the goal with datapoints + goal, err := FetchGoalWithDatapoints(config, goalSlug) + if err != nil { + // Check if this is a permanent error that we shouldn't retry + if isPermanentError(err) { + return false, err + } + // For transient errors, check if we should continue polling + if !time.Now().Before(deadline) { + // Timeout reached + return false, nil + } + // Wait before retry on transient errors + time.Sleep(pollInterval) + continue + } + + // Check if the datapoint ID exists in the recent datapoints + for _, dp := range goal.Datapoints { + if dp.ID == datapointID { + return true, nil + } + } + + // Check if we've exceeded the deadline + if !time.Now().Before(deadline) { + // Timeout reached without finding the datapoint + return false, nil + } + + // Wait before next poll + time.Sleep(pollInterval) + } +} + +// isPermanentError checks if an error indicates a permanent failure that shouldn't be retried +func isPermanentError(err error) bool { + if err == nil { + return false + } + errMsg := err.Error() + // Check for HTTP status codes that indicate permanent failures + // 401 Unauthorized, 403 Forbidden, 404 Not Found + return strings.Contains(errMsg, "status 401") || + strings.Contains(errMsg, "status 403") || + strings.Contains(errMsg, "status 404") +} diff --git a/beeminder_test.go b/beeminder_test.go index 14382c0..d6f91fd 100644 --- a/beeminder_test.go +++ b/beeminder_test.go @@ -2,6 +2,7 @@ package main import ( "encoding/json" + "fmt" "net/http" "net/http/httptest" "net/url" @@ -1368,3 +1369,257 @@ func TestGoalTypeField(t *testing.T) { }) } } + +// TestWaitForDatapoint tests the WaitForDatapoint polling function +func TestWaitForDatapoint(t *testing.T) { + t.Run("finds datapoint immediately", func(t *testing.T) { + // Create a mock server that returns a goal with the target datapoint + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Check for datapoints query parameter + if !strings.Contains(r.URL.RawQuery, "datapoints=true") { + t.Error("Expected datapoints=true query parameter") + } + + goal := Goal{ + Slug: "testgoal", + Datapoints: []Datapoint{ + {ID: "target-id", Value: 1.0, Comment: "test"}, + {ID: "other-id", Value: 2.0, Comment: "other"}, + }, + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(goal) + })) + defer mockServer.Close() + + config := &Config{ + Username: "testuser", + AuthToken: "testtoken", + BaseURL: mockServer.URL, + } + + // Should find the datapoint immediately + found, err := WaitForDatapoint(config, "testgoal", "target-id", 1*time.Second, 100*time.Millisecond) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + if !found { + t.Error("Expected to find datapoint immediately") + } + }) + + t.Run("times out when datapoint not found", func(t *testing.T) { + // Create a mock server that never returns the target datapoint + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + goal := Goal{ + Slug: "testgoal", + Datapoints: []Datapoint{ + {ID: "other-id", Value: 2.0, Comment: "other"}, + }, + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(goal) + })) + defer mockServer.Close() + + config := &Config{ + Username: "testuser", + AuthToken: "testtoken", + BaseURL: mockServer.URL, + } + + // Should timeout without finding the datapoint + start := time.Now() + found, err := WaitForDatapoint(config, "testgoal", "missing-id", 300*time.Millisecond, 100*time.Millisecond) + elapsed := time.Since(start) + + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + if found { + t.Error("Expected not to find datapoint (timeout)") + } + // Verify it actually waited for approximately the timeout duration + if elapsed < 250*time.Millisecond { + t.Errorf("Expected to wait at least 250ms, but only waited %v", elapsed) + } + }) + + t.Run("continues polling on fetch errors", func(t *testing.T) { + callCount := 0 + // Create a mock server that returns an error first, then success + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount++ + if callCount == 1 { + // First call returns error + w.WriteHeader(http.StatusInternalServerError) + return + } + // Second call returns success with the datapoint + goal := Goal{ + Slug: "testgoal", + Datapoints: []Datapoint{ + {ID: "target-id", Value: 1.0, Comment: "test"}, + }, + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(goal) + })) + defer mockServer.Close() + + config := &Config{ + Username: "testuser", + AuthToken: "testtoken", + BaseURL: mockServer.URL, + } + + // Should retry after error and eventually find the datapoint + found, err := WaitForDatapoint(config, "testgoal", "target-id", 1*time.Second, 100*time.Millisecond) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + if !found { + t.Error("Expected to find datapoint after retrying") + } + if callCount < 2 { + t.Errorf("Expected at least 2 API calls, got %d", callCount) + } + }) + + t.Run("returns error on permanent failure", func(t *testing.T) { + // Create a mock server that always returns 404 + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer mockServer.Close() + + config := &Config{ + Username: "testuser", + AuthToken: "testtoken", + BaseURL: mockServer.URL, + } + + // Should return error immediately on permanent failure + start := time.Now() + found, err := WaitForDatapoint(config, "testgoal", "target-id", 5*time.Second, 100*time.Millisecond) + elapsed := time.Since(start) + + if err == nil { + t.Error("Expected error on permanent failure, got nil") + } + if found { + t.Error("Expected not to find datapoint on error") + } + // Should fail fast, not wait for full timeout + if elapsed > 1*time.Second { + t.Errorf("Expected to fail fast (< 1s), but took %v", elapsed) + } + }) + + t.Run("handles zero timeout with at least one attempt", func(t *testing.T) { + callCount := 0 + // Create a mock server that tracks calls + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount++ + goal := Goal{ + Slug: "testgoal", + Datapoints: []Datapoint{ + {ID: "target-id", Value: 1.0, Comment: "test"}, + }, + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(goal) + })) + defer mockServer.Close() + + config := &Config{ + Username: "testuser", + AuthToken: "testtoken", + BaseURL: mockServer.URL, + } + + // Should make at least one attempt even with zero timeout + found, err := WaitForDatapoint(config, "testgoal", "target-id", 0, 100*time.Millisecond) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + if !found { + t.Error("Expected to find datapoint on first attempt") + } + if callCount < 1 { + t.Errorf("Expected at least 1 API call with zero timeout, got %d", callCount) + } + }) +} + +// TestIsPermanentError tests the isPermanentError function +func TestIsPermanentError(t *testing.T) { + tests := []struct { + name string + err error + expected bool + }{ + {"nil error", nil, false}, + {"401 unauthorized", fmt.Errorf("API returned status 401"), true}, + {"403 forbidden", fmt.Errorf("API returned status 403"), true}, + {"404 not found", fmt.Errorf("API returned status 404"), true}, + {"500 server error", fmt.Errorf("API returned status 500"), false}, + {"network error", fmt.Errorf("connection refused"), false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isPermanentError(tt.err) + if result != tt.expected { + t.Errorf("isPermanentError(%v) = %v, want %v", tt.err, result, tt.expected) + } + }) + } +} + +// TestCreateDatapointReturnsDatapoint tests that CreateDatapoint returns the created datapoint +func TestCreateDatapointReturnsDatapoint(t *testing.T) { + // Create a mock server that returns a datapoint + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + t.Errorf("Expected POST request, got %s", r.Method) + } + + // Return a mock datapoint response + datapoint := Datapoint{ + ID: "12345", + Timestamp: 1234567890, + Value: 42.0, + Comment: "test datapoint", + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(datapoint) + })) + defer mockServer.Close() + + config := &Config{ + Username: "testuser", + AuthToken: "testtoken", + BaseURL: mockServer.URL, + } + + // Call CreateDatapoint + datapoint, err := CreateDatapoint(config, "testgoal", "1234567890", "42", "test datapoint") + if err != nil { + t.Fatalf("CreateDatapoint failed: %v", err) + } + + // Verify the returned datapoint + if datapoint == nil { + t.Fatal("Expected datapoint to be returned, got nil") + } + if datapoint.ID != "12345" { + t.Errorf("Expected ID '12345', got '%s'", datapoint.ID) + } + if datapoint.Value != 42.0 { + t.Errorf("Expected Value 42.0, got %v", datapoint.Value) + } + if datapoint.Comment != "test datapoint" { + t.Errorf("Expected Comment 'test datapoint', got '%s'", datapoint.Comment) + } +} diff --git a/handlers_test.go b/handlers_test.go index 5f35e60..87aa075 100644 --- a/handlers_test.go +++ b/handlers_test.go @@ -1288,68 +1288,3 @@ func TestScrollFollowsNavigation(t *testing.T) { } }) } - -// TestLimsumFetchDelay tests the delay behavior before fetching limsum after adding a datapoint -func TestLimsumFetchDelay(t *testing.T) { - t.Run("limsumFetchDelay has correct default value", func(t *testing.T) { - // Verify the default delay is 2 seconds - if limsumFetchDelay != 2*time.Second { - t.Errorf("limsumFetchDelay = %v, want %v", limsumFetchDelay, 2*time.Second) - } - }) - - t.Run("limsumFetchDelay can be overridden for testing", func(t *testing.T) { - // Store the original value and defer restoration - originalDelay := limsumFetchDelay - defer func() { limsumFetchDelay = originalDelay }() - - // Override the delay for testing - limsumFetchDelay = 1 * time.Millisecond - - // Verify the delay is now 1 millisecond - if limsumFetchDelay != 1*time.Millisecond { - t.Errorf("limsumFetchDelay = %v, want %v", limsumFetchDelay, 1*time.Millisecond) - } - }) - - t.Run("delay is applied correctly", func(t *testing.T) { - // Store the original value and defer restoration - originalDelay := limsumFetchDelay - defer func() { limsumFetchDelay = originalDelay }() - - // Set a short delay for testing - testDelay := 50 * time.Millisecond - limsumFetchDelay = testDelay - - // Measure the time it takes to execute the delay - start := time.Now() - time.Sleep(limsumFetchDelay) - elapsed := time.Since(start) - - // Verify the delay was applied (allow 10ms tolerance) - if elapsed < testDelay { - t.Errorf("delay was not applied correctly: elapsed %v, want at least %v", elapsed, testDelay) - } - }) - - t.Run("delay does not block error handling", func(t *testing.T) { - // Store the original value and defer restoration - originalDelay := limsumFetchDelay - defer func() { limsumFetchDelay = originalDelay }() - - // Set a short delay for testing - limsumFetchDelay = 1 * time.Millisecond - - // Simulate the delay followed by an error condition - var fetchError error - time.Sleep(limsumFetchDelay) - - // Simulate a fetch error - fetchError = nil // In a real scenario, this would be set by FetchGoal - - // Verify that after the delay, error handling is still possible - if fetchError != nil { - t.Errorf("error handling should work after delay, got error: %v", fetchError) - } - }) -} diff --git a/main.go b/main.go index 6204c7a..74ecc58 100644 --- a/main.go +++ b/main.go @@ -23,11 +23,6 @@ var version = "dev" // navigationTimeout is the duration of inactivity before the cell highlight is auto-disabled const navigationTimeout = 3 * time.Second -// limsumFetchDelay is the duration to wait after adding a datapoint before fetching the updated limsum -// This gives the Beeminder server time to update the goal's limsum with the new datapoint -// This is a variable (not const) to allow tests to override it -var limsumFetchDelay = 2 * time.Second - func (m model) Init() tea.Cmd { if m.state == "auth" { return m.authModel.Init() @@ -609,7 +604,7 @@ func handleAddCommand() { } // Create the datapoint - err = CreateDatapoint(config, goalSlug, timestamp, value, comment) + datapoint, err := CreateDatapoint(config, goalSlug, timestamp, value, comment) if err != nil { fmt.Printf("Error: Failed to add datapoint: %v\n", err) os.Exit(1) @@ -623,8 +618,16 @@ func handleAddCommand() { fmt.Printf("Successfully added datapoint to %s: value=%s, comment=\"%s\"\n", goalSlug, value, comment) - // Wait briefly before fetching limsum to allow the server to update - time.Sleep(limsumFetchDelay) + // Poll for the datapoint to appear in the goal's recent data + // This ensures the server has processed the datapoint before we fetch limsum + pollingTimeout := 10 * time.Second + pollInterval := 500 * time.Millisecond + found, err := WaitForDatapoint(config, goalSlug, datapoint.ID, pollingTimeout, pollInterval) + if err != nil { + fmt.Fprintf(os.Stderr, "Warning: Error while waiting for datapoint: %v\n", err) + } else if !found { + fmt.Fprintf(os.Stderr, "Warning: Datapoint not found in recent data after timeout\n") + } // Fetch the goal to display the updated limsum goal, err := FetchGoal(config, goalSlug) diff --git a/messages.go b/messages.go index 9fb01ff..f597757 100644 --- a/messages.go +++ b/messages.go @@ -61,9 +61,12 @@ func refreshTickCmd() tea.Cmd { } // submitDatapointCmd submits a datapoint to Beeminder API +// Note: The TUI doesn't poll for datapoint appearance like handleAddCommand does. +// Instead, it triggers a full goal refresh after submission (see datapointSubmittedMsg handling), +// which will fetch updated data including the new datapoint and limsum. func submitDatapointCmd(config *Config, goalSlug, timestamp, value, comment string) tea.Cmd { return func() tea.Msg { - err := CreateDatapoint(config, goalSlug, timestamp, value, comment) + _, err := CreateDatapoint(config, goalSlug, timestamp, value, comment) return datapointSubmittedMsg{err: err} } }