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

Fixed #18119 -- Added a DomainNameValidator validator. #18037

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

nmenezes0
Copy link
Contributor

@nmenezes0 nmenezes0 commented Mar 31, 2024

Trac ticket number

ticket-18119

Branch description

Provide a concise overview of the issue or rationale behind the proposed changes.
This ticket adds a DomainNameValidator to validate International domain names. This validator takes a flag is_idna (defaults to True) - to determine whether or not we should use non-ASCII international domain names.

This builds on the existing PR with some changes: not using IP addresses (don't think they are part of a domain name), removing changes to docs as out of date (not sure what updates are needed here now?), changes to the tests and how they were written, reusing the regex in URLValidator.

The ticket in Trac suggests that the regex could be reused elsewhere in the codebase. I have reused it in the URLValidator, but I couldn't see how to use it in django/core/mail/message.py or django/utils/html.py.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • For UI changes, I have attached screenshots in both light and dark modes. - N/A

I wasn't sure what changes I should make to the documentation.

@sarahboyce
Copy link
Contributor

Thank you for this Nina ⭐

removing changes to docs as out of date (not sure what updates are needed here now?)

All new features need some docs and a release note, so this is roughly what I am expecting:

  • Add DomainNameValidator to docs/ref/validators.txt with .. versionadded:: 5.1
  • Add a reference to this change in the 5.1 release notes

@nmenezes0 nmenezes0 force-pushed the feature/18119-domain-name-validation branch from fc3bd96 to f77f5f3 Compare April 20, 2024 21:45
@nmenezes0
Copy link
Contributor Author

Thanks Sarah - docs now updated.

docs/ref/validators.txt Outdated Show resolved Hide resolved
docs/ref/validators.txt Outdated Show resolved Hide resolved
docs/ref/validators.txt Outdated Show resolved Hide resolved
docs/releases/5.1.txt Show resolved Hide resolved
@nmenezes0
Copy link
Contributor Author

Thanks @felixxm - docs now updated following comments.

Copy link
Contributor

@sarahboyce sarahboyce left a comment

Choose a reason for hiding this comment

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

Thank you @nmenezes0 ⭐ I have a few more comments.
Please also squash this to a single commit 👍

docs/ref/validators.txt Outdated Show resolved Hide resolved
docs/ref/validators.txt Outdated Show resolved Hide resolved
django/core/validators.py Outdated Show resolved Hide resolved
django/core/validators.py Outdated Show resolved Hide resolved
django/core/validators.py Outdated Show resolved Hide resolved
docs/ref/validators.txt Outdated Show resolved Hide resolved
@nmenezes0 nmenezes0 force-pushed the feature/18119-domain-name-validation branch from b2b5e90 to 151845f Compare May 11, 2024 17:53
Override __init__ function to add accept IDNA.

Updates to DomainNameValidator to add logic around accept_idna flag

Remove outdated details from docs.

Made changes to domain name validation and tests.

Reused regex in URLValidator.

Added more changes to the DomainNameValidator so all  tests pass.

Removed rogue space.

Blacken.

Update documentation for DomainNameValidator.

Remove unused text.

Tidy docs.

Update docs - fix underline.

Update docs/ref/validators.txt

Remove unnecessary blank lines.

Co-authored-by: Mariusz Felisiak <felisiak.mariusz@gmail.com>

Update docs/releases/5.1.txt

Co-authored-by: Mariusz Felisiak <felisiak.mariusz@gmail.com>

Changes to docs for DomainNameValidator.

Change name of the parameter.

Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com>

Update docs for new naming.

Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com>

Update django/core/validators.py

Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com>

Update comments for validators.

Reorder docs to have DomainNameValidator next to URLNameValidator.

blacken
@nmenezes0 nmenezes0 force-pushed the feature/18119-domain-name-validation branch from 151845f to dd9816e Compare May 11, 2024 18:04
@nmenezes0
Copy link
Contributor Author

Thanks @sarahboyce - I've made those changes and squashed to a single commit.

@nmenezes0
Copy link
Contributor Author

I'm not sure why some of the checks are failing. The error is "commit changed" - potentially due to me pushing the squashed commit?

Copy link
Contributor

@sarahboyce sarahboyce left a comment

Choose a reason for hiding this comment

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

Thank you for the updates @nmenezes0, we're really close ⭐

I'm not sure why some of the checks are failing. The error is "commit changed" - potentially due to me pushing the squashed commit?

Don't worry about this, sounds unrelated to your changes

The ticket in Trac suggests that the regex could be reused elsewhere in the codebase. I have reused it in the URLValidator, but I couldn't see how to use it in django/core/mail/message.py or django/utils/html.py.

Read through the original comment made by @claudep 12 years ago.

  • It's still valid that we could chose to use this validation in other areas of the codebase.
  • I don't think refactoring to include this is in scope of this ticket, however we should make sure it's defined in a good place.
  • I'm not sure which __init__ is being referred to in the comment and I don't get circular imports if I was to import DomainNameValidator to the mentioned files.

@claudep can you clarify where you think DomainNameValidator should be defined or whether it is correct to be in django/core/validators.py?
I think it is ok to be defined here but I might be missing something 👍

docs/ref/validators.txt Outdated Show resolved Hide resolved
docs/ref/validators.txt Outdated Show resolved Hide resolved
docs/ref/validators.txt Show resolved Hide resolved
nmenezes0 and others added 3 commits May 13, 2024 09:19
Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com>
Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com>
Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com>
@nmenezes0
Copy link
Contributor Author

Thanks @sarahboyce - have commited those docs changes - I assume I should squash into one commit again (will do that later).

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