Conversation
WalkthroughThe pull request introduces changes to the handling of Multi-Factor Authentication (MFA) in the Garth library's login process. Key modifications are made across several files, including Changes
Sequence DiagramsequenceDiagram
participant User
participant Garth
participant SSO
User->>Garth: login(email, password)
Garth->>SSO: Attempt login
alt MFA Required
SSO-->>Garth: Return ("needs_mfa", client_state)
Garth-->>User: Prompt for MFA code
User->>Garth: Provide MFA code
Garth->>SSO: Resume login with MFA
SSO-->>Garth: Return OAuth1 and OAuth2 tokens
else Login Successful
SSO-->>Garth: Return OAuth1 and OAuth2 tokens
end
Garth-->>User: Complete login
Possibly related PRs
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)`tests/**`: - test functions shouldn't have a return type hi...
🔇 Additional comments (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
README.md(1 hunks)garth/http.py(1 hunks)garth/sso.py(1 hunks)garth/version.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
garth/http.py
[warning] Code formatting issues detected. File needs to be reformatted using Ruff formatter.
[warning] Trailing whitespace detected and needs to be removed.
🔇 Additional comments (3)
garth/http.py (1)
164-169: LGTM! The login method correctly handles the new return structure.The changes properly unpack and store the OAuth tokens from the SSO login response.
🧰 Tools
🪛 GitHub Actions: CI
[warning] Code formatting issues detected. File needs to be reformatted using Ruff formatter.
[warning] Trailing whitespace detected and needs to be removed.
garth/version.py (1)
1-1: LGTM! Version bump is appropriate.The increment to 0.5.3 aligns with semantic versioning for a backward-compatible bug fix.
README.md (1)
143-144: LGTM! Documentation accurately reflects the implementation changes.The example code has been updated to demonstrate the new MFA handling flow, making it clearer and more explicit.
Also applies to: 146-146
|
@matin Hi there, can you please have a quick look and merge if you have the time? |
|
@cyberjunky I apologize I haven't had a change to review. I'm adding @felipao-mx as a maintainer to review and merge |
felipao-mx
left a comment
felipao-mx
left a comment
There was a problem hiding this comment.
hi @cyberjunky, thanks for your contribution! I have notice that some tests are not currently passing. Could you please take a look? I think these failing tests are related to the return type for "needs_mfa" case and lint checks.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/test_http.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/test_http.py (1)
Pattern tests/**: - test functions shouldn't have a return type hint
- it's ok to use
assertinstead ofpytest.assume()
🪛 GitHub Actions: CI
tests/test_http.py
[error] 254-254: CannotOverwriteExistingCassetteException: Can't overwrite existing cassette ('/home/runner/work/garth/garth/tests/cassettes/test_resume_login.yaml') in your current record mode ('none').
🔇 Additional comments (1)
tests/test_http.py (1)
116-118: LGTM! Clear and descriptive assertion message.The reformatted assertion message improves readability while maintaining clear error reporting.
|
@felipao-mx I fixed the lint errors, not sure if I can fix the two tests. I frankly hate them, do not master them. |
|
hi @cyberjunky ! thanks for fixing lint errors. I totally understand that VCR sometimes can be tricky. Would you be ok if I commit the fixes directly to your branch? I'd be happy to help. |
@felipao-mx yes please. I'm still struggling a little bit, in my limited time, how to record them with credentials. Any help is appreciated! I found out I need to call login() again after the first mfa authorisation and oauth keys were created, but that is no big deal and can be fixed later if needed. |
|
I'm afraid I'm forced to migrate parts of garths code in my library. I'm kind of stuck with this casette/tests stuff. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #92 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 34 34
Lines 1335 1356 +21
=========================================
+ Hits 1335 1356 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
v0.5.3 released! thanks @cyberjunky for your contribution |
|
@felipao-mx wow, nice, thank you very much, I can continue with coding and hopefully close several related requests! |
3 participants
With these changes I got it working, some returns where missing, I could not get it working with returning one result variable.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
resume_loginfunctionality.Chores