qrlogin: fix Export method to handle all AuthExportLoginToken response types#1571
qrlogin: fix Export method to handle all AuthExportLoginToken response types#1571
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1571 +/- ##
==========================================
+ Coverage 70.44% 70.79% +0.35%
==========================================
Files 443 443
Lines 17920 17934 +14
==========================================
+ Hits 12623 12696 +73
+ Misses 4359 4298 -61
- Partials 938 940 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…sponse types The Export method now properly handles all possible response types from AuthExportLoginToken: - AuthLoginToken: normal case, returns new token - AuthLoginTokenSuccess: token already accepted, returns error - AuthLoginTokenMigrateTo: migration needed, returns MigrationNeededError Previously only AuthLoginToken was handled, which could cause unexpected type errors.
AuthLoginTokenSuccess indicates successful authentication, not an error. Return empty token instead of error when login token was already accepted.
…d for AuthLoginTokenSuccess case
- Add tests for MigrationNeededError.Error method (0% -> 100%) - Add tests for OnLoginToken function (0% -> 100%) - Add tests for Token.Image method (0% -> 75%) - Add tests for Import method with migration scenarios (43.5% -> 82.6%) - Overall coverage improved from 67.0% to 84.6%
📝 Changes Made✅ Fixed Commit MessagesFixed all commit messages to follow Conventional Commits standard as required by commitlint:
📊 Significantly Improved Test CoverageAdded comprehensive tests to address codecov feedback: Overall coverage: Method-specific improvements:
🧪 New Tests Added
🎯 Coverage Issues AddressedThis PR addresses the codecov report showing 42.10% patch coverage with 11 missing lines. The new tests specifically target the previously uncovered code paths in:
The commitlint check should now pass ✅ and codecov coverage should be significantly improved 📈 |
…eived When Export returns empty token (AuthLoginTokenSuccess case), Auth method should wait for the loggedIn signal before calling Import, ensuring proper synchronization with the login flow and fixing test failures on macOS Go 1.23.
|
Anybody home? |
|
@tdakkota can you please take a look? |
|
|
||
| // ErrAlreadyAuthenticated indicates that user is already authenticated | ||
| // and no new QR token is needed. | ||
| var ErrAlreadyAuthenticated = errors.New("already authenticated") |
| // If empty token, it means AuthLoginTokenSuccess was returned | ||
| if t.String() == "" { | ||
| // QR was scanned and accepted, break out of loop | ||
| break | ||
| } |
There was a problem hiding this comment.
This break would break out of select, not out of the for loop.
The logic itself seems fine to me, but comment should be fixed.
| case *tg.AuthLoginTokenSuccess: | ||
| // Token was already accepted, authentication successful | ||
| // Return empty token since no new token is needed | ||
| return Token{}, nil |
There was a problem hiding this comment.
ErrAlreadyAuthenticated should be returned here, I guess.
|
Mergin this to unblock iyear/tdl#1061 |
Problem
The Export method in qrlogin package only handled *tg.AuthLoginToken response type from AuthExportLoginToken API call, but the API can return other types like *tg.AuthLoginTokenSuccess and *tg.AuthLoginTokenMigrateTo, which would cause "unexpected type" errors.
Solution
Added proper handling for all possible response types:
Changes
This fix prevents runtime panics when AuthExportLoginToken returns unexpected (but valid) response types.