Fixed #25135 -- Deprecated the contrib.admin allow_tags attribute #5215
Conversation
| if getattr(attr, "allow_tags", False): | ||
| warnings.warn( | ||
| "The allow_tags attribute is deprecated. Return a SafeString instance by " | ||
| "using format_html, format_html_join or mark_safe instead.", |
There was a problem hiding this comment.
format_html(), format_html_join(), or mark_safe()
|
It would be nice to retain a test to ensure |
|
I fixed all of the things mentioned by you. Let me know if there is anything that can be improved. Thanks a lot! (Wow, admin tests are not fun! 💀 ) |
olasitarska
force-pushed
the
25135-allow-tags-in-admin
branch
2 times, most recently
from
c10f441
to
bafd016
Compare
|
@timgraham any ideas what's broken in this build? I can't find anything useful in details of it. |
|
It's nothing to worry about, see https://code.djangoproject.com/wiki/Jenkins#Troubleshooting. |
|
Cool! Thanks Tim! Should I close and reopen this PR to trigger a new build? ;) |
|
One failure in the matrix doesn't matter for review purposes. |
|
👍 |
| @@ -201,18 +203,19 @@ def link_in_col(is_first, field_name, cl): | |||
| except ObjectDoesNotExist: | |||
| result_repr = empty_value_display | |||
| else: | |||
| empty_value_display = getattr(attr, 'empty_value_display', empty_value_display) | |||
| empty_value_display = mark_safe(getattr(attr, 'empty_value_display', empty_value_display)) | |||
There was a problem hiding this comment.
I don't see a reason why we should assume empty_value_display is safe. Patch to fix this and some other edits: http://dpaste.com/2TETT95
Does the new allow_tags test cover django/contrib/admin/helpers.py too? I don't think so as I think it's only part of the change form page (not change list).
|
Applied your patch, thanks! I'm gonna see how I can test it in helpers.py too, can't find a place for it yet. Will try tomorrow morning, it's late here now! 🌔 |
|
Probably |
|
Not sure how to make |
|
Not sure I actually know what I'm doing, but @timgraham any ideas why my local isort wants to sort differently than the check here? Not an issue now, but would be awesome if I could fix my local setup for the future :) |
| @@ -429,7 +430,8 @@ class PostAdmin(admin.ModelAdmin): | |||
| list_display = ['title', 'public'] | |||
| readonly_fields = ( | |||
| 'posted', 'awesomeness_level', 'coolness', 'value', | |||
| 'multiline', 'multiline_html', lambda obj: "foo" | |||
| 'multiline', 'multiline_html', 'multiline_html_allow_tags', | |||
There was a problem hiding this comment.
instead of sorting these, I'd just put 'multiline_html_allow_tags' on the next newline so that we don't need to modify more than one line when removing the deprecated feature
|
Looks good. Could you please squash commits when making those updates? |
olasitarska
force-pushed
the
25135-allow-tags-in-admin
branch
from
4a44d52
to
64efb48
Compare
|
Done, and squashed! :) 🎾 |
|
merged in f2f8972, thanks! |
|
Thanks for help @timgraham! :) |
This fixes the problems with #5024 PR, hopefully :)