Skip to content
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

Registration feat bis : this PR will cover the registration and resetting password functionality #25

Draft
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

anjaniacatus
Copy link

No description provided.

@anjaniacatus anjaniacatus changed the title Registration feat bis Registration feat bis : this PR will cover the registration and resetting password functionality Nov 19, 2024
Copy link
Collaborator

@sheenarbw sheenarbw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good progress!

There are a few things in here that seem a little out of place. Not sure what parts of this ou re still working on.

I'm just wondering, did you consider using allauth or anything like that? It looks like there is still quite a lot of complicated code to write here.

core/urls.py Outdated

urlpatterns = [
path("", include("website.urls")),
path("accounts/register/", custom_auth_views.register, name ="register"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might get a bit verbose if we need to include all the account urls here.

There will also be a bunch of stuff around forgotten passwords I think.

Can you not include the urls in bulk?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True

username = form.cleaned_data.get('username')
email = form.cleaned_data.get('email')
######################### mail system ####################################
template_email = get_template('registration/registration_email.html')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about putting this in a seperate function. eg: send_registration_email()?

This will make the code clearer and easier to test

@@ -0,0 +1,5 @@
{% autoescape off %}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be cool to put the email templates in a different directory so they are easy to find.

Eg templates/registration/emails

dict(email=u.email),
format="text/html")
email_lines = mail.outbox[0].body.splitlines()
reset_password_url = email_lines[2].replace(" ", "")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems very odd - why is itnecessary to change the url in otder to make the url valid? Should the url not just work as is?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also true


import pytest

class UserTestCase(TestCase):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't seen people combine pytest and unittest at the beginning of a project before.

One of the main reasons people like to use pytest is because it doesn't need so much boilerplate, but this is written in a way that it does need the boilerplate.

@anjaniacatus
Copy link
Author

Good progress!

There are a few things in here that seem a little out of place. Not sure what parts of this ou re still working on.

I'm just wondering, did you consider using allauth or anything like that? It looks like there is still quite a lot of complicated code to write here.

i will check that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants