Opened 5 years ago

Closed 3 years ago

#12972 closed Bug (fixed)

flatpages chokes if the url doesn't begin and end with a slash (but doesn't validate for it)

Reported by: jabapyth Owned by: jabapyth
Component: contrib.flatpages Version: master
Severity: Normal Keywords: flatpages, validation, 404
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

There's a helpful hint under the 'url' for creating a new flatpage: "Make sure to have leading and trailing slashes." The strange thing is that it doesn't validate for this, and if you omit the trailing slash, you get a very frustrating 404 when trying to view your finished flatpage.
This patch changes 2 lines, to make the validation regex for the url check for leading and trailing slashes.

Attachments (4)

flatpages_admin.diff (809 bytes) - added by jabapyth 5 years ago.
patch
flatpages_admin_trunk.diff (2.1 KB) - added by jabapyth 5 years ago.
now with tests!
12972.diff (3.3 KB) - added by claudep 4 years ago.
Updated patch
12972-2.diff (3.2 KB) - added by claudep 4 years ago.
Implement check in clean_url instead of regex

Download all attachments as: .zip

Change History (19)

Changed 5 years ago by jabapyth

patch

comment:1 Changed 5 years ago by gvangool

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset

comment:2 Changed 5 years ago by russellm

  • Component changed from Forms to Contrib apps
  • Triage Stage changed from Unreviewed to Accepted

Checking for the trailing slash isn't necessarily a given -- you only need the trailing slash if APPEND_SLASH is enabled.

comment:3 Changed 5 years ago by russellm

  • Patch needs improvement set

Also - please generate patches against the root of the tree.

comment:4 Changed 5 years ago by jabapyth

As far as testing goes, it seems to be a UI change, and therefore hard to unittest -- unless you would like me to write some cases for the regex
to manually examine:

enable APPEND_SLASH (well it is by default), and enable CommonMiddleware (default again)

  • on the admin page, create a new flatpage, but omit the trailing slash in the URL
  • (should fail to validate)

disable APPEND_SLASH

  • create a flatpage, omit the slash
  • (should accept)

comment:5 Changed 5 years ago by jabapyth

  • Needs tests unset
  • Patch needs improvement unset

comment:6 Changed 5 years ago by russellm

  • Needs tests set
  • Patch needs improvement set

Django's testing tools have all sorts of utils for checking form validation. Tests aren't optional here.

Also - PEP8: Constants (url_regex) should be in CAPITALS.

comment:7 Changed 5 years ago by jabapyth

  • Needs tests unset
  • Patch needs improvement unset

I looked around and there didn't seem to be any docs/readme on *how* to add tests to django (other than the basic this is a unittest) -- I mean conventions, organization and the like. so bear with me.
I created a new directory in /tests/regressiontests (I figured that was the right place for it...like I said, there didn't seem to be any intuitive difference between regressiontests and modeltests), called "flatpage_tests", and I put in some URL validation tests. is there anything I should change?

btw. thanks so much for your guidance and council; as you can probably tell I'm fairly new at this. much appreciated.

Changed 5 years ago by jabapyth

now with tests!

comment:8 Changed 4 years ago by gabrielhurley

  • Component changed from Contrib apps to contrib.flatpages

comment:9 Changed 4 years ago by lukeplant

  • Type set to Bug

comment:10 Changed 4 years ago by lukeplant

  • Severity set to Normal

comment:11 Changed 4 years ago by patchhammer

  • Easy pickings unset
  • Patch needs improvement set

flatpages_admin_trunk.diff fails to apply cleanly on to trunk

Changed 4 years ago by claudep

Updated patch

comment:12 Changed 4 years ago by claudep

  • Patch needs improvement unset
  • UI/UX unset

The above attached patch is an updated version of a previous patch. It depends on #17125 to be committed (possibility to modify regex of a RegexField after init). I choose this way of updating the regex because the current trend in Django is to prevent global state. If in the future, settings might vary in the same code base, this patch might be better to address the issue.

comment:13 Changed 4 years ago by kmtracey

  • Patch needs improvement set

It looks to me like you will now (with the latest patch) get a validation error if you are missing the trailing slash...but the error message will be:

This value must contain only letters, numbers, dots, underscores, dashes, slashes or tildes.

correct? That's a pretty confusing error message if what you have supplied does in fact contain only letters, etc. and the actual problem is that you are missing a trailing slash.

Changed 4 years ago by claudep

Implement check in clean_url instead of regex

comment:14 Changed 4 years ago by claudep

  • Patch needs improvement unset

Fair concern. Updated patch that tests for slashes in clean_url instead of using the regex to check it. Now the message errors are indeed more precise.

comment:15 Changed 3 years ago by aaugustin

  • Resolution set to fixed
  • Status changed from new to closed

In [17402]:

Fixed #12972 -- Validated that flatpages URLs start and (when appropriate) end with a slash. Thanks jabapyth, claudep and kmtracey.

Note: See TracTickets for help on using tickets.
Back to Top