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

Fix crossword input validation #20733

Merged
merged 2 commits into from Nov 21, 2018

Conversation

Projects
None yet
4 participants
@zetter
Copy link
Contributor

zetter commented Nov 19, 2018

What does this change?

This improves how accented characters and numbers are handled in crosswords by:

  • allowing numbers to be inputted
  • marking clues as answered when they contain numbers or accented characters

What is the value of this and can you measure success?

Currently certain crosswords are uncompletable. This can be unfulfilling for anyone trying to solve them. Others have seen this too, for example, there's a few comments about it on a recent weekend crossword.

Checklist

Does this affect other platforms?

  • AMP
  • Apps
  • Other (please specify)

Does this affect GLabs Paid Content Pages? Should it have support for Paid Content?

  • No
  • Yes (please give details)

Does this change break ad-free?

  • No
  • It did, but tests caught it and I fixed it
  • It did, but there was no test coverage so I added that then fixed it

Accessibility test checklist

Tested

  • Locally
  • On CODE (optional)

I've worked on extracting the crossword component which has made it easier for me to work on these fixes independently of the rest of the frontend codebase. I've been able to run tests & linters for the isolated component and test it in browser to give me the confidence that this fixes the issue.

—-

I've read that certain contributions require a signed CLA as I'm an external contributor. Please let me know if this is required since I'm happy to do this.

zetter added some commits Nov 19, 2018

Allow numbers to be inputted into crosswords
Sometimes crossword answers have numbers in them. Recently 2 down & 12 
across in Weekend crossword No 410[1] are examples of this.

The existing regular expression to validate input doesn't allow digits, 
causing nothing to happen when they are inputted.

I considered removing this check and replacing it with somthing simpler- 
such as any single non-whitespace character but I'm not quite sure why 
it exists at the moment and it has recently been extended in this way to 
allow accented characters.

[1] https://www.theguardian.com/crosswords/weekend/410
Allow clues containing numbers or accents to be marked as answered
This regex is used to determine if the clue should be marked as answered 
and so greyed out in the clue list. Previously the check that it did was 
more restrictive than the check on input which meant an answer that 
contained accents would never cause a clue to be marked as answered. In 
the previous commit I also allowed answers to contain numbers which 
would have a similar problem without this change.

I could have fixed this by making this regex the same as the input 
validation in crossword.js by duplicating it or sharing the code. 
However, I think a simpler thing to do is to make this less restrictive- 
if we are validating characters on input all we need to do here is check 
that a character has been inputted by making sure the value isn't the 
empty string. The code also works if the value is `null` or `undefined`.
@AWare

This comment has been minimized.

Copy link
Contributor

AWare commented Nov 19, 2018

Hey @zetter this is great.

Our CLA is https://github.com/guardian/frontend/blob/88cfa609c73545085c3e5f3921631ec344a3eb83/cla-individual.txt here.

We're really happy that you've also taken the time to extract the component into its own codebase. I guess we should probably consider whether it should have its own codebase and package instead of living in the frontend repo.

@mchv

This comment has been minimized.

Copy link
Member

mchv commented Nov 20, 2018

@zetter Thanks a lot for your contribution!

@zetter

This comment has been minimized.

Copy link
Contributor Author

zetter commented Nov 21, 2018

Thanks for taking a look at this, I have since submitted a signed CLA.

@AWare

This comment has been minimized.

Copy link
Contributor

AWare commented Nov 21, 2018

N.B. I pulled this branch and pushed it to frontend to build it on teamcity, but it needed rebasing against master. I'm not sure how that build got associated with this PR and the post rebase one didn't. I deployed the rebased branch on CODE and it's AOK 👌🏻

tldr: this builds and deploys okay

@AWare

AWare approved these changes Nov 21, 2018

Copy link
Contributor

AWare left a comment

Thanks for taking the time to make our crosswords better.

@AWare AWare merged commit 59bc718 into guardian:master Nov 21, 2018

14 of 15 checks passed

continuous-integration/teamcity Finished TeamCity Build dotcom :: frontend : Tests failed: 1 (1 new), passed: 1336; build stopped: Scala test failed (found text *** FAILE…
Details
license/snyk - admin/pom.xml (guardian-frontend) No new issues
Details
license/snyk - build.sbt (guardian-frontend) No manifest changes detected
license/snyk - dev/eslint-plugin-guardian-frontend/package.json (guardian-frontend) No manifest changes detected
license/snyk - identity/build.sbt (guardian-frontend) No manifest changes detected
license/snyk - package.json (guardian-frontend) No manifest changes detected
license/snyk - sport/pom.xml (guardian-frontend) No new issues
Details
license/snyk - tools/amp-validation/package.json (guardian-frontend) No manifest changes detected
security/snyk - admin/pom.xml (guardian-frontend) No new issues
Details
security/snyk - build.sbt (guardian-frontend) No manifest changes detected
security/snyk - dev/eslint-plugin-guardian-frontend/package.json (guardian-frontend) No manifest changes detected
security/snyk - identity/build.sbt (guardian-frontend) No manifest changes detected
security/snyk - package.json (guardian-frontend) No manifest changes detected
security/snyk - sport/pom.xml (guardian-frontend) No new issues
Details
security/snyk - tools/amp-validation/package.json (guardian-frontend) No manifest changes detected
@prout-bot

This comment has been minimized.

Copy link
Collaborator

prout-bot commented Nov 21, 2018

Seen on PROD (created by @zetter and merged by @AWare 17 minutes and 4 seconds ago)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment