Fixes for return_on_mfa code #92
Conversation
|
Warning Rate limit exceeded@cyberjunky has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces changes to the Multi-Factor Authentication (MFA) handling process in the Garth library. The modifications primarily affect the login mechanism across multiple 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
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 |
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.
tests/test_http.py
Outdated
| @pytest.mark.vcr | ||
| def test_resume_login(client: Client): | ||
| client.oauth1_token, client.oauth2_token = client.login( | ||
| "[email protected]", "correct_password" | ||
| ) | ||
| assert client.oauth1_token | ||
| assert client.oauth2_token | ||
| client.oauth1_token, client.oauth2_token = None, None | ||
| assert client.oauth1_token is None | ||
| assert client.oauth2_token is None | ||
| client.resume_login("[email protected]", "correct_password") | ||
| assert client.oauth1_token | ||
| assert client.oauth2_token |
There was a problem hiding this comment.
Record VCR cassette for the new test.
The test logic is well-structured, but the pipeline is failing because the VCR cassette for test_resume_login is missing. You'll need to record the cassette first.
Run the test with VCR in record mode to generate the cassette:
#!/bin/bash
# Description: Record VCR cassette for test_resume_login
python -m pytest tests/test_http.py::test_resume_login --vcr-record=once🧰 Tools
🪛 GitHub Actions: CI
[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').
|
@felipao-mx I fixed the lint errors, not sure if I can fix the two tests. I frankly hate them, do not master them. |
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