Opened 18 years ago

Closed 14 years ago

#1282 closed defect (fixed)

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

Reported by: anonymous Owned by: Nick Efford
Component: Generic views Version: dev
Severity: normal Keywords: sprintsept14 easy-pickings
Cc: justinlilly@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

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 (6)

archive_today.diff (789 bytes ) - added by Jannis Leidel <jl@…> 17 years ago.
make archive_today() respect month_format
archive_today_new.diff (1.4 KB ) - added by Nick Efford 17 years ago.
Improved version of original patch
generic-views.zip (2.1 KB ) - added by Nick Efford 17 years ago.
Beginnings of some unit tests for generic views
patch-1282.diff (4.1 KB ) - added by Nick Efford 16 years ago.
Updated patch
1282.diff (4.0 KB ) - added by Chris Beaven 16 years ago.
ticket1282-rev13294.diff (4.0 KB ) - added by Adam Nelson 14 years ago.
Patch updated as of rev:13294

Download all attachments as: .zip

Change History (33)

comment:1 by anonymous, 18 years ago

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

comment:2 by derelm, 18 years ago

milestone: Version 1.0
Version: SVN

revision 2912 introduces translation hooks for MONTHS_3.

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

comment:3 by (none), 18 years ago

milestone: Version 1.0

Milestone Version 1.0 deleted

comment:4 by Simon G. <dev@…>, 18 years ago

Triage Stage: UnreviewedAccepted

by Jannis Leidel <jl@…>, 17 years ago

Attachment: archive_today.diff added

make archive_today() respect month_format

comment:5 by Jannis Leidel <jl@…>, 17 years ago

Has patch: set
Summary: untranslated MONTHS_3 breaks generic date based views for some locales when not modifying month_format[patch] untranslated MONTHS_3 breaks generic date based views for some locales when not modifying month_format

comment:6 by Simon G. <dev@…>, 17 years ago

Summary: [patch] untranslated MONTHS_3 breaks generic date based views for some locales when not modifying month_formatuntranslated MONTHS_3 breaks generic date based views for some locales when not modifying month_format
Triage Stage: AcceptedReady for checkin

comment:7 by Gary Wilson, 17 years ago

Needs tests: set
Patch needs improvement: set
Summary: untranslated MONTHS_3 breaks generic date based views for some locales when not modifying month_formatarchive_today generic view should accept a month_format parameter instead of hardcoding '%b'
Triage Stage: Ready for checkinAccepted

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.

comment:8 by Nick Efford, 17 years ago

Owner: changed from nobody to Nick Efford
Status: newassigned

comment:9 by Nick Efford, 17 years ago

Owner: changed from Nick Efford to nobody
Status: assignednew

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

comment:10 by Nick Efford, 17 years ago

Owner: changed from nobody to Nick Efford

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 :)

by Nick Efford, 17 years ago

Attachment: archive_today_new.diff added

Improved version of original patch

by Nick Efford, 17 years ago

Attachment: generic-views.zip added

Beginnings of some unit tests for generic views

comment:11 by Nick Efford, 17 years ago

Needs tests: unset
Patch needs improvement: unset

comment:12 by Rob Hunter <robertjhunter@…>, 17 years ago

Keywords: sprintsept14 added
Patch needs improvement: set

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?

comment:13 by Rob Hunter <robertjhunter@…>, 17 years ago

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

comment:14 by Chris Beaven, 16 years ago

Keywords: easy-pickings added

comment:15 by Nick Efford, 16 years ago

Patch needs improvement: unset

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.

by Nick Efford, 16 years ago

Attachment: patch-1282.diff added

Updated patch

comment:16 by Nick Efford, 16 years ago

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

comment:17 by Chris Beaven, 16 years ago

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

by Chris Beaven, 16 years ago

Attachment: 1282.diff added

comment:18 by Chris Beaven, 16 years ago

Triage Stage: AcceptedReady 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.

comment:19 by anonymous, 16 years ago

Cc: justinlilly@… added

comment:20 by Chris Beaven, 16 years ago

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

comment:21 by Adam Nelson, 14 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Patch is missing (is this a problem with all the older uploads?) and anyway, I'm sure in the intervening 2 years this patch would not apply cleanly.

in reply to:  21 comment:22 by Karen Tracey, 14 years ago

What do you mean by patch is missing? If you mean it looks empty when clicked on, that's a bug in trac (I believe) where patches with "\ No newline at end of file" don't display properly. You can still download the patch and it's fine. I also would not necessarily assume that an old patch won't apply any more; depending on the area of code touched it may still be OK. It's probably not helpful to move things back from "Ready for checkin" to accepted just because the patch is old, unless you actually verify that the patch no longer applies.

comment:23 by Adam Nelson, 14 years ago

Hm, wasn't aware of that bug in Trac. Anyway, the patch doesn't apply cleanly on the regressiontests, django/views/generic/date_based.py applies fine.

by Adam Nelson, 14 years ago

Attachment: ticket1282-rev13294.diff added

Patch updated as of rev:13294

comment:24 by Adam Nelson, 14 years ago

Patch needs improvement: unset

comment:25 by Eric Holscher, 14 years ago

Needs documentation: set

This seems like a useful patch. I think the only thing missing is documentation in the generic views docs that it accepts this parameter and it should be RFC.

comment:26 by Jannis Leidel, 14 years ago

This is probably going to be fixed by the latest proposed patch for #6735.

comment:27 by Russell Keith-Magee, 14 years ago

Resolution: fixed
Status: newclosed

Function-based generic views were deprecated by the introduction of class-based views in [14254]. Class-based views should solve this problem.

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