Skip to content

Conversation

@NoahMLoomis
Copy link

@NoahMLoomis NoahMLoomis commented Oct 2, 2025

This should fix #286 , since some usernames don't have the '@'.

A future improvement for readability and to avoid confusion could be to replace all instances of "email" mentioned to "username" since it seems to be possible for users to login without an email

Summary by CodeRabbit

  • Bug Fixes
    • Relaxed username validation during login so non-email usernames no longer trigger “invalid credentials”; MFA/login proceeds normally.
    • Improved weigh-in submission handling to avoid errors when the server returns no content, making weight entries more reliable.
    • General login and weigh-in reliability improvements across regions and account types.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 2, 2025

Warning

.coderabbit.yaml has a parsing error

The CodeRabbit configuration file in this repository has a parsing error and default settings were used instead. Please fix the error(s) in the configuration file. You can initialize chat with CodeRabbit to get help with the configuration file.

💥 Parsing errors (1)
Validation error: Invalid enum value. Expected 'chill' | 'assertive', received 'pythonic' at "reviews.profile"
⚙️ Configuration instructions
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Walkthrough

Added a helper to handle 204 No Content JSON responses, removed username '@' format guard in login for non-CN users so login proceeds directly to MFA/login flow when credentials provided, and updated weigh-in methods to allow None returns when responses have no content. No public API signature removals.

Changes

Cohort / File(s) Summary
Core client updates
garminconnect/__init__.py
Added `_validate_json_exists(response: requests.Response) -> dict[str, Any]

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant GarminClient as Garmin Client
  participant Auth as Auth Service

  rect rgb(240,255,240)
  note over GarminClient: Login flow (after change)
  User->>GarminClient: login(username, password)
  GarminClient->>Auth: Initiate login / MFA flow
  Auth-->>GarminClient: Auth result
  GarminClient-->>User: Tokens / Error
  end
Loading
sequenceDiagram
  autonumber
  actor User
  participant GarminClient as Garmin Client
  participant API as Garmin API

  rect rgb(245,245,255)
  note over GarminClient: Weigh‑in POST handling (after change)
  User->>GarminClient: add_weigh_in(...)
  GarminClient->>API: POST /user/weight
  API-->>GarminClient: 200/201 with JSON or 204 No Content
  alt JSON body present
    GarminClient->>GarminClient: _validate_json_exists -> returns JSON dict
    GarminClient-->>User: dict
  else 204 No Content
    GarminClient->>GarminClient: _validate_json_exists -> returns None
    GarminClient-->>User: None
  end
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I hop in code with nimble paws,
Removed the fence of "@" because—
Now logins skip that picky gate,
And weigh-ins handle empty state.
Thump-thump—small fixes, boundless cheers! 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Out of Scope Changes Check ⚠️ Warning The pull request introduces a new _validate_json_exists helper and updates weigh-in methods, which are unrelated to the removal of the '@' validation for login specified in issue #286. These additions concern response handling and data submission endpoints that the linked issue does not cover. Including them in this PR mixes distinct concerns and may complicate review. Please extract the helper addition and weigh-in updates into a separate pull request and keep this PR focused solely on removing the '@' username validation.
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title succinctly and accurately describes the primary change to remove the '@' symbol validation in the username check, directly reflecting the main fix for issue #286. It clearly references the specific validation being removed and is concise without extraneous detail. Thus it effectively communicates the intent of the PR to team members scanning history.
Linked Issues Check ✅ Passed The changes remove the '@' symbol validation from the login function as specified in issue #286. The login flow now proceeds without raising an exception for usernames lacking an '@' symbol, fulfilling the user requirement. All relevant login logic changes align with the linked issue objective.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c11bd0e and c4075f6.

📒 Files selected for processing (1)
  • garminconnect/__init__.py (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • garminconnect/init.py

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Exception "Email must contain '@' symbol"

1 participant