Opened 16 years ago

Closed 16 years ago

#5731 closed (fixed)

[newforms-admin] - remove & replace radio_admin

Reported by: Karen Tracey <kmtracey@…> Owned by: Brian Rosner
Component: contrib.admin Version: newforms-admin
Severity: Keywords: nfa-blocker radio_admin
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

The field attribute radio_admin is out of place (and non-functional) in newforms-admin. It should be removed and replaced, perhaps with a mechanism similar to raw_id_fields. Opening per this thread in djago-developers:

http://groups.google.com/group/django-developers/browse_thread/thread/ea4d86b2b49c2647

Attachments (4)

radio_admin_fields.diff (15.1 KB ) - added by Karen Tracey <kmtracey@…> 16 years ago.
4080.diff (6.3 KB ) - added by Karen Tracey <kmtracey@…> 16 years ago.
4080-rebase6783.diff (6.4 KB ) - added by Karen Tracey <kmtracey@…> 16 years ago.
4080.diff no longer applied cleanly, rebased on r6783
5731-unified.diff (22.4 KB ) - added by Jeff Anderson 16 years ago.
unified patch against current newforms-admin

Download all attachments as: .zip

Change History (13)

comment:1 by Brian Rosner, 16 years ago

Summary: newforms-admin remove & replace radio_admin[newforms-admin] - remove & replace radio_admin
Triage Stage: UnreviewedAccepted

by Karen Tracey <kmtracey@…>, 16 years ago

Attachment: radio_admin_fields.diff added

by Karen Tracey <kmtracey@…>, 16 years ago

Attachment: 4080.diff added

comment:2 by Karen Tracey <kmtracey@…>, 16 years ago

Attached radio_admin_fields.diff, a patch which removes the old radio_admin Field attribute and replaces it with
radio_admin_fields in ModelAdmin. Here's some text for the newforms-admin branch page:


Moved radio_admin from the model definition

The syntax is now separated from the definition of your models.

An example:

# OLD:
class MyModel(models.Model):
    field1 = models.ForeignKey(AnotherModel, radio_admin=True)
    field2 = models.CharField(max_length=100, radio_admin=models.VERTICAL)
    class Admin:
        pass

# NEW:
class MyModel_Options(admin.ModelAdmin):
    model = MyModel
    radio_admin_fields = {'field1': admin.HORIZONTAL, 'field2': admin.VERTICAL }

The use of models.HORIZONTAL/VERTICAL was not documented, but it was used by django.contrib.redirects, so I
maintained the old behavior. I included my best shot at some tests and doc...what doc I found for the new
scheme is pretty bare bones so I just plunked down a revised version of the old doc around where it looks
like the doc for ModelAdmin specifics will go.

The second patch, 4080.diff, is a fix needed to make the display of radio_admin_fields look pretty, but
it's really a separate issue so I split it out from the other patch. The code fix is part of the proposed
fix for #4080 (the rest of that proposed fix has already been implemented through fixing some other ticket),
plus 4080.diff fixes the testcases affected by the fix. #4080 was closed as a dup of #4117, which is a
broader issue, and that one is waiting in design decision needed.

All that is needed for the radio_admin_fields display to look pretty is for the attrs specified on creation
of the RadioSelect widget to be included on the <ul> generated in the output -- right now the attrs are only
being included on the individual <li>s. The simple fix proposed by #4080 seems correct to me, so that is what
I used to test with for radio_admin_fields. If some other fix is made for #4117 then the radio_admin_fields
code might need to change slightly. (The effect of running without any fix for this issue is that the
RadioSelect list is bulleted, which looks funny, and items are not spaced as nicely as they could be.)

(There is also the capability to sidestep the whole issue of #4080 by writing a custom renderer, but ugh,
that would be 99% duplicated code with the default renderer. My impression was tht capability was added
for cases where the desired output was something other than an unordered list, which is not the case for
radio_admin_fields, so I did not pursue that path.)

comment:3 by Karen Tracey <kmtracey@…>, 16 years ago

Has patch: set

by Karen Tracey <kmtracey@…>, 16 years ago

Attachment: 4080-rebase6783.diff added

4080.diff no longer applied cleanly, rebased on r6783

comment:4 by Brian Rosner, 16 years ago

Keywords: nfa-blocker added

This looks like some functionality that breaks current admin. Tagging with nfa-blocker.

comment:5 by Brian Rosner, 16 years ago

Patch needs improvement: set

The patch here looks good, but I would recommend two things so this can be pushed to Ready for Checkin:

  1. Don't remove the legacy radio_admin code. I rather see patches that don't change too much in the core until we know exactly how that will handled. Get in touch with Joseph about that.
  2. Make a patch that includes all the changes that need to be made up to the current revision. It looks like there are two patches that are patches on top of the original.

comment:6 by Brian Rosner, 16 years ago

Owner: changed from nobody to Brian Rosner
Status: newassigned

comment:7 by jkocherhans, 16 years ago

I'd say go ahead and yank the old code if it isn't being used. We have trunk and version control if we need to look at the old stuff.

by Jeff Anderson, 16 years ago

Attachment: 5731-unified.diff added

unified patch against current newforms-admin

comment:8 by Jeff Anderson, 16 years ago

radio-admin-fields.diff applied "fuzzily" against current newforms-admin. The unified patch includes both radio-admin-fields.diff and 4080-rebase6783.diff

comment:9 by Brian Rosner, 16 years ago

Resolution: fixed
Status: assignedclosed

(In [7626]) newforms-admin: Fixed #5731 -- Implemented ModelAdmin.radio_fields to match trunk's radio_admin. Removed legacy code and added tests. Thanks Karen Tracey for the initial work.

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