feat(redesign): introduce new_base.html with waffle flag #4940

elisa-a-v · 2025-01-18T01:41:49Z

This PR adds a middleware that checks a waffle flag (use_new_design) and changes the old template with the new one when a new template exists.

It also includes a copy of base.html without bootstrap, and a new template for the help index that extends from it, both of which need to be updated when tailwind support is ready; things do NOT look good!

To identify the new template we prepend "v2_" to the old template name, which means if the old template_name includes a dir, like "help/index.html", the new template should be in "v2_help/index.html" and NOT in "help/v2_index.html".

The middleware checks a waffle flag to gradually roll out the new design when a requested view has a new template available.

- This file will use tailwind instead of bootstrap once tailwind support has been implemented. - Templates with new design should extend from new_base.html instead of the old one.

New template extends from new_base.html and will be updated with the new design in #4740. It's added now as an MVP for #4815.

semgrep-app · 2025-01-18T01:42:45Z

cl/assets/templates/new_base.html

+  {# Default values are to ensure JS parsing even if 500 error thrown #}
+  var isMember = {{ user.profile.is_member|yesno:"true,false" }},
+      userAlertCount = {{ user.docket_alerts.subscriptions.count|default:"0" }},
+      priceRtAlerts = parseFloat({{ MIN_DONATION.rt_alerts|default:0 }}),


Semgrep identified an issue in your code:

Detected a template variable used in a script tag. Although template variables are HTML escaped, HTML escaping does not always prevent cross-site scripting (XSS) attacks when used directly in JavaScript. If you need this data on the rendered page, consider placing it in the HTML portion (outside of a script tag). Alternatively, use a JavaScript-specific encoder, such as the one available in OWASP ESAPI. For Django, you may also consider using the 'json_script' template tag and retrieving the data in your script by using the element ID (e.g., document.getElementById).

To resolve this comment:

🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.

💬 Ignore this finding

Leave a nosemgrep comment directly above or at the end of line 165 like so // nosemgrep: generic.html-templates.security.var-in-script-tag.var-in-script-tag

Take care to validate that this is not a true positive finding before ignoring it.
Learn more about ignoring code, files and folders here.

_{You can view more details about this finding in the Semgrep AppSec Platform.}

semgrep-app · 2025-01-18T01:42:46Z

cl/assets/templates/new_base.html

+<script type="text/javascript" nonce="{{ request.csp_nonce }}">
+  {# Default values are to ensure JS parsing even if 500 error thrown #}
+  var isMember = {{ user.profile.is_member|yesno:"true,false" }},
+      userAlertCount = {{ user.docket_alerts.subscriptions.count|default:"0" }},


Semgrep identified an issue in your code:

Detected a template variable used in a script tag. Although template variables are HTML escaped, HTML escaping does not always prevent cross-site scripting (XSS) attacks when used directly in JavaScript. If you need this data on the rendered page, consider placing it in the HTML portion (outside of a script tag). Alternatively, use a JavaScript-specific encoder, such as the one available in OWASP ESAPI. For Django, you may also consider using the 'json_script' template tag and retrieving the data in your script by using the element ID (e.g., document.getElementById).

To resolve this comment:

🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.

💬 Ignore this finding

Leave a nosemgrep comment directly above or at the end of line 164 like so // nosemgrep: generic.html-templates.security.var-in-script-tag.var-in-script-tag

Take care to validate that this is not a true positive finding before ignoring it.
Learn more about ignoring code, files and folders here.

_{You can view more details about this finding in the Semgrep AppSec Platform.}

semgrep-app · 2025-01-18T01:42:47Z

cl/assets/templates/new_base.html

+<script type="text/javascript" src="{% static "js/base.js" %}"></script>
+<script type="text/javascript" nonce="{{ request.csp_nonce }}">
+  {# Default values are to ensure JS parsing even if 500 error thrown #}
+  var isMember = {{ user.profile.is_member|yesno:"true,false" }},


Semgrep identified an issue in your code:

Detected a template variable used in a script tag. Although template variables are HTML escaped, HTML escaping does not always prevent cross-site scripting (XSS) attacks when used directly in JavaScript. If you need this data on the rendered page, consider placing it in the HTML portion (outside of a script tag). Alternatively, use a JavaScript-specific encoder, such as the one available in OWASP ESAPI. For Django, you may also consider using the 'json_script' template tag and retrieving the data in your script by using the element ID (e.g., document.getElementById).

To resolve this comment:

🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.

💬 Ignore this finding

Leave a nosemgrep comment directly above or at the end of line 163 like so // nosemgrep: generic.html-templates.security.var-in-script-tag.var-in-script-tag

Take care to validate that this is not a true positive finding before ignoring it.
Learn more about ignoring code, files and folders here.

_{You can view more details about this finding in the Semgrep AppSec Platform.}

semgrep-app · 2025-01-18T01:42:48Z

cl/assets/templates/new_base.html

+  var isMember = {{ user.profile.is_member|yesno:"true,false" }},
+      userAlertCount = {{ user.docket_alerts.subscriptions.count|default:"0" }},
+      priceRtAlerts = parseFloat({{ MIN_DONATION.rt_alerts|default:0 }}),
+      maxFreeDocketAlerts = {{ MAX_FREE_DOCKET_ALERTS|default:0 }},


Semgrep identified an issue in your code:

Detected a template variable used in a script tag. Although template variables are HTML escaped, HTML escaping does not always prevent cross-site scripting (XSS) attacks when used directly in JavaScript. If you need this data on the rendered page, consider placing it in the HTML portion (outside of a script tag). Alternatively, use a JavaScript-specific encoder, such as the one available in OWASP ESAPI. For Django, you may also consider using the 'json_script' template tag and retrieving the data in your script by using the element ID (e.g., document.getElementById).

To resolve this comment:

🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.

💬 Ignore this finding

Leave a nosemgrep comment directly above or at the end of line 166 like so // nosemgrep: generic.html-templates.security.var-in-script-tag.var-in-script-tag

Take care to validate that this is not a true positive finding before ignoring it.
Learn more about ignoring code, files and folders here.

_{You can view more details about this finding in the Semgrep AppSec Platform.}

semgrep-app · 2025-01-18T01:42:49Z

cl/assets/templates/new_base.html

+      userAlertCount = {{ user.docket_alerts.subscriptions.count|default:"0" }},
+      priceRtAlerts = parseFloat({{ MIN_DONATION.rt_alerts|default:0 }}),
+      maxFreeDocketAlerts = {{ MAX_FREE_DOCKET_ALERTS|default:0 }},
+      recapBonusAlerts = {{ DOCKET_ALERT_RECAP_BONUS|default:0 }};


Semgrep identified an issue in your code:

Detected a template variable used in a script tag. Although template variables are HTML escaped, HTML escaping does not always prevent cross-site scripting (XSS) attacks when used directly in JavaScript. If you need this data on the rendered page, consider placing it in the HTML portion (outside of a script tag). Alternatively, use a JavaScript-specific encoder, such as the one available in OWASP ESAPI. For Django, you may also consider using the 'json_script' template tag and retrieving the data in your script by using the element ID (e.g., document.getElementById).

To resolve this comment:

🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.

💬 Ignore this finding

Leave a nosemgrep comment directly above or at the end of line 167 like so // nosemgrep: generic.html-templates.security.var-in-script-tag.var-in-script-tag

Take care to validate that this is not a true positive finding before ignoring it.
Learn more about ignoring code, files and folders here.

_{You can view more details about this finding in the Semgrep AppSec Platform.}

mlissner

This looks pretty good! We will need a way of handling tests though, so that they pass. Maybe an easy way to handle this is to make sure that the flag for the new design is off by default, so that the tests continue running on the old design instead of the new. Then, when we're closer to being done, we can flip the default and make sure they work on the new design.

I think the rest looks good. This will be fun and weird.

mlissner · 2025-01-18T06:47:10Z

Let's get tests passing before merging, but @ERosendo, it might be good to have your review now anyway, just to see if you think the design is a good direction.

I like it, I think, but this is a weird project with a lot of ways to do things, so more perspectives is probably smart.

elisa-a-v · 2025-01-20T21:36:00Z

Maybe an easy way to handle this is to make sure that the flag for the new design is off by default, so that the tests continue running on the old design instead of the new. Then, when we're closer to being done, we can flip the default and make sure they work on the new design.

The tests were indeed creating a flag that is on by default, as per waffle's config (WAFFLE_CREATE_MISSING_FLAGS = True and WAFFLE_FLAG_DEFAULT = env("WAFFLE_FLAG_DEFAULT", default=True)), but that shouldn't have been an issue for the missing templates as Django templates handles that (we're defaulting to the old template if the new one doesn't exist, even when the flag is on).

The issue with the tests was that we already had some templates that started with new_ (new_visualization.html) so the tests were using the wrong templates. I changed the prefix to v2_ instead and also added a check to see if the flag exists before asking waffle if it's active, this way we don't create a new one automatically and default to False.

elisa-a-v · 2025-01-20T21:41:02Z

Oh no I broke the tests again 😮‍💨 I'll see what's going on

elisa-a-v · 2025-01-20T22:14:38Z

The broken tests are those that are counting the queries executed (ApiQueryCountTests), and since we're manually checking the DB for the flag, we now have an additional query every time. I could either patch the test and leave a TODO to clean up after we've completely rolled out the new design, or remove the manual check and just live with the automatic flag creation, what do you think is best here @mlissner ?

ERosendo · 2025-01-23T07:12:00Z

@elisa-a-v I really like the middleware you created! It seems like your approach addresses @mlissner's concerns about testing. However, I have a slight concern regarding the _flag_is_active method. the current implementation introduces an additional database query on each page load, which may have a slight impact on overall system performance

While looking through the Django-waffle documentation, I came across some decorators that might make testing easier. There's one called override_flag that I think could be useful. We could potentially use this to decorate our testing classes from the cases.py file (I believe we typically use these classes instead of Django's built-in testing classes directly).

I think exploring this alternative approach could be beneficial because it might also addresses Mike's concerns about the flag during testing, and it would save us an extra database query when loading pages. What do you think?

elisa-a-v · 2025-01-23T21:38:02Z

Thanks @ERosendo !

You raise an important concern with the overhead introduced by the additional DB query, and now I see it makes no sense to keep that there. In practice, the only issue that the check is preventing is the automatic flag creation with a default True value. We shouldn't have an additional DB query in each and every request just for that!!

Let's revert the last two commits and make sure we create the flag before merging. I see I have enough permissions to do it, shall I create it right away just for superusers and staff? @mlissner

mlissner · 2025-01-23T23:58:17Z

Checking the DB for a query like this is almost instant (the table is tiny and the OS will cache it), so I had accepted that it was going to be something we did. That aside though, I don't see how decorating the tests gets us out of doing the query. Whether or not the flag is true or false, we still have to check it, don't we?

Anyway, I guess my stance is I think whatever we do here is fine to get the tests passing and the waffle in place. If we can avoid the query, great, but I don't see how except to use a setting, and that seems annoying to deal with.

elisa-a-v · 2025-01-24T00:57:39Z

I don't see how decorating the tests gets us out of doing the query.

I don't think it does.

if we can avoid the query, great, but I don't see how except to use a setting

Well, the query is only useful while the flag doesn't exist, because we want to prevent waffle from creating it with a default True value. After that, we don't really need it anymore, and the built-in waffle flag_is_active method is enough.

Note the tests were initially failing not because of a flag that was on, but because of the prefix used that clashed with the preexisting new_visualization.html template that was used as the new version of visualization.html—this caused the tests to fail because these are different templates used in different places, the former is not the replacement for the latter. Changing the prefix from new_ to v2_ fixed this issue (see this commit for which all checks were successful).

The "cleanest" solution code-wise, imo, is reverting the last two commits (the ones that add the DB query and the tests patch), that way we use the plain flag_is_active and the tests are fine. Only thing we need to be careful about is creating the flag before merging.

mlissner · 2025-01-24T01:03:19Z

Sounds good to me. Thanks for explaining again.

elisa-a-v · 2025-01-24T01:29:17Z

Cool. Do we have a preferred way to undo commits? I kinda like git reset better than git revert but I don't want to push --force without checking first.

mlissner · 2025-01-24T01:31:15Z

Some around here have strong opinions. I'm happy with things that get it done. I'd probably reset and force, personally. :)

mlissner · 2025-01-26T08:09:57Z

I'm going for it! Let's see how this goes and if needed, I can revert. Looks like the waffle is in place, so I think we should be good to go.

mlissner · 2025-01-26T08:28:14Z

Total success: https://bsky.app/profile/michaeljaylissner.com/post/3lgmyvtejis2a

🚀

elisa-a-v added 3 commits January 17, 2025 21:09

feat(middleware): introduce middleware to use new templates

db1c413

The middleware checks a waffle flag to gradually roll out the new design when a requested view has a new template available.

feat(templates): add new_base.html for new design

590628e

- This file will use tailwind instead of bootstrap once tailwind support has been implemented. - Templates with new design should extend from new_base.html instead of the old one.

feat(templates): add new template for help index

d216796

New template extends from new_base.html and will be updated with the new design in #4740. It's added now as an MVP for #4815.

elisa-a-v requested a review from mlissner January 18, 2025 01:41

elisa-a-v assigned mlissner and unassigned mlissner Jan 18, 2025

semgrep-app bot reviewed Jan 18, 2025

View reviewed changes

mlissner reviewed Jan 18, 2025

View reviewed changes

mlissner assigned ERosendo Jan 18, 2025

elisa-a-v added 2 commits January 20, 2025 17:31

Merge branch 'main' into 4815-new-base.html

4c527a0

fix(middleware): change prefix to avoid conflict with old templates

7aa59be

elisa-a-v marked this pull request as ready for review January 20, 2025 21:13

elisa-a-v requested a review from ERosendo January 22, 2025 15:08

ERosendo assigned elisa-a-v and unassigned ERosendo Jan 23, 2025

elisa-a-v assigned mlissner and unassigned elisa-a-v Jan 23, 2025

elisa-a-v mentioned this pull request Jan 24, 2025

Flaky Selenium Tests in Disclosure module #4958

Open

mlissner assigned elisa-a-v and unassigned mlissner Jan 24, 2025

Merge branch 'main' into 4815-new-base.html

414380a

elisa-a-v force-pushed the 4815-new-base.html branch from 55fea10 to 414380a Compare January 24, 2025 15:54

elisa-a-v requested a review from mlissner January 24, 2025 16:39

elisa-a-v assigned mlissner and unassigned elisa-a-v Jan 24, 2025

mlissner merged commit cdbf894 into main Jan 26, 2025
15 checks passed

mlissner deleted the 4815-new-base.html branch January 26, 2025 08:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(redesign): introduce new_base.html with waffle flag #4940

feat(redesign): introduce new_base.html with waffle flag #4940

elisa-a-v commented Jan 18, 2025 •

edited

Loading

semgrep-app bot Jan 18, 2025

semgrep-app bot Jan 18, 2025

semgrep-app bot Jan 18, 2025

semgrep-app bot Jan 18, 2025

semgrep-app bot Jan 18, 2025

mlissner left a comment

mlissner commented Jan 18, 2025

elisa-a-v commented Jan 20, 2025

elisa-a-v commented Jan 20, 2025

elisa-a-v commented Jan 20, 2025

ERosendo commented Jan 23, 2025

elisa-a-v commented Jan 23, 2025 •

edited

Loading

mlissner commented Jan 23, 2025

elisa-a-v commented Jan 24, 2025

mlissner commented Jan 24, 2025

elisa-a-v commented Jan 24, 2025

mlissner commented Jan 24, 2025

mlissner commented Jan 26, 2025

mlissner commented Jan 26, 2025

feat(redesign): introduce new_base.html with waffle flag #4940

feat(redesign): introduce new_base.html with waffle flag #4940

Conversation

elisa-a-v commented Jan 18, 2025 • edited Loading

semgrep-app bot Jan 18, 2025

Choose a reason for hiding this comment

semgrep-app bot Jan 18, 2025

Choose a reason for hiding this comment

semgrep-app bot Jan 18, 2025

Choose a reason for hiding this comment

semgrep-app bot Jan 18, 2025

Choose a reason for hiding this comment

semgrep-app bot Jan 18, 2025

Choose a reason for hiding this comment

mlissner left a comment

Choose a reason for hiding this comment

mlissner commented Jan 18, 2025

elisa-a-v commented Jan 20, 2025

elisa-a-v commented Jan 20, 2025

elisa-a-v commented Jan 20, 2025

ERosendo commented Jan 23, 2025

elisa-a-v commented Jan 23, 2025 • edited Loading

mlissner commented Jan 23, 2025

elisa-a-v commented Jan 24, 2025

mlissner commented Jan 24, 2025

elisa-a-v commented Jan 24, 2025

mlissner commented Jan 24, 2025

mlissner commented Jan 26, 2025

mlissner commented Jan 26, 2025

elisa-a-v commented Jan 18, 2025 •

edited

Loading

elisa-a-v commented Jan 23, 2025 •

edited

Loading