Opened 9 years ago

Closed 8 years ago

Last modified 5 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: UI/UX:

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 8 years ago.
patch + doc + tests
4305.duplicate_fieldnames.diff (2.9 KB) - added by Julien Phalip 8 years ago.
Updated patch so it uses the ModelAdmin's validation introduced in [7929]

Download all attachments as: .zip

Change History (15)

comment:1 Changed 9 years ago by Chris Beaven

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
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 Changed 9 years ago by Malcolm Tredinnick

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

Triage Stage: UnreviewedAccepted

Fair enough.

comment:4 Changed 9 years ago by roger

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 Changed 9 years ago by James Bennett

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

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 Changed 9 years ago by Karen Tracey <kmtracey@…>

Keywords: nfa-someday added

comment:8 Changed 8 years ago by Jeff Anderson

milestone: 1.0

comment:9 Changed 8 years ago by Julien Phalip

Owner: changed from nobody to Julien Phalip

comment:10 Changed 8 years ago by Julien Phalip

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.

Changed 8 years ago by Julien Phalip

Attachment: duplicate_fieldnames.diff added

patch + doc + tests

comment:11 Changed 8 years ago by Julien Phalip

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

Changed 8 years ago by Julien Phalip

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

comment:12 Changed 8 years ago by Julien Phalip

Resolution: duplicate
Status: newclosed

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

comment:13 Changed 5 years ago by Jacob

milestone: 1.0

Milestone 1.0 deleted

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