-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix register fails on channles requests. #137 #519
Conversation
Release NotesRelease type: patchFix register fails on channels requests. #137 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #519 +/- ##
==========================================
+ Coverage 92.82% 92.93% +0.10%
==========================================
Files 33 33
Lines 1478 1501 +23
==========================================
+ Hits 1372 1395 +23
Misses 106 106 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, left some notes...
Also please add tests
gqlauth/models.py
Outdated
if isinstance(info.context, dict): | ||
request = info.context["request"] | ||
return { | ||
"user": self.user, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try to do something like
is_channels = isinstance(info.context, dict)
if is_channels:
domain, port = request.headers["host"].split(":")
else:
domain = site.domain
# yada yada
return {
"domain" domain,
# yada yada
I've been trying to write test code, but I'm not sure how to do it. I've thought of two ways to do it
|
gqlauth/models.py
Outdated
port = request.get_port() | ||
site_name = site.name | ||
domain = site.domain | ||
protocol = ("https" if request.is_secure() else "http",) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a tuple 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it wasn't. but you what you wrote is a tuple
("https" if request.is_secure() else "http",)
`
protocol = ("https" if request.is_secure() else "http",) | |
protocol = "https" if request.is_secure() else "http" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to have been automatically fixed by [pre-commit-ci[bot]. I'd like you to take a look at this bot.
You can try to use the tests consumer
BTW TBH I am not sure that you should use channels for mutation / queries... Do you have any reason to do so? |
Following your suggestion, I wrote a test code to call register mutation in channels context using verified_user_status_type and verified_channels_app_communicator. However, it seems to fail to run. The strange thing is that result.data["register"]["success"] is False, but result.errors is None when I print it out. When I ran the same code in sync context, it worked fine, but this doesn't cover the newly added code. Do you have any ideas on how to fix this? |
I'll try to run it myself later 🤞 |
The problem is that it's trying to register a new user, and the username and phone_number already exist. So I fixed it, however, it doesn't seem to cover the newly added code still. I've republished the PR, so please review it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally I accept the changes though I want to test this manually because the consumer is having problems ATM.
tests/test_register.py
Outdated
@@ -50,6 +50,17 @@ def test_register_invalid_password_validation(verified_user_status_type, anonymo | |||
assert executed["errors"] | |||
|
|||
|
|||
async def test_channels_register( | |||
verified_user_status_type, captcha, verified_channels_app_communicator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need a verified consumer here I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I replaced it with unverified_channels_app_communicator, as it doesn't require authentication when registering.
Fixed register mutation not working in channels environment. Instead of the get_current_site() function, we implemented it by getting the required values from the request.headers object and the request.consumer.scope object and returning them. There are a few things that need to be reviewed.