Opened 16 years ago
Closed 12 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)
Change History (33)
comment:1 Changed 16 years ago by
comment:2 Changed 16 years ago by
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:4 Changed 15 years ago by
Triage Stage: | Unreviewed → Accepted |
---|
Changed 15 years ago by
Attachment: | archive_today.diff added |
---|
make archive_today() respect month_format
comment:5 Changed 15 years ago by
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 15 years ago by
Summary: | [patch] untranslated MONTHS_3 breaks generic date based views for some locales when not modifying month_format → untranslated MONTHS_3 breaks generic date based views for some locales when not modifying month_format |
---|---|
Triage Stage: | Accepted → Ready for checkin |
comment:7 Changed 15 years ago by
Needs tests: | set |
---|---|
Patch needs improvement: | set |
Summary: | untranslated MONTHS_3 breaks generic date based views for some locales when not modifying month_format → archive_today generic view should accept a month_format parameter instead of hardcoding '%b' |
Triage Stage: | Ready for checkin → 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 15 years ago by
Owner: | changed from nobody to Nick Efford |
---|---|
Status: | new → assigned |
comment:9 Changed 15 years ago by
Owner: | changed from Nick Efford to nobody |
---|---|
Status: | assigned → new |
Dropping this so that someone who understands better how to write tests for generic views can have a go.
comment:10 Changed 15 years ago by
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 15 years ago by
Attachment: | archive_today_new.diff added |
---|
Improved version of original patch
Changed 15 years ago by
Attachment: | generic-views.zip added |
---|
Beginnings of some unit tests for generic views
comment:11 Changed 15 years ago by
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
comment:12 Changed 15 years ago by
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 subclassingdjango.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 normallowercase_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 15 years ago by
By the way congratulations writing the first test for generic views! I'll have to settle for second place ;-)
comment:14 Changed 14 years ago by
Keywords: | easy-pickings added |
---|
comment:15 Changed 14 years ago by
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.
comment:16 Changed 14 years ago by
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 14 years ago by
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 14 years ago by
comment:18 Changed 14 years ago by
Triage Stage: | Accepted → 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 14 years ago by
Cc: | justinlilly@… added |
---|
comment:21 follow-up: 22 Changed 12 years ago by
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → 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 Changed 12 years ago by
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 12 years ago by
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.
comment:24 Changed 12 years ago by
Patch needs improvement: | unset |
---|
comment:25 Changed 12 years ago by
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 12 years ago by
This is probably going to be fixed by the latest proposed patch for #6735.
comment:27 Changed 12 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Function-based generic views were deprecated by the introduction of class-based views in [14254]. Class-based views should solve this problem.
btw, while archive_day takes month_format parameter (and respects it), archive_today doesn't do so. it hardcodes strftime('%b') which is wrong.