Opened 7 years ago

Closed 7 years ago

#5731 closed (fixed)

[newforms-admin] - remove & replace radio_admin

Reported by: Karen Tracey <kmtracey@…> Owned by: brosner
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: UI/UX:

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@…> 7 years ago.
4080.diff (6.3 KB) - added by Karen Tracey <kmtracey@…> 7 years ago.
4080-rebase6783.diff (6.4 KB) - added by Karen Tracey <kmtracey@…> 7 years ago.
4080.diff no longer applied cleanly, rebased on r6783
5731-unified.diff (22.4 KB) - added by programmerq 7 years ago.
unified patch against current newforms-admin

Download all attachments as: .zip

Change History (13)

comment:1 Changed 7 years ago by brosner

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from newforms-admin remove & replace radio_admin to [newforms-admin] - remove & replace radio_admin
  • Triage Stage changed from Unreviewed to Accepted

Changed 7 years ago by Karen Tracey <kmtracey@…>

Changed 7 years ago by Karen Tracey <kmtracey@…>

comment:2 Changed 7 years ago by Karen Tracey <kmtracey@…>

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

  • Has patch set

Changed 7 years ago by Karen Tracey <kmtracey@…>

4080.diff no longer applied cleanly, rebased on r6783

comment:4 Changed 7 years ago by brosner

  • Keywords nfa-blocker added

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

comment:5 Changed 7 years ago by brosner

  • 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 Changed 7 years ago by brosner

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

comment:7 Changed 7 years ago by jkocherhans

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.

Changed 7 years ago by programmerq

unified patch against current newforms-admin

comment:8 Changed 7 years ago by programmerq

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

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

(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