Do not get message classes from MESSAGE_TAGS #2552
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If the developer had overridden MESSAGE_TAGS in their site, Wagtail messages used these classes in the admin. This caused the messages to lose their styles. Wagtail now ignores the MESSAGE_TAGS setting, using the default classes defined in `django.contrib.messages.constants.LEVEL_TAGS`. Fixes wagtail#2551
gasman
added a commit
to gasman/wagtail
that referenced
this pull request
Feb 22, 2022
Fixes a test failure against Django main. In wagtail#2552, a fix was applied to ensure that the project-level MESSAGE_TAGS setting was ignored, allowing end-users to customise that setting for their own projects without it leaking into Wagtail admin styles. Unfortunately, the test was flawed (or was broken in a Django regression at some point): in Django <=4.0, MESSAGE_TAGS was not affected by override_settings after the first request, which meant that unless the test was run in isolation, the custom classname that was supposed to flag up the problem never got applied, and the test always succeeded. The change to SVG icons broke the intent of wagtail#2552, since it used message.level_tag for the icon's classname (and this picks up MESSAGE_TAGS customisations), but due to the broken test this went unnoticed. django/django@24b3165 fixed the override_settings behaviour, making the test fail as it should have done long ago. Here we adjust the test to not rely on override_settings (so that it does what it's supposed to do on all Django versions), fix a test that gets broken as a side effect (because it's unnecessarily checking message.level_tag), and fixes our SVG-icon-powered message include to bypass the MESSAGE_TAGS setting like the old implementation did. Confusing? Yes.
gasman
added a commit
to gasman/wagtail
that referenced
this pull request
Feb 22, 2022
Fixes a test failure against Django main. In wagtail#2552, a fix was applied to ensure that the project-level MESSAGE_TAGS setting was ignored, allowing end-users to customise that setting for their own projects without it leaking into Wagtail admin styles. Unfortunately, the test was flawed (or was broken in a Django regression at some point): in Django <=4.0, MESSAGE_TAGS was not affected by override_settings after the first request, which meant that unless the test was run in isolation, the custom classname that was supposed to flag up the problem never got applied, and the test always succeeded. The change to SVG icons broke the intent of wagtail#2552, since it used message.level_tag for the icon's classname (and this picks up MESSAGE_TAGS customisations), but due to the broken test this went unnoticed. django/django@24b3165 fixed the override_settings behaviour, making the test fail as it should have done long ago. Here we adjust the test to not rely on override_settings (so that it does what it's supposed to do on all Django versions), fix a test that gets broken as a side effect (because it's unnecessarily checking message.level_tag), and fixes our SVG-icon-powered message include to bypass the MESSAGE_TAGS setting like the old implementation did. Confusing? Yes.
kaedroho
pushed a commit
that referenced
this pull request
Feb 23, 2022
Fixes a test failure against Django main. In #2552, a fix was applied to ensure that the project-level MESSAGE_TAGS setting was ignored, allowing end-users to customise that setting for their own projects without it leaking into Wagtail admin styles. Unfortunately, the test was flawed (or was broken in a Django regression at some point): in Django <=4.0, MESSAGE_TAGS was not affected by override_settings after the first request, which meant that unless the test was run in isolation, the custom classname that was supposed to flag up the problem never got applied, and the test always succeeded. The change to SVG icons broke the intent of #2552, since it used message.level_tag for the icon's classname (and this picks up MESSAGE_TAGS customisations), but due to the broken test this went unnoticed. django/django@24b3165 fixed the override_settings behaviour, making the test fail as it should have done long ago. Here we adjust the test to not rely on override_settings (so that it does what it's supposed to do on all Django versions), fix a test that gets broken as a side effect (because it's unnecessarily checking message.level_tag), and fixes our SVG-icon-powered message include to bypass the MESSAGE_TAGS setting like the old implementation did. Confusing? Yes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If the developer had overridden MESSAGE_TAGS in their site, Wagtail messages used these classes in the admin. This caused the messages to lose their styles.
Wagtail now ignores the MESSAGE_TAGS setting, using the default classes defined in
django.contrib.messages.constants.LEVEL_TAGS.Fixes #2551