Issue 178 healy: Upsert user email to database #336
Conversation
… do it for create_user_request
i want to push a branch with the least diffs from main as possible
…allation and create_user_request, added on_conflict to create_user, and created variables for user_name and unique_issue_id in test_complete_and_update_usage_record_only_updates_one_record
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @BigOleHealz - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider the privacy implications of storing user emails. It may be worth implementing an opt-in mechanism for email storage, even for public emails.
- Enhance error handling throughout the new code, particularly in network requests and API interactions. This will improve the robustness of the feature.
- Consider a general code review for optimization opportunities, especially in database operations and function structure. This could improve performance and maintainability.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -47,12 +48,14 @@ def test_create_and_update_user_request_works() -> None: | |||
| owner_id=OWNER_ID, | |||
| user_id=USER_ID, | |||
| user_name=USER_NAME, | |||
| email=EMAIL, | |||
There was a problem hiding this comment.
suggestion (testing): Insufficient test coverage for email handling in create_user
The create_user function now includes an email parameter, but the test doesn't verify if the email is correctly stored in the users table. Consider adding assertions to check this new functionality.
email=EMAIL,
# Assert email is correctly stored
assert created_user["email"] == EMAIL
There was a problem hiding this comment.
@BigOleHealz Can you do this?
There was a problem hiding this comment.
I need to talk with you about this. I understand the idea, but there is no created_user variable in this function, file, or in the entire repo
There was a problem hiding this comment.
I need to talk with you about this one... i'm not sure exactly what its asking. I understand the idea, there is no created_user variable
services/supabase/gitauto_manager.py
Outdated
| if not str(email).lower().endswith("@users.noreply.github.com"): | ||
| response: APIResponse = self.client.table("users").select("email").eq("user_id", user_id).execute() | ||
|
|
||
| if response.data and len(response.data[0]) > 0: | ||
| if email != response.data[0].get('email'): | ||
| self.client.table("users").update( | ||
| json={"email": email} | ||
| ).eq("user_id", user_id).execute() |
There was a problem hiding this comment.
issue (code-quality): Remove unnecessary casts to int, str, float or bool (remove-unnecessary-cast)
There was a problem hiding this comment.
it isnt unnecessary. if email is None, casting it to lowercase will through an error
There was a problem hiding this comment.
Not good because if an error happen, users can not get the output.
There was a problem hiding this comment.
ok so do you want me to remove the cast to string or surround it with a try-catch block?
There was a problem hiding this comment.
ok let's talk about this. it's using the @handle_exceptions(default_return_value=None, raise_on_error=True) decorator
hiroshinishio
changed the title
| @@ -47,12 +48,14 @@ def test_create_and_update_user_request_works() -> None: | |||
| owner_id=OWNER_ID, | |||
| user_id=USER_ID, | |||
| user_name=USER_NAME, | |||
| email=EMAIL, | |||
There was a problem hiding this comment.
@BigOleHealz Can you do this?
| @@ -14,6 +14,7 @@ | |||
| INSTALLATION_ID, | |||
| UNIQUE_ISSUE_ID, | |||
| NEW_INSTALLATION_ID, | |||
| EMAIL, | |||
There was a problem hiding this comment.
I think EMAIL should cover all patterns we discussed?
There was a problem hiding this comment.
I'm not sure what you mean. you want me to create and test multiple emails?
What does this PR do?
Makes API request to github API based on username to get email. If email is received, it upserts to users table on installation and updates on create_user_request
PR checklist
Test Cases
Here is the list of test cases:
Evidence
Here is the list of Loom video URL(s):
Environment Variables Setting
Here is the list of configurations and screenshots where we have changes other than code:
Summary by Sourcery
Fetch user email from GitHub API and update the users table in the database during installation and user request creation. Add tests to ensure correct email handling.
New Features:
Enhancements:
Tests: