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

feat: error if /made links added to help text #4246

Merged
merged 7 commits into from
Feb 5, 2025

Conversation

jamdelion
Copy link
Contributor

@jamdelion jamdelion commented Feb 3, 2025

🎟️ Trello ticket: https://trello.com/c/Sv8IfEVi/3206-add-validation-to-editor-modals-to-prevent-made-policy-reference-links-from-being-added-to-help-text

What does this PR do?

  • It adds another validation rule to the RichTextInput, similar to the pre-existing getLinkNewTabError. When a link in the RichTextInput breaks this rule, a warning icon appears on the right side of the input:

Screenshot 2025-02-04 at 16 26 28

  • Additionally, since I wanted to prevent editors from proceeding if they broke this rule, I also added a guard function in FormModal.tsx to prevent form submission if the data was found to contain a link ending in /made.
  • Unrelated to main PR subject - splits up List/Public tests out of one single file to try to get round the CI timeout issues. See commit history for clearer changes if diff is too big now!

Questions

  • Are any other validation errors serious enough that we'd like to prevent editors proceeding if the errors are present? (For /made links I decided yes because I think it's worth trying to reduce the number of data migrations we do!)

Copy link

github-actions bot commented Feb 4, 2025

Removed vultr server and associated DNS entries

@jamdelion jamdelion marked this pull request as ready for review February 4, 2025 16:29
@jamdelion jamdelion requested a review from a team February 4, 2025 16:29
Copy link
Contributor

@RODO94 RODO94 left a comment

Choose a reason for hiding this comment

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

I was going to take another look tomorrow, but thought I'd put in an initial review just with one question on an interface you have

text,
}: {
marks:
| {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this part of the interface doing ? it's a union of some keys or undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean the | character at the start? It's just a union of the two types (the object of keys and undefined), but syntactically looks neater when there is the extra | so that everything is nicely in line :) Otherwise they might be at jaunty angles. My editor puts the extra character in automatically in this case

@@ -98,6 +99,18 @@ const NodeTypeSelect: React.FC<{
);
};

const containsMadeLink = (data: any) => {
// An anchor tag where the href attribute ends with '/made'
const regexPattern = /<a[^>]*href=["'].*?\/made["'][^>]*>/;
Copy link
Contributor

Choose a reason for hiding this comment

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

Oooohh cool regex

Copy link
Member

Choose a reason for hiding this comment

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

The matching logic here is not consistent with what you've implemented below in getLegislationLinkError - for example, I can currently trigger this toast with any link ending in /made regardless if it's a www.legislation.gov.uk domain (which is also extra confusing as a user because I won't have the input-level warning icon) ! Can we bring them into sync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing this Jess, you're totally right. This is fixed now!

I also realised my original function was a little too strict so have allowed for e.g. https://legislation.gov.uk... (without the www).

@jamdelion jamdelion changed the title fix: error if /made links added to help text feat: error if /made links added to help text Feb 4, 2025
Copy link
Member

@jessicamcinchak jessicamcinchak left a comment

Choose a reason for hiding this comment

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

Thanks for picking this one up!

✔️ RichTextInput errors working exactly as expected on pizza (shows warning icon for urls, detects issues if multiple links are present, layers errors if open-in-new-tab issue is also present)

🐛 One issue spotted with how the Toast is identifying links though to please address!

➕ I agree that, for now at least, this is the only error serious enough to prevent continuing per your PR question so we can mitigate need to ever repeat this content migration.

@@ -98,6 +99,18 @@ const NodeTypeSelect: React.FC<{
);
};

const containsMadeLink = (data: any) => {
// An anchor tag where the href attribute ends with '/made'
const regexPattern = /<a[^>]*href=["'].*?\/made["'][^>]*>/;
Copy link
Member

Choose a reason for hiding this comment

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

The matching logic here is not consistent with what you've implemented below in getLegislationLinkError - for example, I can currently trigger this toast with any link ending in /made regardless if it's a www.legislation.gov.uk domain (which is also extra confusing as a user because I won't have the input-level warning icon) ! Can we bring them into sync?

text?: string;
}) => boolean | undefined;
errorMessage: string;
}
Copy link
Member

Choose a reason for hiding this comment

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

This is a handy, clear type 👍

Copy link
Member

@jessicamcinchak jessicamcinchak left a comment

Choose a reason for hiding this comment

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

Changes look good, thanks !

@jamdelion jamdelion merged commit 7c47fac into main Feb 5, 2025
13 checks passed
@jamdelion jamdelion deleted the jh/fix-made-links-rich-text branch February 5, 2025 15:20
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.

3 participants