fix for google sign in issue #1741
Conversation
WalkthroughThe changes enhance the account deletion process by ensuring that email confirmation records are also deleted when a user account is removed. This involves modifications to the Changes
Sequence DiagramsequenceDiagram
participant User
participant AccountProcessor
participant Database
User->>AccountProcessor: Request Account Deletion
AccountProcessor->>Database: Delete User Account
AccountProcessor->>Database: Delete Email Confirmation Record
Database-->>AccountProcessor: Deletion Confirmed
AccountProcessor-->>User: Account Deleted
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
kairon/shared/account/processor.py(1 hunks)tests/integration_test/services_test.py(2 hunks)tests/unit_test/api/api_processor_test.py(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (python)
- GitHub Check: Python CI
🔇 Additional comments (13)
tests/integration_test/services_test.py (2)
31-31: LGTM!The import of
UserEmailConfirmationis properly placed and aligns with the account deletion enhancement.
29534-29534: Verify PR scope and completeness.The PR title mentions fixing a Google sign-in issue, but the changes appear to focus on account deletion functionality. Could you please clarify:
- How do these changes address the Google sign-in issue?
- Are there additional changes needed to fully resolve the sign-in problem?
kairon/shared/account/processor.py (1)
1272-1272: LGTM! Good cleanup of email confirmation records.The addition of email confirmation record deletion during account deletion ensures proper cleanup of all associated user data.
tests/unit_test/api/api_processor_test.py (10)
482-483: LGTM! Good test setup.Properly setting up email confirmation record before account setup to ensure complete test coverage.
494-494: LGTM! Good verification of cleanup.Comprehensive verification that email confirmation record is properly deleted during account deletion.
Also applies to: 499-500
515-517: LGTM! Good test setup for shared bot scenario.Properly setting up email confirmation record before shared bot test to ensure complete test coverage.
529-529: LGTM! Good verification of cleanup for shared bot.Comprehensive verification that email confirmation record is properly deleted during shared bot deletion.
Also applies to: 533-534
544-545: LGTM! Good test setup for account deletion.Properly setting up email confirmation record before account deletion test to ensure complete test coverage.
552-552: LGTM! Good verification of cleanup for account deletion.Comprehensive verification that email confirmation record is properly deleted during account deletion.
Also applies to: 557-560
570-571: LGTM! Good test setup for user deletion.Properly setting up email confirmation record before user deletion test to ensure complete test coverage.
591-591: LGTM! Good verification of cleanup for user deletion.Comprehensive verification that email confirmation record is properly deleted during user deletion.
Also applies to: 596-598
607-608: LGTM! Good test setup for account re-add scenario.Properly setting up email confirmation record before account deletion and re-add test to ensure complete test coverage.
614-614: LGTM! Good verification of cleanup for account re-add.Comprehensive verification that email confirmation record is properly deleted during account deletion and re-add.
Also applies to: 621-622
| @@ -29530,7 +29531,7 @@ def _password_reset(*args, **kwargs): | |||
| "Authorization": pytest.token_type_delete + " " + pytest.access_token_delete | |||
| }, | |||
| ).json() | |||
|
|
|||
| UserEmailConfirmation(email="integration@demo.ai").save() | |||
There was a problem hiding this comment.
Test logic needs revision for proper coverage.
The current implementation has several issues:
- The email confirmation record is created after the account deletion operation
- There's no verification that the email confirmation record is properly deleted
- The email address is hardcoded
Consider this revised approach:
+ # Create email confirmation record before deletion
+ test_email = "integration@demo.ai"
+ UserEmailConfirmation(email=test_email).save()
+
+ # Verify record exists
+ assert UserEmailConfirmation.objects(email=test_email).count() == 1
+
response = client.delete(
f"/api/account/delete/{pytest.email_delete}",
headers={
"Authorization": pytest.token_type_delete + " " + pytest.access_token_delete
},
).json()
- UserEmailConfirmation(email="integration@demo.ai").save()
assert response["success"]
assert response["message"] == "Account deleted"
assert response["error_code"] == 0
+
+ # Verify email confirmation record is deleted
+ assert UserEmailConfirmation.objects(email=test_email).count() == 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| UserEmailConfirmation(email="integration@demo.ai").save() | |
| # Create email confirmation record before deletion | |
| test_email = "integration@demo.ai" | |
| UserEmailConfirmation(email=test_email).save() | |
| # Verify record exists | |
| assert UserEmailConfirmation.objects(email=test_email).count() == 1 | |
| response = client.delete( | |
| f"/api/account/delete/{pytest.email_delete}", | |
| headers={ | |
| "Authorization": pytest.token_type_delete + " " + pytest.access_token_delete | |
| }, | |
| ).json() | |
| assert response["success"] | |
| assert response["message"] == "Account deleted" | |
| assert response["error_code"] == 0 | |
| # Verify email confirmation record is deleted | |
| assert UserEmailConfirmation.objects(email=test_email).count() == 0 |
Summary by CodeRabbit
New Features
Bug Fixes
Tests