Opened 11 years ago

Closed 6 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: master
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: UI/UX:

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@…> 10 years ago.
make archive_today() respect month_format
archive_today_new.diff (1.4 KB) - added by Nick Efford 9 years ago.
Improved version of original patch
generic-views.zip (2.1 KB) - added by Nick Efford 9 years ago.
Beginnings of some unit tests for generic views
patch-1282.diff (4.1 KB) - added by Nick Efford 8 years ago.
Updated patch
1282.diff (4.0 KB) - added by Chris Beaven 8 years ago.
ticket1282-rev13294.diff (4.0 KB) - added by Adam Nelson 7 years ago.
Patch updated as of rev:13294

Download all attachments as: .zip

Change History (33)

comment:1 Changed 11 years ago 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.

comment:2 Changed 11 years ago by derelm

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 Changed 10 years ago by (none)

milestone: Version 1.0

Milestone Version 1.0 deleted

comment:4 Changed 10 years ago by Simon G. <dev@…>

Triage Stage: UnreviewedAccepted

Changed 10 years ago by Jannis Leidel <jl@…>

Attachment: archive_today.diff added

make archive_today() respect month_format

comment:5 Changed 10 years ago by Jannis Leidel <jl@…>

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 Changed 9 years ago by Simon G. <dev@…>

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 Changed 9 years ago by Gary Wilson

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 Changed 9 years ago by Nick Efford

Owner: changed from nobody to Nick Efford
Status: newassigned

comment:9 Changed 9 years ago by Nick Efford

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 Changed 9 years ago by Nick Efford

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

Changed 9 years ago by Nick Efford

Attachment: archive_today_new.diff added

Improved version of original patch

Changed 9 years ago by Nick Efford

Attachment: generic-views.zip added

Beginnings of some unit tests for generic views

comment:11 Changed 9 years ago by Nick Efford

Needs tests: unset
Patch needs improvement: unset

comment:12 Changed 9 years ago by Rob Hunter <robertjhunter@…>

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 Changed 9 years ago by Rob Hunter <robertjhunter@…>

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

comment:14 Changed 9 years ago by Chris Beaven

Keywords: easy-pickings added

comment:15 Changed 8 years ago by Nick Efford

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.

Changed 8 years ago by Nick Efford

Attachment: patch-1282.diff added

Updated patch

comment:16 Changed 8 years ago by Nick Efford

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 Changed 8 years ago by Chris Beaven

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

Changed 8 years ago by Chris Beaven

Attachment: 1282.diff added

comment:18 Changed 8 years ago by Chris Beaven

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 Changed 8 years ago by anonymous

Cc: justinlilly@… added

comment:20 Changed 8 years ago by Chris Beaven

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

comment:21 Changed 7 years ago by Adam Nelson

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.

comment:22 in reply to:  21 Changed 7 years ago by Karen Tracey

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 Changed 7 years ago by Adam Nelson

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.

Changed 7 years ago by Adam Nelson

Attachment: ticket1282-rev13294.diff added

Patch updated as of rev:13294

comment:24 Changed 7 years ago by Adam Nelson

Patch needs improvement: unset

comment:25 Changed 6 years ago by Eric Holscher

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 Changed 6 years ago by Jannis Leidel

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

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

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