Code

Opened 6 years ago

Closed 6 years ago

#6058 closed (fixed)

6776 mark_safe breaks newforms_admin

Reported by: tvrg Owned by: jkocherhans
Component: contrib.admin Version: newforms-admin
Severity: Keywords: newforms-admin
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

In the latest changeset [6776] on newforms-admin breaks the admin interface

some return's are merged to use the mark_safe method, but the imports were not adapted/merged

see patch

Attachments (3)

nfa_marksafe.patch (428 bytes) - added by tvrg 6 years ago.
00-admin-autoescape.diff (5.0 KB) - added by Petr Marhoun <petr.marhoun@…> 6 years ago.
autoescape-kmt.diff (5.2 KB) - added by Karen Tracey <kmtracey@…> 6 years ago.

Download all attachments as: .zip

Change History (15)

Changed 6 years ago by tvrg

comment:1 Changed 6 years ago by tvrg

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from tom to tvrg
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Ready for checkin

comment:2 Changed 6 years ago by tvrg

  • Version changed from SVN to newforms-admin

Changed 6 years ago by Petr Marhoun <petr.marhoun@…>

comment:3 Changed 6 years ago by Petr Marhoun <petr.marhoun@…>

I have more problems - see the patch.

I don't understand autoescape fully so I am not sure with solutions. But problems are real.

comment:4 Changed 6 years ago by mtredinnick

Comment 3 is a dupe of #6059. Let's keep one issue per ticket, please.

comment:5 follow-up: Changed 6 years ago by Karen Tracey <kmtracey@…>

Yes, the change to django/contrib/admin/templates/admin/change_form.html is an alternative fix for #6059. I took the approach of using mark_safe on the variable before sending it to the template instead of using the safe filter in template...got the impression from other code this was a preferred way to handle it?

The changes to django/contrib/admin/widgets.py are similarly covered by #6060, though the #6060 patch also attempts to mark_safe the output of the AdminSplitDateTime widget, so there's a slight difference there.

I haven't yet run into the problems requiring the changes to:

django/contrib/admin/templates/admin/change_list_results.html

django/contrib/admin/templates/admin/includes/fieldset.html

django/newforms/widgets.py

so they might be needing their own tickets, since Malcolm indicates we should stick to one issue per ticket. Though I also see how this could be viewed as all one big issue -- autoescape and newforms-admin...

comment:6 in reply to: ↑ 5 ; follow-up: Changed 6 years ago by Petr Marhoun <petr.marhoun@…>

Replying to Karen Tracey <kmtracey@gmail.com>:

The changes to django/contrib/admin/widgets.py are similarly covered by #6060, though the #6060 patch also attempts to mark_safe the output of the AdminSplitDateTime widget, so there's a slight difference there.

AdminSplitDateTimeWidget inherits from SplitDateTimeWidget and SplitDateTimeWidgets inherits from MultiWidget. I don't know which place is best for fixing - I have decided for MultiWidget.

I haven't yet run into the problems requiring the changes to:

django/contrib/admin/templates/admin/change_list_results.html

Maybe you haven't saw them only - could you check style for sorting? I sometimes use application/xhtml+xml and then I see (without patch):

XML Parsing Error: not well-formed
Location: http://hradec:8000/admin/auth/user/
Line Number 129, Column 11:<th class=&quot;sorted ascending&quot;>
----------^


django/contrib/admin/templates/admin/includes/fieldset.html

It is for validation errors.

django/newforms/widgets.py

It is the MultiWidget.

so they might be needing their own tickets, since Malcolm indicates we should stick to one issue per ticket. Though I also see how this could be viewed as all one big issue -- autoescape and newforms-admin...

Yes, it was my idea (one big issue) and the time difference was only a few minutes - when I started to read my patches before uploading, #6059 and #6060 were not here. But I am afraid I have no time today for new tickets and patches.

comment:7 in reply to: ↑ 6 Changed 6 years ago by Karen Tracey <kmtracey@…>

Replying to Petr Marhoun <petr.marhoun@gmail.com>:

Replying to Karen Tracey <kmtracey@gmail.com>:

The changes to django/contrib/admin/widgets.py are similarly covered by #6060, though the #6060 patch also attempts to mark_safe the output of the AdminSplitDateTime widget, so there's a slight difference there.

AdminSplitDateTimeWidget inherits from SplitDateTimeWidget and SplitDateTimeWidgets inherits from MultiWidget. I don't know which place is best for fixing - I have decided for MultiWidget.

Ah, yes, that's probably a better place to do it.

I haven't yet run into the problems requiring the changes to:

django/contrib/admin/templates/admin/change_list_results.html

Maybe you haven't saw them only - could you check style for sorting? I sometimes use application/xhtml+xml and then I see (without patch):

XML Parsing Error: not well-formed
Location: http://hradec:8000/admin/auth/user/
Line Number 129, Column 11:<th class=&quot;sorted ascending&quot;>
----------^

OK, I see the escaping in the source, it just wasn't causing a visible problem in my environment/browser. A better (in that I don't see much use of the safe filter in the admin templates, but rather mark_safe used in the code) place to fix it may be in templatetags/admin_list.py, where the variable is set.


django/contrib/admin/templates/admin/includes/fieldset.html

It is for validation errors.

Hadn't gotten to validating input earlier. This one can be fixed at the source in options.py by calling mark_safe on the return value of errors for FieldLine.

django/newforms/widgets.py

It is the MultiWidget.

Gotcha.

so they might be needing their own tickets, since Malcolm indicates we should stick to one issue per ticket. Though I also see how this could be viewed as all one big issue -- autoescape and newforms-admin...

Yes, it was my idea (one big issue) and the time difference was only a few minutes - when I started to read my patches before uploading, #6059 and #6060 were not here. But I am afraid I have no time today for new tickets and patches.

Yeah, we were running into the same problems simultaneously (though I had seen your fix for the missing import when I started). I too am running out of time to spend on this today, so I am going to attach one last diff to this ticket, that contains all the fixes I am using. Similar to the 2nd patch except I used mark_safe at the source rather than the safe filter in the templates. I see Joseph is starting to commit fixes anyway so shortly a new checkout will probably resolve all of these.

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

comment:8 Changed 6 years ago by tvrg

I agree, there is a lot more to this, I just patched what i noticed this morning, and then was busy fixing some of my own code that broke after the changeset.

comment:9 Changed 6 years ago by tvrg

  • Needs tests set
  • Triage Stage changed from Ready for checkin to Accepted

comment:10 Changed 6 years ago by jkocherhans

  • Owner changed from tvrg to jkocherhans

I'll be working on tests shortly based on Karen's last patch. tvrg, if you are planning on writing the tests, and expect you'll be able to finish in the next few hours, please reassign yourself to this ticket.

comment:11 Changed 6 years ago by tvrg

I won't finish any tests today (considering day and timezone :))

comment:12 Changed 6 years ago by jkocherhans

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

Doh. This was fixed in [6782], but I put the wrong ticket number in the commit message.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.