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

Improve path validation of templated created pages #1848

Merged
merged 12 commits into from
Dec 13, 2022

Conversation

BenSurgisonGDS
Copy link
Contributor

@BenSurgisonGDS BenSurgisonGDS commented Dec 9, 2022

See: You can create a page from a template with no name

Added validation and acceptance tests for:

  • Path already exists
  • Path not entered
  • Path only containing spaces
  • Path with only a single slash
  • Path missing a starting slash
  • Path ending in a slash
  • Path containing consecutive slashes
  • Path containing a search parameter
  • Path containing spaces
  • Path including invalid characters

@BenSurgisonGDS BenSurgisonGDS self-assigned this Dec 9, 2022
@BenSurgisonGDS BenSurgisonGDS linked an issue Dec 9, 2022 that may be closed by this pull request
@BenSurgisonGDS BenSurgisonGDS force-pushed the improve-url-validation-of-template-created-pages branch 2 times, most recently from bed3ac7 to fca8c74 Compare December 9, 2022 10:43
@BenSurgisonGDS BenSurgisonGDS marked this pull request as ready for review December 9, 2022 10:59
@joelanman
Copy link
Contributor

don't have time for a full review until later, but one point - I think we talk about a 'path' on the page, and have 'URL' in the errors - we should use 'path' throughout

@lfdebrux lfdebrux force-pushed the improve-url-validation-of-template-created-pages branch from 216f1da to 55e9837 Compare December 9, 2022 16:13
@lfdebrux lfdebrux changed the title Improve url validation of templated created pages Improve path validation of templated created pages Dec 9, 2022
@joelanman
Copy link
Contributor

having problems checking this out to test - I've tried

  • git clean -dfx
  • npm install
  • npm run start:package

and I get

GOV.UK Prototype Kit 13.0.1

starting...
node:internal/modules/cjs/loader:936
  throw err;
  ^

Error: Cannot find module 'govuk-prototype-kit'
Require stack:
- /Users/joelanman/projects/govuk-prototype-kit/cypress/fixtures/extensions/extension-bar/filters.js
- /Users/joelanman/projects/govuk-prototype-kit/lib/utils.js
- /Users/joelanman/projects/govuk-prototype-kit/lib/middleware/authentication/authentication.js
- /Users/joelanman/projects/govuk-prototype-kit/server.js
- /Users/joelanman/projects/govuk-prototype-kit/listen-on-port.js
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
    at Function.Module._load (node:internal/modules/cjs/loader:778:27)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/Users/joelanman/projects/govuk-prototype-kit/cypress/fixtures/extensions/extension-bar/filters.js:1:23)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:1005:19) {
  code: 'MODULE_NOT_FOUND',

.should('contains.text', 'Create new Start page')
cy.get('.govuk-label')
.should('contains.text', 'Path for the new page')
cy.get('#chosen-url')
Copy link
Contributor

Choose a reason for hiding this comment

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

just wondering if we should use path throughout, unless we are sometimes dealing with urls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The headings on the page were not part of the PR. The only reason they are referenced is to for the acceptance tests. Should this be in another PR?

@joelanman
Copy link
Contributor

it should not allow paths with multiple consecutive slashes

Copy link
Contributor

@joelanman joelanman 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, just needs a changelog

@BenSurgisonGDS BenSurgisonGDS force-pushed the improve-url-validation-of-template-created-pages branch from fddfe72 to 88da3ba Compare December 13, 2022 15:00
@BenSurgisonGDS BenSurgisonGDS merged commit 5acba7f into main Dec 13, 2022
@BenSurgisonGDS BenSurgisonGDS deleted the improve-url-validation-of-template-created-pages branch December 13, 2022 18:26
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.

You can create a page from a template with no name
3 participants