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

Prevent long Nation Inapplicability Alternative URLs from breaking the database transaction #9087

Merged
merged 1 commit into from
May 30, 2024

Conversation

minhngocd
Copy link
Contributor

URLs which are longer than 255 characters break the database limit, so we are introducing a character limit for URLs to prevent this from happening. Users should seek to shorten their URLs.

https://trello.com/c/CPM8XMsf

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

Follow these steps if you are doing a Rails upgrade.

@minhngocd
Copy link
Contributor Author

minhngocd commented May 29, 2024

Error message content might need a review

image image

@minhngocd minhngocd force-pushed the fix-alternative-url-length branch from 71eb40b to 609c16d Compare May 29, 2024 15:58
Copy link
Contributor

@ryanb-gds ryanb-gds left a comment

Choose a reason for hiding this comment

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

I have a suggestion on the message. My suspicion is we might wait a while for content review. I don't like that the message starts with the field name which is not the same as the label either. We could avoid that by putting the message in a locale file, which might be nice

app/models/nation_inapplicability.rb Outdated Show resolved Hide resolved
@minhngocd minhngocd force-pushed the fix-alternative-url-length branch 2 times, most recently from e222f4b to 7daf6f9 Compare May 29, 2024 16:12
@minhngocd
Copy link
Contributor Author

@ryanb-gds updated with your suggestions and also updated the screenshots to reflect 👍

@minhngocd minhngocd force-pushed the fix-alternative-url-length branch from 7daf6f9 to 4486309 Compare May 29, 2024 16:15
…e database transaction

URLs which are longer than 255 characters break the database limit, so we are introducing a character limit for URLs to prevent this from happening. Users should seek to shorten their URLs.
@minhngocd minhngocd force-pushed the fix-alternative-url-length branch from 4486309 to 5b27eaa Compare May 29, 2024 17:21
@minhngocd
Copy link
Contributor Author

@ryanb-gds Managed to change the humanised version of the attribute so it should read better (updated screenshot).

The only gripe now is the anchor link doesn't quite take you to the field because of the attribute id being coded into the view, and the error summary component not having any connection to this so will not know which of the 3 fields to link to.

Could override the href so that if it is related to Nation Inapplicability alternative URL to just send it to Excluded Nations field anchor instead. but this would create an exceptional case in Error Summary Component which would be ugly. Open to suggestions or i think we could just call it functional enough here.

@ryanb-gds
Copy link
Contributor

@ryanb-gds Managed to change the humanised version of the attribute so it should read better (updated screenshot).

The only gripe now is the anchor link doesn't quite take you to the field because of the attribute id being coded into the view, and the error summary component not having any connection to this so will not know which of the 3 fields to link to.

Could override the href so that if it is related to Nation Inapplicability alternative URL to just send it to Excluded Nations field anchor instead. but this would create an exceptional case in Error Summary Component which would be ugly. Open to suggestions or i think we could just call it functional enough here.

🤯

I think it's probably functional enough? Having tested this on the current codebase the link for the alternative URL field in the error summary doesn't actually work anyway. Rails and conditional fields are not friends

Copy link
Contributor

@ryanb-gds ryanb-gds 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 up ⭐

@minhngocd minhngocd merged commit 41fe244 into main May 30, 2024
19 checks passed
@minhngocd minhngocd deleted the fix-alternative-url-length branch May 30, 2024 08:50
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