Opened 7 years ago

Closed 5 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: Jared Forsyth Owned by: Jared Forsyth
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 Jared Forsyth 7 years ago.
patch
flatpages_admin_trunk.diff (2.1 KB) - added by Jared Forsyth 7 years ago.
now with tests!
12972.diff (3.3 KB) - added by Claude Paroz 5 years ago.
Updated patch
12972-2.diff (3.2 KB) - added by Claude Paroz 5 years ago.
Implement check in clean_url instead of regex

Download all attachments as: .zip

Change History (19)

Changed 7 years ago by Jared Forsyth

Attachment: flatpages_admin.diff added

patch

comment:1 Changed 7 years ago by Gert Van Gool

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

comment:2 Changed 7 years ago by Russell Keith-Magee

Component: FormsContrib apps
Triage Stage: UnreviewedAccepted

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

comment:3 Changed 7 years ago by Russell Keith-Magee

Patch needs improvement: set

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

comment:4 Changed 7 years ago by Jared Forsyth

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 7 years ago by Jared Forsyth

Needs tests: unset
Patch needs improvement: unset

comment:6 Changed 7 years ago by Russell Keith-Magee

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 7 years ago by Jared Forsyth

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 7 years ago by Jared Forsyth

Attachment: flatpages_admin_trunk.diff added

now with tests!

comment:8 Changed 6 years ago by Gabriel Hurley

Component: Contrib appscontrib.flatpages

comment:9 Changed 5 years ago by Luke Plant

Type: Bug

comment:10 Changed 5 years ago by Luke Plant

Severity: Normal

comment:11 Changed 5 years ago by patchhammer

Easy pickings: unset
Patch needs improvement: set

flatpages_admin_trunk.diff fails to apply cleanly on to trunk

Changed 5 years ago by Claude Paroz

Attachment: 12972.diff added

Updated patch

comment:12 Changed 5 years ago by Claude Paroz

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 5 years ago by Karen Tracey

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 5 years ago by Claude Paroz

Attachment: 12972-2.diff added

Implement check in clean_url instead of regex

comment:14 Changed 5 years ago by Claude Paroz

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 5 years ago by Aymeric Augustin

Resolution: fixed
Status: newclosed

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