Django

Code

Ticket #1282 (new)

Opened 3 years ago

Last modified 4 months ago

archive_today generic view should accept a month_format parameter instead of hardcoding '%b'

Reported by: anonymous Assigned to: nickefford
Milestone: Component: Generic views
Version: SVN Keywords: sprintsept14 easy-pickings
Cc: justinlilly@gmail.com Triage Stage: Ready for checkin
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description

builtin archive_day generic view assumes to get a 3 char abbreviated month as month-parameter. MONTHS_3 defines the english values, like "may" or "dec".

when setting the LANGUAGE_CODE to 'de-de' for example (which sets locale?), the abbreviations you get when running strftime('%b'), differ from that and archive_day will fail.

Attachments

archive_today.diff (0.8 kB) - added by Jannis Leidel <jl@websushi.org> on 03/18/07 13:28:17.
make archive_today() respect month_format
archive_today_new.diff (1.4 kB) - added by nickefford on 09/14/07 14:42:48.
Improved version of original patch
generic-views.zip (2.1 kB) - added by nickefford on 09/14/07 14:43:37.
Beginnings of some unit tests for generic views
patch-1282.diff (4.1 kB) - added by nickefford on 07/19/08 07:40:55.
Updated patch
1282.diff (4.0 kB) - added by SmileyChris on 07/24/08 16:58:54.

Change History

01/27/06 08:30:54 changed by anonymous

btw, while archive_day takes month_format parameter (and respects it), archive_today doesn't do so. it hardcodes strftime('%b') which is wrong.

05/16/06 04:54:03 changed by derelm

  • version set to SVN.
  • milestone set to Version 1.0.

revision 2912 introduces translation hooks for MONTHS_3.

the issue with archive_day not respecting month_format is still valid though.

01/17/07 16:12:17 changed by

  • milestone deleted.

Milestone Version 1.0 deleted

01/20/07 16:27:08 changed by Simon G. <dev@simon.net.nz>

  • stage changed from Unreviewed to Accepted.

03/18/07 13:28:17 changed by Jannis Leidel <jl@websushi.org>

  • attachment archive_today.diff added.

make archive_today() respect month_format

03/18/07 13:28:35 changed by Jannis Leidel <jl@websushi.org>

  • has_patch set to 1.
  • summary changed from untranslated MONTHS_3 breaks generic date based views for some locales when not modifying month_format to [patch] untranslated MONTHS_3 breaks generic date based views for some locales when not modifying month_format.

08/20/07 05:04:29 changed by Simon G. <dev@simon.net.nz>

  • summary changed from [patch] untranslated MONTHS_3 breaks generic date based views for some locales when not modifying month_format to untranslated MONTHS_3 breaks generic date based views for some locales when not modifying month_format.
  • stage changed from Accepted to Ready for checkin.

08/25/07 16:24:33 changed by gwilson

  • needs_better_patch set to 1.
  • summary changed from untranslated MONTHS_3 breaks generic date based views for some locales when not modifying month_format to archive_today generic view should accept a month_format parameter instead of hardcoding '%b'.
  • needs_tests set to 1.
  • stage changed from Ready for checkin to Accepted.

The original intent of this ticket was fixed in [2912], so changing the title to reflect the still valid problem of the month_format hardcoding.

Also, I believe month_format should be passed along to achive_day, otherwise it will use %b by default. And we also should have some tests for this generic view.

09/14/07 09:25:23 changed by nickefford

  • owner changed from nobody to nickefford.
  • status changed from new to assigned.

09/14/07 12:39:37 changed by nickefford

  • owner changed from nickefford to nobody.
  • status changed from assigned to new.

Dropping this so that someone who understands better how to write tests for generic views can have a go.

09/14/07 14:38:04 changed by nickefford

  • owner changed from nobody to nickefford.

Reclaiming, because I've managed to figure it out.

The attached patch makes the suggested changes. I also include a zip file containing the beginnings of regression tests for generic views. Unzipping in the top-level directory should install these under tests/regressiontests/generic_views. One of the core devs should probably have a close look at these to make sure I'm doing it sensibly, but the tests pass :)

09/14/07 14:42:48 changed by nickefford

  • attachment archive_today_new.diff added.

Improved version of original patch

09/14/07 14:43:37 changed by nickefford

  • attachment generic-views.zip added.

Beginnings of some unit tests for generic views

09/14/07 14:43:57 changed by nickefford

  • needs_better_patch deleted.
  • needs_tests deleted.

09/14/07 15:40:26 changed by Rob Hunter <robertjhunter@gmail.com>

  • keywords set to sprintsept14.
  • needs_better_patch set to 1.

A few comments:

  • The "application" being loaded for the test is called "generic views" but the URLconf uses date_based explicitly. I'd feel more comfortable with URLs like /generic_views/date_based/archive_today/... so that other generic views have a place to put their tests.
  • The test case inherits from UnitTestCase and instantiates its own client. Did you have a particular reason to avoid subclassing django.test.TestCase class which does just that?
  • The test case sets up several articles with different dates (past present and future) but calls them just Article 1, 2, 3 etc. It would be more illustrative (maybe enough to remove the comment) to call them "Past article", "Today's article 1" and "Article from the Future". (or something)
  • Is counting the number of articles returned a sufficient test? It probably is.
  • The methods on the test case class have camelCase names, instead of Django's normal lowercase_with_underscores standard.
  • I see you had to use the real now() method, which could cause spurious errors if buildbot runs the test just before midnight. With the current code, I don't think there's a nice way around this. :-(
  • Your patch contains a HTML 4 "transitional" template, with content that is never used. How about just an empty file?

09/14/07 15:42:27 changed by Rob Hunter <robertjhunter@gmail.com>

By the way congratulations writing the first test for generic views! I'll have to settle for second place ;-)

02/21/08 15:33:04 changed by SmileyChris

  • keywords changed from sprintsept14 to sprintsept14 easy-pickings.

07/19/08 07:40:14 changed by nickefford

  • needs_better_patch deleted.

I've refreshed the patch for latest (post nfa merge) trunk, so that the tests now build on the existing test infrastructure for generic views.

Note: in addition to applying this patch, it is also necessary to create the template file article_archive_day.html in tests/templates/views. The file can be empty.

07/19/08 07:40:55 changed by nickefford

  • attachment patch-1282.diff added.

Updated patch

07/19/08 07:45:14 changed by nickefford

Hmm, patch seems to be confusing Trac. Inspecting the diff, I can see the following, which might be the source of the problem:

\ No newline at end of file

07/24/08 16:51:14 changed by SmileyChris

Nick, it looks like you forgot to add views/article_archive_day.html in your patch. I'll put a new one up for you

07/24/08 16:58:54 changed by SmileyChris

  • attachment 1282.diff added.

07/24/08 17:00:20 changed by SmileyChris

  • stage changed from Accepted to Ready for checkin.

I should really read notes - I see that you mentioned that already in your previous patch. I just did a few other minor tweaks to the patch so mine is still good (and contains the new template file too).

So in review, the patch is good to go.

07/24/08 21:47:05 changed by anonymous

  • cc set to justinlilly@gmail.com.

07/25/08 17:28:15 changed by SmileyChris

The tests in this patch would be useful for #7944, too.

Add/Change #1282 (archive_today generic view should accept a month_format parameter instead of hardcoding '%b')




Change Properties
Action