Fix badge challenge endpoints returning 400 (add desc=true, 1-based start)#331
Open
sebmill wants to merge 4 commits intocyberjunky:masterfrom
Open
Fix badge challenge endpoints returning 400 (add desc=true, 1-based start)#331sebmill wants to merge 4 commits intocyberjunky:masterfrom
sebmill wants to merge 4 commits intocyberjunky:masterfrom
Conversation
Contributor
WalkthroughAdds web-proxy wrapper and endpoints for nutrition and golf, activity import and workout scheduling methods, demo/menu integrations, README and CI/version updates, and changes Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Merge conflicts detected
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@garminconnect/__init__.py`:
- Line 1375: Extract the repeated pagination dict construction (params =
{"desc": "true", "start": str(start + 1), "limit": str(limit)}) into a small
typed helper function (e.g. def _badge_pagination_params(start: int, limit: int)
-> dict[str, str]) and replace the four inline occurrences with calls to this
helper; ensure the helper returns the same keys/values and use consistent type
hints (dict[str, str] or typing.Dict[str, str]) as per project style.
- Line 1375: Revert the unexpected 1-based offset: in the params building in
garminconnect.__init__.py (where params = {"desc": "true", "start": str(start +
1), "limit": str(limit)}) remove the "+ 1" so the start is passed as str(start).
Ensure callers that already use demo.py's start_badge (which is set to 1) are
left unchanged or update demo.py and badge-related caller sites to use 0-based
indexing consistently; if the backend truly requires 1-based values, move the +1
adjustment to the single place that sends to the external API (not in the shared
params creation) and add a comment clarifying indexing expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6f69a4a8-5178-4409-a87d-a0267569d7c8
📒 Files selected for processing (1)
garminconnect/__init__.py
garminconnect/__init__.py
Outdated
| limit = _validate_positive_integer(limit, "limit") | ||
| url = self.garmin_connect_badge_challenges_url | ||
| params = {"start": str(start), "limit": str(limit)} | ||
| params = {"desc": "true", "start": str(start + 1), "limit": str(limit)} |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Deduplicate badge pagination param construction
The same param dict is repeated in four methods. Extracting a typed helper avoids future drift if Garmin changes required query parameters again.
♻️ Proposed refactor
+ `@staticmethod`
+ def _badge_challenge_params(start: int, limit: int) -> dict[str, str]:
+ return {"desc": "true", "start": str(start + 1), "limit": str(limit)}
+
def get_badge_challenges(self, start: int, limit: int) -> dict[str, Any]:
"""Return badge challenges for current user."""
start = _validate_non_negative_integer(start, "start")
limit = _validate_positive_integer(limit, "limit")
url = self.garmin_connect_badge_challenges_url
- params = {"desc": "true", "start": str(start + 1), "limit": str(limit)}
+ params = self._badge_challenge_params(start, limit)
logger.debug("Requesting badge challenges for user")
return self.connectapi(url, params=params)
@@
def get_available_badge_challenges(self, start: int, limit: int) -> dict[str, Any]:
@@
- params = {"desc": "true", "start": str(start + 1), "limit": str(limit)}
+ params = self._badge_challenge_params(start, limit)
@@
def get_non_completed_badge_challenges(
self, start: int, limit: int
) -> dict[str, Any]:
@@
- params = {"desc": "true", "start": str(start + 1), "limit": str(limit)}
+ params = self._badge_challenge_params(start, limit)
@@
def get_inprogress_virtual_challenges(
self, start: int, limit: int
) -> dict[str, Any]:
@@
- params = {"desc": "true", "start": str(start + 1), "limit": str(limit)}
+ params = self._badge_challenge_params(start, limit)As per coding guidelines, "use type hints consistently".
Also applies to: 1385-1385, 1397-1397, 1409-1409
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@garminconnect/__init__.py` at line 1375, Extract the repeated pagination dict
construction (params = {"desc": "true", "start": str(start + 1), "limit":
str(limit)}) into a small typed helper function (e.g. def
_badge_pagination_params(start: int, limit: int) -> dict[str, str]) and replace
the four inline occurrences with calls to this helper; ensure the helper returns
the same keys/values and use consistent type hints (dict[str, str] or
typing.Dict[str, str]) as per project style.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find badge challenge callsites and start semantics =="
rg -n -C2 'get_(badge_challenges|available_badge_challenges|non_completed_badge_challenges|inprogress_virtual_challenges)\('
echo
echo "== Find explicit 1-based assumptions =="
rg -n -C2 'start_badge\s*=\s*1|Badge related calls start counting at 1|first page.*start\s*=\s*1|1-based'Repository: cyberjunky/python-garminconnect
Length of output: 3512
Confirmed: Breaking change in badge challenge pagination will skip first page for existing callers
Lines 1375, 1385, 1397, and 1409 add + 1 to the start parameter. This assumes a 0-based API contract, but demo.py (line 91) explicitly configures start_badge = 1 with the comment "Badge related calls start counting at 1", and all three badge challenge methods (lines 3680, 3711, 3718) call these endpoints using that 1-based value. After this change, those calls will request page 2 and skip first-page results without any warning.
Fix: Either (a) update demo.py and release notes to require 0-based start values with migration guidance for users, or (b) revert the + 1 adjustment and correct at the API call site only if the backend actually requires 1-based input.
Also applies to: 1385, 1397, 1409
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@garminconnect/__init__.py` at line 1375, Revert the unexpected 1-based
offset: in the params building in garminconnect.__init__.py (where params =
{"desc": "true", "start": str(start + 1), "limit": str(limit)}) remove the "+ 1"
so the start is passed as str(start). Ensure callers that already use demo.py's
start_badge (which is set to 1) are left unchanged or update demo.py and
badge-related caller sites to use 0-based indexing consistently; if the backend
truly requires 1-based values, move the +1 adjustment to the single place that
sends to the external API (not in the shared params creation) and add a comment
clarifying indexing expectations.
Owner
|
Strange, the demo program still fetches my badge challenges just fine?? |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
garminconnect/__init__.py (1)
1418-1418:⚠️ Potential issue | 🟠 MajorBreaking change: Existing callers using 1-based
startwill skip the first page.The PR objective states the public API should remain 0-based with internal conversion to 1-based. However, the reviewer's comment shows
get_available_badge_challenges(1, 100)still works, which means callers usingstart=1will now request page 2 from Garmin's API (since1 + 1 = 2).If demo.py and existing users already pass 1-based values expecting the old behavior, they will silently skip results. Consider:
- Updating demo.py and documentation to use
start=0for the first page- Adding a release note about this breaking change
- Optionally: detecting and warning if
start >= 1during a deprecation period🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garminconnect/__init__.py` at line 1418, The change increments the public 0-based start parameter (in get_available_badge_challenges) causing callers who pass start=1 to request the wrong page; revert the unconditional "+ 1" conversion so external API remains 0-based (remove or conditionally apply the start+1 when translating only for Garmin API calls), update the params assignment logic that builds params = {"desc":"true","start": str(start + 1),...} to correctly convert 0-based -> 1-based internally without breaking callers, and add a short deprecation warning (e.g., log.warn) inside get_available_badge_challenges when start >= 1 during the transition period and update demo/docs to use start=0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demo.py`:
- Around line 2462-2468: The display strings use the wrong method name: update
the call to call_and_display that invokes api.schedule_workout so the
method_name and api_call_desc reflect "schedule_workout" (not
"scheduled_workout"); edit the parameters in the call_and_display invocation
(method_name and api_call_desc) to reference api.schedule_workout and the
correct function name, preserving the existing workout_id, schedule_date, and
workout_name arguments.
In `@garminconnect/__init__.py`:
- Around line 1923-2018: The file validation block duplicated in import_activity
(validate file existence, is_file, filename, extension and allowed formats)
should be extracted into a private helper (e.g., _validate_activity_file) that
returns the Path and normalized extension and is reused by both upload_activity
and import_activity; update import_activity to call _validate_activity_file
(replace checks from the block around file_base_name/file_extension) and keep
identical error types/messages; additionally, add a short inline comment next to
the multipart filename construction in import_activity (the line building the
files dict with f'"{file_base_name}"') clarifying that the embedded quotes are
intentional to mimic Garmin Connect Mobile multipart encoding so future
maintainers understand why the filename is wrapped in quotes.
- Around line 364-391: The connectwebproxy method currently only catches
GarthHTTPError, causing inconsistent handling versus connectapi; update
connectwebproxy to mirror connectapi by also catching HTTPError and
AssertionError from garth.request and handling them the same way (inspect
response.status_code, log via logger.exception, raise
GarminConnectAuthenticationError on 401, GarminConnectTooManyRequestsError on
429, GarminConnectConnectionError for other 4xx and general errors), and ensure
AssertionError raised during OAuth refresh is converted to
GarminConnectAuthenticationError as connectapi does.
In `@README.md`:
- Around line 385-388: Add a blank line immediately before the "### Additional
Resources" heading in README.md so the heading is separated from the previous
paragraph (ensure the line containing "### Additional Resources" is preceded by
an empty line); this satisfies markdownlint MD022 and ensures proper rendering
of that section.
---
Duplicate comments:
In `@garminconnect/__init__.py`:
- Line 1418: The change increments the public 0-based start parameter (in
get_available_badge_challenges) causing callers who pass start=1 to request the
wrong page; revert the unconditional "+ 1" conversion so external API remains
0-based (remove or conditionally apply the start+1 when translating only for
Garmin API calls), update the params assignment logic that builds params =
{"desc":"true","start": str(start + 1),...} to correctly convert 0-based ->
1-based internally without breaking callers, and add a short deprecation warning
(e.g., log.warn) inside get_available_badge_challenges when start >= 1 during
the transition period and update demo/docs to use start=0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 18ebd57d-4be1-4173-9d17-3e2e16e619bb
📒 Files selected for processing (5)
.github/workflows/ci.ymlREADME.mddemo.pygarminconnect/__init__.pypyproject.toml
Comment on lines
+2462
to
+2468
| call_and_display( | ||
| api.schedule_workout, | ||
| workout_id, | ||
| schedule_date, | ||
| method_name="scheduled_workout", | ||
| api_call_desc=f"api.scheduled_workout({workout_id}, '{schedule_date}') - {workout_name}", | ||
| ) |
Contributor
There was a problem hiding this comment.
Inconsistent method name in display strings.
The actual API method called is api.schedule_workout() but the method_name and api_call_desc parameters display scheduled_workout. This inconsistency could confuse users reviewing the demo output.
🔧 Proposed fix
call_and_display(
api.schedule_workout,
workout_id,
schedule_date,
- method_name="scheduled_workout",
- api_call_desc=f"api.scheduled_workout({workout_id}, '{schedule_date}') - {workout_name}",
+ method_name="schedule_workout",
+ api_call_desc=f"api.schedule_workout({workout_id}, '{schedule_date}') - {workout_name}",
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| call_and_display( | |
| api.schedule_workout, | |
| workout_id, | |
| schedule_date, | |
| method_name="scheduled_workout", | |
| api_call_desc=f"api.scheduled_workout({workout_id}, '{schedule_date}') - {workout_name}", | |
| ) | |
| call_and_display( | |
| api.schedule_workout, | |
| workout_id, | |
| schedule_date, | |
| method_name="schedule_workout", | |
| api_call_desc=f"api.schedule_workout({workout_id}, '{schedule_date}') - {workout_name}", | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@demo.py` around lines 2462 - 2468, The display strings use the wrong method
name: update the call to call_and_display that invokes api.schedule_workout so
the method_name and api_call_desc reflect "schedule_workout" (not
"scheduled_workout"); edit the parameters in the call_and_display invocation
(method_name and api_call_desc) to reference api.schedule_workout and the
correct function name, preserving the existing workout_id, schedule_date, and
workout_name arguments.
Comment on lines
+364
to
+391
| def connectwebproxy(self, path: str, **kwargs: Any) -> Any: | ||
| """Wrapper for web proxy requests to connect.garmin.com with error handling.""" | ||
| try: | ||
| return self.garth.request("GET", "connect", path, **kwargs).json() | ||
| except GarthHTTPError as e: | ||
| status = getattr(getattr(e.error, "response", None), "status_code", None) | ||
| logger.exception( | ||
| "API call failed for web proxy path '%s' (status=%s)", | ||
| path, | ||
| status, | ||
| ) | ||
| if status == 401: | ||
| raise GarminConnectAuthenticationError( | ||
| f"Web proxy auth error: {e}" | ||
| ) from e | ||
| if status == 429: | ||
| raise GarminConnectTooManyRequestsError( | ||
| f"Web proxy rate limit: {e}" | ||
| ) from e | ||
| if status and 400 <= status < 500: | ||
| raise GarminConnectConnectionError( | ||
| f"Web proxy client error ({status}): {e}" | ||
| ) from e | ||
| raise GarminConnectConnectionError(f"Web proxy error: {e}") from e | ||
| except Exception as e: | ||
| logger.exception("Connection error during web proxy path=%s", path) | ||
| raise GarminConnectConnectionError(f"Connection error: {e}") from e | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent error handling compared to connectapi.
The connectwebproxy method only catches GarthHTTPError, while connectapi (lines 334-359) catches both HTTPError and GarthHTTPError, and also handles AssertionError for OAuth token refresh issues. If garth.request() can raise these other exception types, this could lead to inconsistent behavior or unhandled exceptions.
Consider aligning the exception handling:
♻️ Proposed fix to align error handling
def connectwebproxy(self, path: str, **kwargs: Any) -> Any:
"""Wrapper for web proxy requests to connect.garmin.com with error handling."""
try:
return self.garth.request("GET", "connect", path, **kwargs).json()
+ except AssertionError as e:
+ error_msg = str(e).lower()
+ if "oauth" in error_msg and (
+ "oauth1" in error_msg or "oauth2" in error_msg
+ ):
+ logger.exception("OAuth token refresh failed during web proxy call.")
+ raise GarminConnectAuthenticationError(
+ f"Token refresh failed. Please re-authenticate. Original error: {e}"
+ ) from e
+ raise
- except GarthHTTPError as e:
+ except (HTTPError, GarthHTTPError) as e:
- status = getattr(getattr(e.error, "response", None), "status_code", None)
+ if isinstance(e, GarthHTTPError):
+ status = getattr(
+ getattr(e.error, "response", None), "status_code", None
+ )
+ else:
+ status = getattr(getattr(e, "response", None), "status_code", None)
logger.exception(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@garminconnect/__init__.py` around lines 364 - 391, The connectwebproxy method
currently only catches GarthHTTPError, causing inconsistent handling versus
connectapi; update connectwebproxy to mirror connectapi by also catching
HTTPError and AssertionError from garth.request and handling them the same way
(inspect response.status_code, log via logger.exception, raise
GarminConnectAuthenticationError on 401, GarminConnectTooManyRequestsError on
429, GarminConnectConnectionError for other 4xx and general errors), and ensure
AssertionError raised during OAuth refresh is converted to
GarminConnectAuthenticationError as connectapi does.
Comment on lines
+1923
to
+2018
| def import_activity(self, activity_path: str) -> dict[str, Any]: | ||
| """Upload activity as an import (not re-exported to third parties like Strava). | ||
|
|
||
| Uses the Garmin import endpoint with headers matching Garmin Connect | ||
| Mobile, so imported activities are treated as imports rather than | ||
| device-synced activities. | ||
|
|
||
| Args: | ||
| activity_path: Path to the activity file (FIT, TCX, or GPX). | ||
|
|
||
| Returns: | ||
| Dictionary containing the DetailedImportResult with successes, | ||
| failures, and activity IDs. | ||
|
|
||
| Raises: | ||
| FileNotFoundError: If the activity file does not exist. | ||
| GarminConnectInvalidFileFormatError: If the file format is invalid. | ||
| GarminConnectConnectionError: If the upload fails. | ||
|
|
||
| """ | ||
| if not activity_path: | ||
| raise ValueError("activity_path cannot be empty") | ||
|
|
||
| if not isinstance(activity_path, str): | ||
| raise ValueError("activity_path must be a string") | ||
|
|
||
| p = Path(activity_path) | ||
| if not p.exists(): | ||
| raise FileNotFoundError(f"File not found: {activity_path}") | ||
|
|
||
| if not p.is_file(): | ||
| raise ValueError(f"path is not a file: {activity_path}") | ||
|
|
||
| file_base_name = p.name | ||
| if not file_base_name: | ||
| raise ValueError("invalid file path - no filename found") | ||
|
|
||
| file_parts = file_base_name.split(".") | ||
| if len(file_parts) < 2: | ||
| raise GarminConnectInvalidFileFormatError( | ||
| f"File has no extension: {activity_path}" | ||
| ) | ||
|
|
||
| file_extension = file_parts[-1].lower() | ||
| if file_extension.upper() not in Garmin.ActivityUploadFormat.__members__: | ||
| allowed_formats = ", ".join(Garmin.ActivityUploadFormat.__members__.keys()) | ||
| raise GarminConnectInvalidFileFormatError( | ||
| f"Invalid file format '{file_extension}'. " | ||
| f"Allowed formats: {allowed_formats}" | ||
| ) | ||
|
|
||
| url = f"{self.garmin_connect_upload}/{file_extension}" | ||
| headers = { | ||
| "NK": "NT", | ||
| "origin": "https://sso.garmin.com", | ||
| "User-Agent": "GCM-iOS-5.7.2.1", | ||
| } | ||
|
|
||
| try: | ||
| with p.open("rb") as file_handle: | ||
| files = { | ||
| "file": ( | ||
| f'"{file_base_name}"', | ||
| file_handle, | ||
| "application/octet-stream", | ||
| ) | ||
| } | ||
| logger.debug("Importing activity file %s via %s", file_base_name, url) | ||
| response = self.garth.post( | ||
| "connectapi", url, files=files, headers=headers, api=True | ||
| ) | ||
| if hasattr(response, "json"): | ||
| result: dict[str, Any] = response.json() | ||
| return result | ||
| return {"status": "uploaded", "fileName": file_base_name} | ||
| except (HTTPError, GarthHTTPError) as e: | ||
| if isinstance(e, GarthHTTPError): | ||
| status = getattr( | ||
| getattr(e.error, "response", None), "status_code", None | ||
| ) | ||
| else: | ||
| status = getattr(getattr(e, "response", None), "status_code", None) | ||
| if status == 409: | ||
| logger.info("Activity already exists (duplicate): %s", file_base_name) | ||
| raise GarminConnectConnectionError( | ||
| f"Activity already exists (duplicate): {file_base_name}" | ||
| ) from e | ||
| logger.exception( | ||
| "Import failed for '%s' (status=%s)", activity_path, status | ||
| ) | ||
| raise GarminConnectConnectionError(f"Import error: {e}") from e | ||
| except OSError as e: | ||
| raise GarminConnectConnectionError( | ||
| f"Failed to read file {activity_path}: {e}" | ||
| ) from e | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting shared file validation logic with upload_activity.
Lines 1943-1972 duplicate the file validation logic from upload_activity (lines 1873-1921). Extracting this into a private helper would reduce duplication and ensure consistent validation.
Also, the filename wrapping on line 1985 (f'"{file_base_name}"') embeds literal quotes in the filename. If this is intentional to match Garmin Connect Mobile's multipart form encoding, a brief comment explaining this would help future maintainers.
♻️ Proposed helper for file validation
def _validate_activity_file(self, activity_path: str) -> tuple[Path, str]:
"""Validate activity file path and return (Path, extension).
Raises:
ValueError: If path is invalid.
FileNotFoundError: If file doesn't exist.
GarminConnectInvalidFileFormatError: If format is unsupported.
"""
if not activity_path:
raise ValueError("activity_path cannot be empty")
if not isinstance(activity_path, str):
raise ValueError("activity_path must be a string")
p = Path(activity_path)
if not p.exists():
raise FileNotFoundError(f"File not found: {activity_path}")
if not p.is_file():
raise ValueError(f"path is not a file: {activity_path}")
file_base_name = p.name
if not file_base_name:
raise ValueError("invalid file path - no filename found")
file_parts = file_base_name.split(".")
if len(file_parts) < 2:
raise GarminConnectInvalidFileFormatError(
f"File has no extension: {activity_path}"
)
file_extension = file_parts[-1]
if file_extension.upper() not in Garmin.ActivityUploadFormat.__members__:
allowed = ", ".join(Garmin.ActivityUploadFormat.__members__.keys())
raise GarminConnectInvalidFileFormatError(
f"Invalid file format '{file_extension}'. Allowed formats: {allowed}"
)
return p, file_extension🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@garminconnect/__init__.py` around lines 1923 - 2018, The file validation
block duplicated in import_activity (validate file existence, is_file, filename,
extension and allowed formats) should be extracted into a private helper (e.g.,
_validate_activity_file) that returns the Path and normalized extension and is
reused by both upload_activity and import_activity; update import_activity to
call _validate_activity_file (replace checks from the block around
file_base_name/file_extension) and keep identical error types/messages;
additionally, add a short inline comment next to the multipart filename
construction in import_activity (the line building the files dict with
f'"{file_base_name}"') clarifying that the embedded quotes are intentional to
mimic Garmin Connect Mobile multipart encoding so future maintainers understand
why the filename is wrapped in quotes.
README.md
Outdated
Comment on lines
+385
to
+388
|
|
||
| ### Additional Resources | ||
| - **Simple Example**: [example.py](https://raw.githubusercontent.com/cyberjunky/python-garminconnect/master/example.py) - Getting started guide | ||
| - **Comprehensive Demo**: [demo.py](https://raw.githubusercontent.com/cyberjunky/python-garminconnect/master/demo.py) - All 105+ API methods | ||
| - **Comprehensive Demo**: [demo.py](https://raw.githubusercontent.com/cyberjunky/python-garminconnect/master/demo.py) - All 126+ API methods |
Contributor
There was a problem hiding this comment.
Add blank line before heading.
Per markdownlint (MD022), headings should be surrounded by blank lines for proper rendering and consistency.
📝 Proposed fix
**Helper functions:** `create_warmup_step`, `create_interval_step`, `create_recovery_step`, `create_cooldown_step`, `create_repeat_group`
+
### Additional Resources🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 386-386: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 385 - 388, Add a blank line immediately before the
"### Additional Resources" heading in README.md so the heading is separated from
the previous paragraph (ensure the line containing "### Additional Resources" is
preceded by an empty line); this satisfies markdownlint MD022 and ensures proper
rendering of that section.
Author
|
Thanks, great catch. What’s happening
Verified behavior
Demo alignmentI updated So the fix is needed: it preserves 0-based library semantics while satisfying Garmin’s current backend contract. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demo.py`:
- Around line 1421-1429: Add a cancel option to the interactive file selection
loop by accepting 'q' (or 'Q') as input and handling it similarly to other
interactive functions like schedule_workout_data; inside the loop that reads
input into choice and uses activity_files, first read the raw input string, if
it equals 'q' set selected_file to None (or return/cancel from the surrounding
function) and break, otherwise attempt to parse it to int and proceed with the
existing validation (1 <= choice <= len(activity_files)) and messages for
invalid input; ensure any callers that expect selected_file handle the
None/cancel case consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+1421
to
+1429
| while True: | ||
| try: | ||
| choice = int(input(f"Enter number (1-{len(activity_files)}): ")) | ||
| if 1 <= choice <= len(activity_files): | ||
| selected_file = activity_files[choice - 1] | ||
| break | ||
| print("Invalid selection. Try again.") | ||
| except ValueError: | ||
| print("Please enter a valid number.") |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a cancel option for file selection.
The interactive file selection loop doesn't provide a way to cancel. Other similar interactive functions in this file (e.g., schedule_workout_data at line 2444) include a 'q' option to cancel. For consistency:
♻️ Suggested improvement
while True:
try:
- choice = int(input(f"Enter number (1-{len(activity_files)}): "))
+ choice_input = input(f"Enter number (1-{len(activity_files)}, or 'q' to cancel): ").strip()
+ if choice_input.lower() == "q":
+ print("❌ Import cancelled")
+ return
+ choice = int(choice_input)
if 1 <= choice <= len(activity_files):
selected_file = activity_files[choice - 1]
break
print("Invalid selection. Try again.")
except ValueError:
print("Please enter a valid number.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@demo.py` around lines 1421 - 1429, Add a cancel option to the interactive
file selection loop by accepting 'q' (or 'Q') as input and handling it similarly
to other interactive functions like schedule_workout_data; inside the loop that
reads input into choice and uses activity_files, first read the raw input
string, if it equals 'q' set selected_file to None (or return/cancel from the
surrounding function) and break, otherwise attempt to parse it to int and
proceed with the existing validation (1 <= choice <= len(activity_files)) and
messages for invalid input; ensure any callers that expect selected_file handle
the None/cancel case consistently.
Author
|
@cyberjunky small change of heart: rather than simply incrementing the start, I added a better of handling of the |
…tart) Made-with: Cursor
…all badge challenge endpoints
sebmill
force-pushed
the
fix-badge-challenge-400
branch
from
13dd17b to
53b2ad8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
demo.py (3)
2462-2468:⚠️ Potential issue | 🟡 MinorUse the actual method name in schedule-workout display metadata.
The call invokes
api.schedule_workout, but the display labels still sayscheduled_workout.🔧 Proposed fix
call_and_display( api.schedule_workout, workout_id, schedule_date, - method_name="scheduled_workout", - api_call_desc=f"api.scheduled_workout({workout_id}, '{schedule_date}') - {workout_name}", + method_name="schedule_workout", + api_call_desc=f"api.schedule_workout({workout_id}, '{schedule_date}') - {workout_name}", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo.py` around lines 2462 - 2468, The display metadata is using the wrong method name "scheduled_workout" while calling api.schedule_workout; update the call_and_display invocation (call_and_display) to use method_name="schedule_workout" and change the api_call_desc string to reflect api.schedule_workout (e.g., replace "api.scheduled_workout" with "api.schedule_workout") while keeping the existing workout_id, schedule_date, and workout_name variables so the label matches the actual api.schedule_workout call.
91-91:⚠️ Potential issue | 🟠 MajorDemo badge pagination still uses 1-based start, contradicting public API semantics.
Using
start_badge = 1in demo calls bypasses the library’s intended 0-based contract and masks pagination behavior.🔧 Proposed fix
- self.start_badge = 1 # Badge related calls in demo default to 1 (backend page index) + self.start_badge = 0 # Public API is 0-based; library converts to backend 1-based internallyAlso applies to: 3893-3899, 3924-3937
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo.py` at line 91, The demo sets self.start_badge = 1 which contradicts the library’s 0-based pagination; change the default to 0 and update all demo badge-related calls that assume a 1-based page index (search for self.start_badge, any usages in methods that build badge pagination requests, and the other noted blocks around lines ~3893-3899 and ~3924-3937) so they pass 0-based indices to the public API; ensure any user-facing comments and tests in those demo sections are adjusted to reflect 0-based semantics.
1421-1429:⚠️ Potential issue | 🟡 MinorAdd a cancel path in import file selection.
This interactive loop still lacks the standard
'q'cancel behavior used elsewhere in the demo.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo.py` around lines 1421 - 1429, The selection loop reading user input for activity_files should accept a 'q' cancel path like other demo prompts: before converting input to int, check if the raw input equals 'q' (or 'Q'), set selected_file to None (or otherwise signal cancellation to the caller) and break/return to exit the import flow; otherwise proceed with the existing int conversion/validation logic and keep the ValueError handling. Update the while True block that uses variables choice, activity_files, and selected_file to implement this early 'q' check and ensure downstream code handles selected_file being None.garminconnect/__init__.py (2)
1927-2022: 🧹 Nitpick | 🔵 Trivial
import_activitystill duplicates upload-file validation logic.The same validation path exists in
upload_activity, which increases drift risk. Also, the quoted multipart filename should be documented inline as intentional.As per coding guidelines, "use type hints consistently".
364-390:⚠️ Potential issue | 🟠 Major
connectwebproxyerror handling still diverges fromconnectapi.This reintroduces inconsistent behavior for OAuth refresh/assertion and HTTP exception mapping on web-proxy calls.
#!/bin/bash set -euo pipefail echo "== connectapi exception branches ==" rg -n -A35 -B5 'def connectapi\(' garminconnect/__init__.py echo echo "== connectwebproxy exception branches ==" rg -n -A35 -B5 'def connectwebproxy\(' garminconnect/__init__.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garminconnect/__init__.py` around lines 364 - 390, connectwebproxy's exception handling must be made identical to connectapi: replace the current GarthHTTPError and generic Exception branches in connectwebproxy with the same HTTP-status-to-exception mapping and OAuth refresh/assertion handling used by connectapi (i.e., inspect getattr(e.error, "response", None).status_code and raise GarminConnectAuthenticationError, GarminConnectTooManyRequestsError, GarminConnectConnectionError, and any OAuth refresh/assertion-related exceptions exactly as connectapi does). Either call or extract the shared handler used by connectapi (or duplicate its branches) so both connectapi and connectwebproxy use the same logic; keep logger.exception calls for context but ensure the same messages/exception types and chaining (from e) are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@garminconnect/__init__.py`:
- Around line 1418-1419: The pagination logic incorrectly maps 0-based pages to
backend pages by only special-casing start==0; replace the conditional
assignment (backend_start = str(1 if start == 0 else start)) with a proper
1-based conversion such as backend_start = str(int(start) + 1) so every 0-based
start maps to backend page start+1, and apply the same change to the other
occurrences that use backend_start/params (the similar assignments at the other
locations referenced in the review).
---
Duplicate comments:
In `@demo.py`:
- Around line 2462-2468: The display metadata is using the wrong method name
"scheduled_workout" while calling api.schedule_workout; update the
call_and_display invocation (call_and_display) to use
method_name="schedule_workout" and change the api_call_desc string to reflect
api.schedule_workout (e.g., replace "api.scheduled_workout" with
"api.schedule_workout") while keeping the existing workout_id, schedule_date,
and workout_name variables so the label matches the actual api.schedule_workout
call.
- Line 91: The demo sets self.start_badge = 1 which contradicts the library’s
0-based pagination; change the default to 0 and update all demo badge-related
calls that assume a 1-based page index (search for self.start_badge, any usages
in methods that build badge pagination requests, and the other noted blocks
around lines ~3893-3899 and ~3924-3937) so they pass 0-based indices to the
public API; ensure any user-facing comments and tests in those demo sections are
adjusted to reflect 0-based semantics.
- Around line 1421-1429: The selection loop reading user input for
activity_files should accept a 'q' cancel path like other demo prompts: before
converting input to int, check if the raw input equals 'q' (or 'Q'), set
selected_file to None (or otherwise signal cancellation to the caller) and
break/return to exit the import flow; otherwise proceed with the existing int
conversion/validation logic and keep the ValueError handling. Update the while
True block that uses variables choice, activity_files, and selected_file to
implement this early 'q' check and ensure downstream code handles selected_file
being None.
In `@garminconnect/__init__.py`:
- Around line 364-390: connectwebproxy's exception handling must be made
identical to connectapi: replace the current GarthHTTPError and generic
Exception branches in connectwebproxy with the same HTTP-status-to-exception
mapping and OAuth refresh/assertion handling used by connectapi (i.e., inspect
getattr(e.error, "response", None).status_code and raise
GarminConnectAuthenticationError, GarminConnectTooManyRequestsError,
GarminConnectConnectionError, and any OAuth refresh/assertion-related exceptions
exactly as connectapi does). Either call or extract the shared handler used by
connectapi (or duplicate its branches) so both connectapi and connectwebproxy
use the same logic; keep logger.exception calls for context but ensure the same
messages/exception types and chaining (from e) are used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bfeaf91d-1308-445c-9ef2-150abf90ea9a
📒 Files selected for processing (2)
demo.pygarminconnect/__init__.py
Comment on lines
+1418
to
+1419
| backend_start = str(1 if start == 0 else start) | ||
| params = {"desc": "true", "start": backend_start, "limit": str(limit)} |
Contributor
There was a problem hiding this comment.
Incorrect 0-based to 1-based conversion for badge pagination.
backend_start = str(1 if start == 0 else start) only fixes page 0. For page 1+, this sends the wrong backend page and returns shifted/duplicate results.
🔧 Proposed fix
+ `@staticmethod`
+ def _badge_challenge_params(start: int, limit: int) -> dict[str, str]:
+ """Build backend params for badge-challenge endpoints.
+
+ Public API uses 0-based start; backend requires 1-based.
+ """
+ return {"desc": "true", "start": str(start + 1), "limit": str(limit)}
+
def get_badge_challenges(self, start: int, limit: int) -> dict[str, Any]:
@@
- backend_start = str(1 if start == 0 else start)
- params = {"desc": "true", "start": backend_start, "limit": str(limit)}
+ params = self._badge_challenge_params(start, limit)
@@
def get_available_badge_challenges(self, start: int, limit: int) -> dict[str, Any]:
@@
- backend_start = str(1 if start == 0 else start)
- params = {"desc": "true", "start": backend_start, "limit": str(limit)}
+ params = self._badge_challenge_params(start, limit)
@@
def get_non_completed_badge_challenges(
self, start: int, limit: int
) -> dict[str, Any]:
@@
- backend_start = str(1 if start == 0 else start)
- params = {"desc": "true", "start": backend_start, "limit": str(limit)}
+ params = self._badge_challenge_params(start, limit)
@@
def get_inprogress_virtual_challenges(
self, start: int, limit: int
) -> dict[str, Any]:
@@
- backend_start = str(1 if start == 0 else start)
- params = {"desc": "true", "start": backend_start, "limit": str(limit)}
+ params = self._badge_challenge_params(start, limit)Also applies to: 1429-1430, 1442-1443, 1455-1456
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@garminconnect/__init__.py` around lines 1418 - 1419, The pagination logic
incorrectly maps 0-based pages to backend pages by only special-casing start==0;
replace the conditional assignment (backend_start = str(1 if start == 0 else
start)) with a proper 1-based conversion such as backend_start = str(int(start)
+ 1) so every 0-based start maps to backend page start+1, and apply the same
change to the other occurrences that use backend_start/params (the similar
assignments at the other locations referenced in the review).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
get_badge_challenges,get_available_badge_challenges,get_non_completed_badge_challenges, andget_inprogress_virtual_challengeswere returning 400 Bad Request when called with the existing parameters (start=0,limit=50, nodesc).Cause
Garmin's API now expects:
desc=true(required).startfor pagination (the first page isstart=1, notstart=0). Confirmed by comparing with live requests from the Garmin Connect web UI (DevTools → Network).Change
desc=true,start=str(start + 1),limit=str(limit).start(e.g.start=0= first page); the conversion to 1-based is done only when calling the backend.Testing
Summary by CodeRabbit