Code

Opened 8 years ago

Closed 3 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: nickefford
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@…> 7 years ago.
make archive_today() respect month_format
archive_today_new.diff (1.4 KB) - added by nickefford 7 years ago.
Improved version of original patch
generic-views.zip (2.1 KB) - added by nickefford 7 years ago.
Beginnings of some unit tests for generic views
patch-1282.diff (4.1 KB) - added by nickefford 6 years ago.
Updated patch
1282.diff (4.0 KB) - added by SmileyChris 6 years ago.
ticket1282-rev13294.diff (4.0 KB) - added by adamnelson 4 years ago.
Patch updated as of rev:13294

Download all attachments as: .zip

Change History (33)

comment:1 Changed 8 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 8 years ago by derelm

  • milestone set to Version 1.0
  • Version set to 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 7 years ago by anonymous

  • milestone Version 1.0 deleted

Milestone Version 1.0 deleted

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

  • Triage Stage changed from Unreviewed to Accepted

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

make archive_today() respect month_format

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

  • Has patch set
  • 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

comment:6 Changed 7 years ago by Simon G. <dev@…>

  • 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
  • Triage Stage changed from Accepted to Ready for checkin

comment:7 Changed 7 years ago by gwilson

  • Needs tests set
  • Patch needs improvement set
  • 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'
  • Triage 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.

comment:8 Changed 7 years ago by nickefford

  • Owner changed from nobody to nickefford
  • Status changed from new to assigned

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

comment:10 Changed 7 years ago 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 :)

Changed 7 years ago by nickefford

Improved version of original patch

Changed 7 years ago by nickefford

Beginnings of some unit tests for generic views

comment:11 Changed 7 years ago by nickefford

  • Needs tests unset
  • Patch needs improvement unset

comment:12 Changed 7 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 7 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 6 years ago by SmileyChris

  • Keywords easy-pickings added

comment:15 Changed 6 years ago by nickefford

  • 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 6 years ago by nickefford

Updated patch

comment:16 Changed 6 years ago 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

comment:17 Changed 6 years ago 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

Changed 6 years ago by SmileyChris

comment:18 Changed 6 years ago by SmileyChris

  • Triage 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.

comment:19 Changed 6 years ago by anonymous

  • Cc justinlilly@… added

comment:20 Changed 6 years ago by SmileyChris

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

comment:21 follow-up: Changed 4 years ago by adamnelson

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 4 years ago by kmtracey

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 4 years ago by adamnelson

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 4 years ago by adamnelson

Patch updated as of rev:13294

comment:24 Changed 4 years ago by adamnelson

  • Patch needs improvement unset

comment:25 Changed 4 years ago by ericholscher

  • 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 4 years ago by jezdez

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

comment:27 Changed 3 years ago by russellm

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

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.