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

Add emails and phone link checker #2199

Merged
merged 2 commits into from
Apr 18, 2023
Merged

Conversation

timobrembeck
Copy link
Member

Short description

Add the possibility to replace mailto: and tel: links with the link checker

Proposed changes

  • Don't ignore them by default anymore (but leave the possibility to ignore them in the future again)
  • Extend the validation to support mailto: and tel: links
    • Enforce email validation on mailto links
    • Do not enforce validation on phone links

Caveats: Only the link target is replaced, not the link text. So if this feature is used without caution, it might create inconsistencies (e.g. tel links with a different phone number target than their displayed text etc.)... Thus, I decided to only make the new categories visible for expert users.

Resolved issues

Fixes: #2198


Pull Request Review Guidelines

@timobrembeck timobrembeck requested a review from a team as a code owner April 4, 2023 13:25
@codeclimate
Copy link

codeclimate bot commented Apr 4, 2023

Code Climate has analyzed commit 46f5ed2 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 46.1% (50% is the threshold).

This pull request will bring the total coverage in the repository to 76.0% (0.0% change).

View more on Code Climate.

@seluianova
Copy link
Contributor

seluianova commented Apr 5, 2023

I haven't tested the branch thoroughly yet, but got a quick thought so far:

Thus, I decided to only make the new categories visible for expert users.

If phones/mails are broken, they are still available under "Invalid" section for non-expert users (thus can be damaged).
Also there is the same thing with URLs. If text is the same as the link, non-expert users can make an inconsistency too.
Was it considered?

@timobrembeck
Copy link
Member Author

If phones/mails are broken, they are still available under "Invalid" section for non-expert users (thus can be damaged).

True, but I'd argue that an invalid phone/mail link is important enough to appear in the invalid section, so that also non-experts are aware of the issue.

Also there is the same thing with URLs. If text is the same as the link, non-expert users can make an inconsistency too.
Was it considered?

Yes, but I think for links the link text is only identical in very rare cases, whereas for mails and phone numbers this will be the case in nearly all links (at least this is my assumption)... I'd say it's a good enough starting point, maybe the service team can provide feedback after we tested this for a while...

Copy link
Contributor

@seluianova seluianova left a comment

Choose a reason for hiding this comment

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

Looks good to me as far as I tested 👌

I would only note one thing, not sure how important:
If I try to save an invalid email, the error is "Url: Enter a valid URL"
I assume the expected one would be "Enter a valid email address"
image

@JoeyStk JoeyStk requested review from JoeyStk and removed request for JoeyStk April 11, 2023 14:25
Copy link
Contributor

@JoeyStk JoeyStk 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 🎉
As far as I tested it, it seems to work. I have two thoughts, but I'm not sure how relevant they are.

  1. Caveats: Only the link target is replaced, not the link text. I found this piece of information very important to know. So I think it definitely should be documented. If you haven't already, I also would think about maybe adding a warning about this inside the CMS, probably rather just a small note after clicking on the replace button?
  2. I noticed, that all email addresses I entered were found in the link checker, valid and invalid ones. I assumed, that this is the expected behavior, since I can just ignore the ones that are correct, but I thought it was still worth mentioning it

@timobrembeck timobrembeck force-pushed the feature/email-phone-links branch 2 times, most recently from b2b9c4c to fa9dfbb Compare April 15, 2023 10:49
@timobrembeck
Copy link
Member Author

I would only note one thing, not sure how important: If I try to save an invalid email, the error is "Url: Enter a valid URL" I assume the expected one would be "Enter a valid email address"

@seluianova Good idea, I adapted the error message in this case.

  1. Caveats: Only the link target is replaced, not the link text. I found this piece of information very important to know. So I think it definitely should be documented. If you haven't already, I also would think about maybe adding a warning about this inside the CMS, probably rather just a small note after clicking on the replace button?

@JoeyStk Yes, makes sense. I agree that it might be confusing, especially if the link text is a URL itself and not a descriptive title. I added a second field now that indicates that the link text is read-only, what do you think?

link-text-replace-disabled

  1. I noticed, that all email addresses I entered were found in the link checker, valid and invalid ones. I assumed, that this is the expected behavior, since I can just ignore the ones that are correct, but I thought it was still worth mentioning it

Indeed, since the link checker itself does not check them (as indicated by the message "Email link (not automatically checked)")... maybe we could provide an upstream contribution to https://github.com/DjangoAdminHackers/django-linkcheck to also include (at least format-checking) of email and phone number links in the future...

@seluianova
Copy link
Contributor

Re-tested with the recent changes - ✅

@timobrembeck timobrembeck merged commit f69023d into develop Apr 18, 2023
5 checks passed
@timobrembeck timobrembeck deleted the feature/email-phone-links branch April 18, 2023 10:12
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.

Unable to replace malformed mailto: or tel: links
3 participants