fix for google sign in issue #1600
Conversation
WalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AccountProcessor
participant UserEmailConfirmation
User->>AccountProcessor: delete_account(account_id, email)
AccountProcessor->>UserEmailConfirmation: delete associated record
AccountProcessor-->>User: confirm deletion
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)tests/integration_test/services_test.py (2)
The import statement is properly placed and organized with related imports.
The addition of Let's check the relationship between password reset and email confirmation: 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
tests/unit_test/api/api_processor_test.py (2)
483-484: Refactor repeated initialization ofUserEmailConfirmationThe code initializing and saving
UserEmailConfirmationwith the account email is repeated across multiple test methods (test_delete_account_for_account_bots,test_delete_account_for_account,test_delete_account_for_user,test_delete_account_again_add). Consider refactoring this into a helper function to reduce code duplication and enhance maintainability.Also applies to: 546-547, 572-573, 610-611
500-501: Refactor repeated assertions forUserEmailConfirmationdeletionThe assertions checking for the non-existence of
UserEmailConfirmationafter account deletion are repeated in several tests. Refactoring these into a helper function can improve readability and maintainability.Also applies to: 534-535, 561-562, 599-600, 624-625
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
kairon/shared/account/processor.py(1 hunks)tests/unit_test/api/api_processor_test.py(9 hunks)
🔇 Additional comments (8)
tests/unit_test/api/api_processor_test.py (8)
31-32: Import UserEmailConfirmation for account deletion tests
The addition of UserEmailConfirmation to the import statement is appropriate, as it is used in subsequent tests to verify the deletion of email confirmations when an account is deleted.
500-501: Verify UserEmailConfirmation is deleted after account deletion
The test correctly asserts that the UserEmailConfirmation object no longer exists after the account is deleted, ensuring data integrity.
534-535: Ensure deletion of UserEmailConfirmation with shared account
Verifying that UserEmailConfirmation is deleted when a shared account is removed confirms consistent cleanup across different account types.
559-560: Proper exception handling when deleting a non-existent account
The test appropriately checks that attempting to delete an already deleted account raises an AppException with the message "Account does not exist!".
561-562: Confirm UserEmailConfirmation removal after account deletion
Asserting the non-existence of UserEmailConfirmation ensures that email confirmation records are properly cleaned up.
599-600: Validate UserEmailConfirmation is removed when deleting multiple users
The test verifies that UserEmailConfirmation objects for all users are deleted when the account is removed, ensuring complete data cleanup.
624-625: Check for deletion of UserEmailConfirmation after re-adding account
After recreating an account with the same email, confirming that old UserEmailConfirmation objects are not present maintains data consistency.
483-484: Handle potential None value for email
Ensure that the email retrieved from account.get('email') is not None before saving UserEmailConfirmation to prevent possible exceptions.
| UserEmailConfirmation.objects().get(email=email).delete() | ||
|
|
There was a problem hiding this comment.
Add error handling and fix indentation for UserEmailConfirmation deletion
The deletion of UserEmailConfirmation records needs improvement:
- Add error handling for cases where the record doesn't exist
- Consider moving this operation earlier in the method for better cleanup order
- Fix the indentation to match the method's scope
Apply this diff to implement the suggested changes:
- UserEmailConfirmation.objects().get(email=email).delete()
+ try:
+ UserEmailConfirmation.objects().get(email=email).delete()
+ except DoesNotExist:
+ logging.warning(f"No email confirmation record found for {email}")Also, consider moving this block right after the account validation and before bot deletion for better cleanup order.
Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
Bug Fixes
Tests