Opened 17 years ago

Closed 16 years ago

Last modified 13 years ago

#4305 closed (duplicate)

[Patch] duplicated settings in the admin site seem to cause confusion

Reported by: rogerpack2005@… Owned by: Julien Phalip
Component: contrib.admin Version: newforms-admin
Severity: Keywords: nfa-someday
Cc: django@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

from the tutorial, if you duplicate settings

fields = (

(None, {'fields': ('pub_date', 'question',)}),
('Date information', {'fields': ('pub_date',)}),

)

Then set only one of the 'duplicated' fields, it responds with an error that they must all be filled in. Odd, but I suppose when you have dupliated fields the error is really yours, anyway, and I'm not sure what it really should do, in that case.

Attachments (2)

duplicate_fieldnames.diff (3.5 KB ) - added by Julien Phalip 16 years ago.
patch + doc + tests
4305.duplicate_fieldnames.diff (2.9 KB ) - added by Julien Phalip 16 years ago.
Updated patch so it uses the ModelAdmin's validation introduced in [7929]

Download all attachments as: .zip

Change History (15)

comment:1 by Chris Beaven, 17 years ago

Resolution: wontfix
Status: newclosed

Perhaps we should show an error, but I'm not convinced it's worth the development time. If anyone wants to provide a patch, feel free to reopen.

comment:2 by Malcolm Tredinnick, 17 years ago

Resolution: wontfix
Status: closedreopened

Somebody might want to write a patch for newforms-admin. It should be easy to detect and worth pointing out to the developer. This is worth leaving open (there are lots of low priority enhancement tickets like this open; it's not really a burden).

comment:3 by Chris Beaven, 17 years ago

Triage Stage: UnreviewedAccepted

Fair enough.

comment:4 by roger, 17 years ago

as a side note also if you duplicate fields within a model specification django replies with odd errors.. (http://betterlogic.com/roger/?p=41) -- you may want to check and throw cleaner errors :) thanks!

comment:5 by James Bennett, 17 years ago

Owner: changed from nobody to jkocherhans
Status: reopenednew
Version: SVNnewforms-admin

This feels like a combination of "you shouldn't be doing that" and "validation of ModelAdmin in newforms-admin should fix this". Changing component and reassigning to jkocherhans.

comment:6 by jkocherhans, 17 years ago

Component: django-admin.pyAdmin interface
Owner: changed from jkocherhans to nobody

Not working on this right now, if someone wants to take it up, #3222 is related.

comment:7 by Karen Tracey <kmtracey@…>, 17 years ago

Keywords: nfa-someday added

comment:8 by Jeff Anderson, 16 years ago

milestone: 1.0

comment:9 by Julien Phalip, 16 years ago

Owner: changed from nobody to Julien Phalip

comment:10 by Julien Phalip, 16 years ago

Cc: django@… added
Has patch: set
Summary: duplicated settings in the admin site seem to cause confusion[Patch+Doc] duplicated settings in the admin site seem to cause confusion

I added a patch with documentation. I also added my name in the AUTHORS as it would be my first contribution, hope that's fine ;)

The patch is I believe the fastest solution for detecting the presence of duplicates. It will raise an exception at compilation time if a duplicate is found. This would force the developer to remove the duplicates, which is probably best practice.

The patch does not identify the actual duplicates to display them in the exception message, but doing so would be overkill and be costly in performance. The developer could easily spot the duplicates himself anyway.

by Julien Phalip, 16 years ago

Attachment: duplicate_fieldnames.diff added

patch + doc + tests

comment:11 by Julien Phalip, 16 years ago

Summary: [Patch+Doc] duplicated settings in the admin site seem to cause confusion[Patch] duplicated settings in the admin site seem to cause confusion

Just added tests to the patch.

E:\Software\workspace\django-newforms-admin\tests>runtests.py --settings=settings modeladmin


Ran 1 test in 0.031s

OK

by Julien Phalip, 16 years ago

Updated patch so it uses the ModelAdmin's validation introduced in [7929]

comment:12 by Julien Phalip, 16 years ago

Resolution: duplicate
Status: newclosed

Closing in favour of #7885 which goes beyond by taking care of both 'fields' and 'fieldsets' attributes.

comment:13 by Jacob, 13 years ago

milestone: 1.0

Milestone 1.0 deleted

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