Modernize Google plugin authentication#427
Conversation
783b90a to
23da94e
Compare
kwk
left a comment
There was a problem hiding this comment.
Thank you for this PR. I have no way to test this. And I think the generated tests could be optimized. Sometimes the documentation of those tests is very poor and the code can accidentally hide real errors as I've described in my comments.
ba4a9c4 to
daa2478
Compare
Google Plugin Updates: - Replace deprecated oauth2client with google-auth libraries - Update from httplib2 to modern google.auth.transport.requests - Add proper credential file management with JSON format - Improve OAuth2 flow with better error handling - Add support for credential refresh and re-authorization - Update documentation for current Google API setup requirements Test Coverage Improvements: - Test credential file loading/saving with various formats - Test OAuth flow error scenarios and credential refresh - Test configuration validation and runtime error cases - Test event filtering and task logging functionality - Increase test coverage - All tests properly mock external dependencies Generated by: Claude 3.5 Sonnet (Anthropic AI Assistant) and Cursor Auto (AI assistant). Code modernization and comprehensive test suite generated based on: - Analysis of coverage gaps - Best practices for Google API authentication - Existing codebase patterns and testing strategies Fixes psss#415 Signed-off-by: Sandro Bonazzola <sbonazzo@redhat.com>
daa2478 to
bb502b8
Compare
kwk
left a comment
There was a problem hiding this comment.
I must say I dislike the fact that most of the Code seems to be written by AI and I have to review it. Apparently some generations are poor and not well thought through. It is hard to tell for me if you have put any thought or effort into testing this or writing pieces by hand. It looks like none of the tests were written by you. Every test has a sonnet signature. Sorry to disappoint you but I'm not going to review that.
| expiry = None | ||
| if 'token_expiry' in cred_data and cred_data['token_expiry']: | ||
| try: | ||
| # Handle different datetime formats | ||
| if cred_data['token_expiry'].endswith('Z'): | ||
| expiry = datetime.fromisoformat(cred_data['token_expiry'][:-1]) | ||
| else: | ||
| expiry = datetime.fromisoformat(cred_data['token_expiry']) | ||
| except ValueError: | ||
| # If parsing fails, let expiry remain None | ||
| pass | ||
|
|
||
| return Credentials( # type: ignore[no-untyped-call] | ||
| token=cred_data.get('access_token'), | ||
| refresh_token=cred_data.get('refresh_token'), | ||
| token_uri=cred_data.get('token_uri'), | ||
| client_id=cred_data.get('client_id'), | ||
| client_secret=cred_data.get('client_secret'), | ||
| scopes=cred_data.get('scopes'), | ||
| expiry=expiry |
There was a problem hiding this comment.
Is it fine if some of the credential data is None?
| except (FileNotFoundError, json.JSONDecodeError, KeyError): | ||
| return None |
There was a problem hiding this comment.
If some credential data indeed is None, that means you ran into a KeyError, before, right? Having said that the credential data will not be None even though it potentially can be None. Do you want that?
There was a problem hiding this comment.
The try block is a bit too big for my taste. And any error message from a poorly encoded JSON file is not shown or logged anyhwere.
|
#415 is a valid issue. Thanks for filing it. Here are the changes that I observe to be unrelated to the original #415 issue:
As a result, it's difficult for humans to review. Maybe these are good ideas, but they're clouding the main point: we want to swap in the modern libraries so we can keep |
Google Plugin Updates:
Test Coverage Improvements:
Generated by: Claude 3.5 Sonnet (Anthropic AI Assistant) Code modernization and comprehensive test suite generated based on:
Fixes #415