Update email.py #5936
Update email.py #5936
Conversation
WalkthroughThe changes involve significant refactoring of the authentication logic in the Changes
Poem
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: 3
🧹 Outside diff range and nitpick comments (1)
apiserver/plane/authentication/views/app/email.py (1)
22-22: Remove trailing whitespace to adhere to code style guidelines.Static analysis tools have detected trailing whitespace on lines 22, 28, and 132. Removing these trailing spaces will improve code cleanliness and maintain consistency.
Also applies to: 28-28, 132-132
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[notice] 22-22: apiserver/plane/authentication/views/app/email.py#L22
Trailing whitespace
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apiserver/plane/authentication/views/app/email.py(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Codacy Static Code Analysis
apiserver/plane/authentication/views/app/email.py
[notice] 22-22: apiserver/plane/authentication/views/app/email.py#L22
Trailing whitespace
[notice] 28-28: apiserver/plane/authentication/views/app/email.py#L28
Trailing whitespace
[notice] 132-132: apiserver/plane/authentication/views/app/email.py#L132
Trailing whitespace
🔇 Additional comments (2)
apiserver/plane/authentication/views/app/email.py (2)
17-27: General refactoring improves code reuse and maintainability.
The introduction of the redirect_with_error method in the AuthEndpoint base class effectively consolidates error handling logic. This refactoring reduces code duplication and enhances maintainability.
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[notice] 22-22: apiserver/plane/authentication/views/app/email.py#L22
Trailing whitespace
71-71:
Fix the typo in the error code key "USER_DOES_NOT_EXSIT".
The error code key "USER_DOES_NOT_EXSIT" contains a typo. It should be "USER_DOES_NOT_EXIST" to match the error message and the defined error codes.
Apply this diff to correct the typo:
- AUTHENTICATION_ERROR_CODES["USER_DOES_NOT_EXSIT"],
+ AUTHENTICATION_ERROR_CODES["USER_DOES_NOT_EXIST"],Likely invalid or redundant comment.
| existing_user = User.objects.filter(email=email).first() | ||
|
|
||
| if not existing_user: | ||
| exc = AuthenticationException( | ||
| error_code=AUTHENTICATION_ERROR_CODES["USER_DOES_NOT_EXIST"], | ||
| error_message="USER_DOES_NOT_EXIST", | ||
| payload={"email": str(email)}, | ||
| ) | ||
| params = exc.get_error_dict() | ||
| if next_path: | ||
| params["next_path"] = str(next_path) | ||
| url = urljoin( | ||
| base_host(request=request, is_app=True), | ||
| "sign-in?" + urlencode(params), | ||
| ) | ||
| return HttpResponseRedirect(url) | ||
| return self.redirect_with_error( | ||
| request, | ||
| AUTHENTICATION_ERROR_CODES["USER_DOES_NOT_EXSIT"], | ||
| "USER_DOES_NOT_EXIST", | ||
| next_path, | ||
| {"email": str(email)} | ||
|
|
||
| ) | ||
|
|
||
| try: | ||
| provider = EmailProvider( | ||
| request=request, | ||
| key=email, | ||
| code=password, | ||
| is_signup=False, | ||
| callback=post_user_auth_workflow, | ||
|
|
||
| ) | ||
|
|
||
| user = provider.authenticate() | ||
| # Login the user and record his device info | ||
| user_login(request=request, user=user, is_app=True) | ||
| # Get the redirection path | ||
| if next_path: | ||
| path = str(next_path) | ||
| else: | ||
| path = get_redirection_path(user=user) | ||
|
|
||
| # redirect to referer path | ||
| path = next_path if next_path else get_redirection_path(user=user) | ||
| url = urljoin(base_host(request=request, is_app=True), path) | ||
| return HttpResponseRedirect(url) | ||
| except AuthenticationException as e: | ||
| params = e.get_error_dict() | ||
| if next_path: | ||
| params["next_path"] = str(next_path) | ||
| url = urljoin( | ||
| base_host(request=request, is_app=True), | ||
| "sign-in?" + urlencode(params), | ||
| ) | ||
| return HttpResponseRedirect(url) | ||
|
|
||
|
|
||
| class SignUpAuthEndpoint(View): | ||
| return self.redirect_with_error(request, e.error_code, e.error_message, next_path) | ||
| class SignInAuthEndpoint (AuthEndpoint): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent exception handling in handle_user_authentication.
The method handle_user_authentication effectively manages user authentication and errors. However, consider following the same exception-raising pattern as suggested for validate_email_and_password to maintain consistency across methods.
You could refactor the method to raise AuthenticationException where appropriate and handle exceptions in the calling method.
| AUTHENTICATION_ERROR_CODES["INVAILD_EMAIL_SIGN_IN"], | ||
| "INVALID_EMAIL_SIGN_IN", |
There was a problem hiding this comment.
Correct the typo in the error code key "INVAILD_EMAIL_SIGN_IN".
There's a typo in the error code key "INVAILD_EMAIL_SIGN_IN". It should be "INVALID_EMAIL_SIGN_IN" to ensure consistency and prevent potential lookup errors.
Apply this diff to fix the typo:
- AUTHENTICATION_ERROR_CODES["INVAILD_EMAIL_SIGN_IN"],
+ AUTHENTICATION_ERROR_CODES["INVALID_EMAIL_SIGN_IN"],📝 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.
| AUTHENTICATION_ERROR_CODES["INVAILD_EMAIL_SIGN_IN"], | |
| "INVALID_EMAIL_SIGN_IN", | |
| AUTHENTICATION_ERROR_CODES["INVALID_EMAIL_SIGN_IN"], | |
| "INVALID_EMAIL_SIGN_IN", |
| if not email or not password: | ||
| # Redirection params | ||
| exc = AuthenticationException( | ||
| error_code=AUTHENTICATION_ERROR_CODES[ | ||
| "REQUIRED_EMAIL_PASSWORD_SIGN_IN" | ||
| ], | ||
| error_message="REQUIRED_EMAIL_PASSWORD_SIGN_IN", | ||
| payload={"email": str(email)}, | ||
| ) | ||
| params = exc.get_error_dict() | ||
| # Next path | ||
| if next_path: | ||
| params["next_path"] = str(next_path) | ||
| url = urljoin( | ||
| base_host(request=request, is_app=True), | ||
| "sign-in?" + urlencode(params), | ||
| ) | ||
| return HttpResponseRedirect(url) | ||
|
|
||
| # Validate email | ||
| return self.redirect_with_error( | ||
| request, | ||
| AUTHENTICATION_ERROR_CODES["REQUIRED_EMAIL_PASSWORD_SIGN_IN"], | ||
| "REQUIRED_EMAIL_PASSWORD_SIGN_IN", | ||
| next_path, | ||
| {"email": str(email)}, | ||
| ) | ||
|
|
||
|
|
||
|
|
||
| email = email.strip().lower() | ||
| try: | ||
| validate_email(email) | ||
| except ValidationError: | ||
| exc = AuthenticationException( | ||
| error_code=AUTHENTICATION_ERROR_CODES["INVALID_EMAIL_SIGN_IN"], | ||
| error_message="INVALID_EMAIL_SIGN_IN", | ||
| payload={"email": str(email)}, | ||
| ) | ||
| params = exc.get_error_dict() | ||
| if next_path: | ||
| params["next_path"] = str(next_path) | ||
| url = urljoin( | ||
| base_host(request=request, is_app=True), | ||
| "sign-in?" + urlencode(params), | ||
| ) | ||
| return HttpResponseRedirect(url) | ||
|
|
||
| return self.redirect_with_error( | ||
| request, | ||
| AUTHENTICATION_ERROR_CODES["INVAILD_EMAIL_SIGN_IN"], | ||
| "INVALID_EMAIL_SIGN_IN", | ||
| next_path, | ||
| {"email": str(email)} | ||
| ) | ||
| return email | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor error handling to raise exceptions instead of returning HttpResponseRedirect.
The method validate_email_and_password currently returns either a validated email or an HttpResponseRedirect on error. Mixing return types can make the code less predictable and harder to maintain. Consider raising AuthenticationException for validation errors and handling it in the calling method. This approach aligns with standard error-handling practices and improves code clarity.
Apply this diff to modify validate_email_and_password:
def validate_email_and_password(self, email, password, next_path, request):
if not email or not password:
- return self.redirect_with_error(
- request,
- AUTHENTICATION_ERROR_CODES["REQUIRED_EMAIL_PASSWORD_SIGN_IN"],
- "REQUIRED_EMAIL_PASSWORD_SIGN_IN",
- next_path,
- {"email": str(email)},
- )
+ raise AuthenticationException(
+ error_code=AUTHENTICATION_ERROR_CODES["REQUIRED_EMAIL_PASSWORD_SIGN_IN"],
+ error_message="REQUIRED_EMAIL_PASSWORD_SIGN_IN",
+ payload={"email": str(email)},
+ )
email = email.strip().lower()
try:
validate_email(email)
except ValidationError:
- return self.redirect_with_error(
- request,
- AUTHENTICATION_ERROR_CODES["INVALID_EMAIL_SIGN_IN"],
- "INVALID_EMAIL_SIGN_IN",
- next_path,
- {"email": str(email)},
- )
+ raise AuthenticationException(
+ error_code=AUTHENTICATION_ERROR_CODES["INVALID_EMAIL_SIGN_IN"],
+ error_message="INVALID_EMAIL_SIGN_IN",
+ payload={"email": str(email)},
+ )
return emailThen, in the calling method, handle the exceptions appropriately:
def post(self, request):
next_path = request.POST.get("next_path")
redirect_response = self.check_instance_setup(request, next_path)
if redirect_response:
return redirect_response
email = request.POST.get("email", False)
password = request.POST.get("password", False)
- email = self.validate_email_and_password(email, password, next_path, request)
- if isinstance(email, HttpResponseRedirect):
- return email
+ try:
+ email = self.validate_email_and_password(email, password, next_path, request)
+ except AuthenticationException as e:
+ return self.redirect_with_error(request, e.error_code, e.error_message, next_path)
return self.handle_user_authentication(request, email, password, next_path)This change ensures that error handling is centralized and the control flow is clear.
Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
New Features
Bug Fixes