Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #24908 -- Duplicate readonly field rendering. #4824

Closed
wants to merge 2 commits into from

Conversation

jpic
Copy link
Contributor

@jpic jpic commented Jun 6, 2015

ModelAdmin added readonly_fields to exclude, but would not un-declare
them if they were overridden, causing the readonly field to be rendered
twice.

https://code.djangoproject.com/ticket/24908

ModelAdmin added readonly_fields to exclude, but would not un-declare
them if they were overridden.
@@ -613,8 +614,14 @@ def get_form(self, request, obj=None, **kwargs):
# if exclude is an empty list we pass None to be consistent with the
# default on modelform_factory
exclude = exclude or None

# Cancel out overridden form fields which are in readonly_fields.
new_attrs = OrderedDict([(f, None) for f in readonly_fields
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think keeping the attributes as None on the modelform is the right approach. I think they shouldn't be there at all.

Furthermore, we use hanging indents or keep everything in one line if it doesn't exceed 119 characters (I don't think you need the explicit list in there, a generator should work:

new_attrs = OrderedDict(
    (f, None) for f in readonly_fields
    if f in self.form.declared_fields
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think keeping the attributes as None on the modelform is the right approach.

It's the official way of un-defining overridden model form fields, if i understand ticket #8620 and commit 9fb0f5d correctly.

I think they shouldn't be there at all.

How would you suggest we do this ?

@MarkusH
Copy link
Member

MarkusH commented Jun 14, 2015

As an aside: please do not add @ mentions in the commit messages. People get notified on every push (I think) of that commit to GitHub ;)

@timgraham
Copy link
Member

merged in fedef7b, thanks!

@timgraham timgraham closed this Jul 2, 2015
@jpic
Copy link
Contributor Author

jpic commented Jul 3, 2015

I thought you'd want me to rebase first anyway, my bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants