Fix some mako-missing-default xsslint issues in test files. - #16709
Conversation
|
Thanks for the pull request, @UmanShahzad! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request. |
|
jenkins run a11y |
|
jenkins run bokchoy |
|
jenkins run lettuce |
|
jenkins run quality |
|
Some issues with Jenkins atm... |
|
@UmanShahzad yeah, issues with jenkins. :( |
|
@jzoldak Quality tests are failing but I brought xsslint violations down by almost 20. I'm guessing it's because of other xsslint issues in the same files I'm editing? If so could we just merge it to fix quality builds on |
|
@jzoldak If you're cool with the above ^ please merge this when you see it. Otherwise I'll have to undo changes in some files to prevent this particular PR's build from failing. |
| <%page expression_filter="h"/> | ||
| <%inherit file="dashboard.html" /> | ||
| <%block name="pagetitle">Overridden Title!</%block> | ||
| ${parent.body()} |
There was a problem hiding this comment.
@jzoldak: I'd be surprised if HTML-escaping wouldn't need to be turned off for ${parent.body()}. I didn't review all these files individually, but just a note that some care needs to go into adding default HTML-escaping, except of course on empty templates like some of these.
Additionally, we should really bring down the threshold on this rule once the dust settles, or people may introduce new templates without this page expression, which is what we want to protect against (i.e. new issues).
There was a problem hiding this comment.
I understand. In this case this is just a test file, so it should be OK, but we should definitely be following best practices moving forward given this situation we ran into.
There was a problem hiding this comment.
Thanks @UmanShahzad. A couple of additional details:
- Although it is a test file, you never know who is going to copy and paste code from where, so it would be good if the file was accurate.
- When we introduce HTML-escaping by default, it affects the whole template and can cause new linter errors. In this case, it did, as @efischer19 points out in this comment:
https://github.com/edx/edx-platform/pull/16723#issuecomment-348235004
He is upping the thresholds in the following PR, but it would have been nice to not have added new issues:
https://github.com/edx/edx-platform/pull/16723
There was a problem hiding this comment.
FYI: You can see the violations for all files you touched here:
https://build.testeng.edx.org/job/edx-platform-quality-pr/43752/artifact/edx-platform/reports/xsscommitlint/xsscommitlint.report/*view*/
Some examples are:
Linting common/test/test_sites/test_site/templates/courseware/tabs.html:
common/test/test_sites/test_site/templates/courseware/tabs.html: 21:12: mako-unwanted-html-filter: <a href="${tab.link_func(course, reverse) | h}" class="${tab_class}">
common/test/test_sites/test_site/templates/courseware/tabs.html: 22:18: mako-unwanted-html-filter: Test Site Tab: ${_(tab.name) | h}
As a separate discussion, we could debate whether mako-unwanted-html-filter should break the build or not since strictly speaking it isn't a problem. The problem is simply that if it isn't cleaned up by removing the | h, it makes it difficult to know what files are safe by default and which aren't, and which expressions are safe and which aren't.
@efischer19: Maybe you can just fix these on your PR, rather than upping the threshold?
There was a problem hiding this comment.
Just in case this isn't fixed as part of @efischer19's PR, the other failure that could be cleaned up is:
Linting common/test/test_sites/test_site/templates/static_templates/contact.html:
common/test/test_sites/test_site/templates/static_templates/contact.html: 25:14: mako-html-entities: <p>${_("We are always seeking feedback to improve our courses. If you are an enrolled student and have any questions, feedback, suggestions, or any other issues specific to a particular class, please post on the discussion forums of that class.")}</p>
We could then adjust the thresholds back down.
There was a problem hiding this comment.
I think the problem is I had no clue I was introducing these violations with this PR. You can tell at a glance for things like pylint or pep8, but it doesn't seem obvious at all (to me and maybe others) with xsslint reporting atm. Reporting should be improved to show the specific errors a particular PR introduces and nothing more, to keep it simple and prevent something like this.
There was a problem hiding this comment.
@UmanShahzad: Agreed. It used to tell you which thresholds were breaking, but now that is hard to see. See this comment and following comments for a discussion of this:
https://github.com/edx/edx-platform/pull/16719#issuecomment-348209133
FYI: @jzoldak
There was a problem hiding this comment.
I agree. It seems like @UmanShahzad got "hit" by this because he touched a file to improve it, then the tooling started reporting all the previously un-reported errors that existed in that file. It feels like we're punishing good intentions to some degree.
There was a problem hiding this comment.
@efischer19: It's a little different from the way you describe it. It's not that you get hit by tooling for touching a file. It is that when you add HTML-escaping by default to the top of a file it affects all expressions in that template, and sometimes new problems appear. So, some care is needed when adding HTML-escaping by default. You can't just drop it in to any template that doesn't have it without this care, or we would have done that to get rid of these violations.
There was a problem hiding this comment.
Ah, okay - that does make more sense. Thanks!
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Monday, December 04, 2017. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
7 participants
Motivated by https://github.com/edx/edx-platform/pull/16708.