WIP: Try adding guest user library #3
Conversation
m4ra
force-pushed
the
jp-2502-guest-accounts
branch
from
c3e2c5d to
dc4a661
Compare
|
@partizipation @goapunk to enable the guest user from the registration form, I would add the maybe_make_guest_user in the a+ DefaultSingUpForm.save(). |
|
@partizipation the signup template to add the login as guest button is here. But I think the registration form is in a separate story. |
|
|
||
| GUEST_USER = { | ||
| "NAME_GENERATOR": "guest_user.functions.generate_uuid_username", # Default option | ||
| } |
There was a problem hiding this comment.
I changed this in the fork of the django-guest, because the long uuid appears on the menu tab and it's ugly. So I switched to the numbered version. But maybe we deploy this one, and PM can let us know.
| {% url 'guest_create' as guest_create_url %} | ||
| {% with next_param=request.GET.next %} | ||
| {% with guest_url=guest_create_url|add:"?next="|add:next_param %} | ||
| <p>{% blocktranslate %}Sie wollen als Gast fortfahren? Dann klicken Sie <a href="{{ guest_url }}">hier</a>.{% endblocktranslate %}</p> |
There was a problem hiding this comment.
We need the english version here.
There was a problem hiding this comment.
Asked Janek for english text yesterday, so waiting to add the content of these pages until I have that and can just do everything at once
| {% with next_param=request.GET.next %} | ||
| {% with guest_url=guest_create_url|add:"?next="|add:next_param %} | ||
| <p> | ||
| {% blocktranslate %}Sie wollen als Gast fortfahren? Dann klicken Sie |
partizipation
force-pushed
the
jp-2502-guest-accounts
branch
from
391ad52 to
2ed7317
Compare
There was a problem hiding this comment.
Overall good progress. Needs a bit of cleaning, and some last restructuring of the DefaultSingUp and Guest related forms. And three extra tests:
a) public project with a topic module can be viewed by guest user
b) upon guest user conversion we are sending email with the appropriate subject
c) upon email confirmation, guest user is deleted and its associated user has an email and is verified.
apps/users/forms.py
Outdated
| return { | ||
| "username": self.cleaned_data["username"], | ||
| "password": self.cleaned_data["password1"], | ||
| } |
There was a problem hiding this comment.
Since this is only for the GuestCreateForm, move the method get_credentials in the respective class GuestConvertForm
apps/account/views.py
Outdated
| # perform_login(self.request, user, email_verification='optional') | ||
|
|
||
| # Complete the signup process (sends confirmation email if required) | ||
| response = complete_signup(self.request, user, email_verification='mandatory', success_url="/") |
There was a problem hiding this comment.
I think you don't need to add the email_verification parameter neither the success_url here, as we are adding it in the settings/base.py. A new test in converting an account and a) checking the mailbox has 1 email and b) the subject of the email sent is the expected one, will help figuring out that part. You can add it in the testsaccount/test_views.py`. See lines 132 - 137 for how the mailbox and redirect testing is working.
Now, about the redirection, I don't think we need one here, since the conversion takes place in the dashboard of the profile, I would say we don't redirect.
There was a problem hiding this comment.
I think you don't need to add the email_verification parameter neither the success_url here
It's gives an error without these parameters, so I'm guessing they're needed since we are using this allauth function outside of the library. I've added a reference to these vars in the settings though.
the redirection, I don't think we need one here
The current behavior on signup for regular users is a redirect - in other words, you are not logged in. If for guest user convert we want to keep them logged in, we could do that but it would be more effort. I had some code in that direction earlier though, before I realized the redirect is probably desired.
| response = client.get(url) | ||
| assert response.status_code == 403 | ||
|
|
||
|
|
There was a problem hiding this comment.
would be good to have an extra test with a public project and asserting 200 as response status.
| """ | ||
| user = email_address.user | ||
| Guest.objects.filter(user=user).delete() | ||
| print("guest deleted") |
There was a problem hiding this comment.
would be great to have a test for this signal too.
apps/users/forms.py
Outdated
|
|
||
| def __init__(self, *args, **kwargs): | ||
| self.user = kwargs.pop("user", None) # Accept an existing user instance |
There was a problem hiding this comment.
we shouldn't have a user at this point.
apps/users/forms.py
Outdated
| self.fields["username"].required = True | ||
| self.fields["password1"].required = True | ||
| self.fields["password2"].required = True | ||
|
|
There was a problem hiding this comment.
Move all related guest user form attributes to the relevant class below, so we keep each form cleaner, even if there are few repetitions.
we decided to fork django-guest-user and add email and password for the guest authentication. See here
Couldn't get around the circular reference, tried a bunch of things. Code has a bunch of commented out attempts so it's a little messy but maybe this PR helps review.Tried: