Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Studio homepage escaping #11551

Merged
merged 1 commit into from
Feb 18, 2016
Merged

Studio homepage escaping #11551

merged 1 commit into from
Feb 18, 2016

Conversation

mushtaqak
Copy link
Contributor

TNL-4006

Make the entire template safe by default.

@robrap Please have a review.

@mushtaqak
Copy link
Contributor Author

@robrap Does this need a test ? I think not, because test for checking template is safe would have been previously written.

@mushtaqak
Copy link
Contributor Author

@awaisdar001 @adampalay Please a review

@adampalay
Copy link
Contributor

👍

@robrap
Copy link
Contributor

robrap commented Feb 16, 2016

@mushtaqak The following are the types of changes to look out for as you switch to safe by default:

  1. Remove | h in lines like this. Note that Mako shouldn't double escape, but this clean-up will avoid causing confusion regarding why some things are escaped and others are not.
  2. Lines like this need to wrap the HTML with the HTML() method from djangolib.markup as documented in the i18n read the docs page.
  3. Lines like this should look more like number 2 above, where there is a single call to _() and the HTML is injected as documented in read the docs with the HTML() method.

@mushtaqak
Copy link
Contributor Author

That is great Robert. I will implement these when I come tomorrow morning :-)

@mushtaqak
Copy link
Contributor Author

@robrap May you please see

@@ -1,4 +1,8 @@
<%! from django.utils.translation import ugettext as _ %>
<%!
from openedx.core.djangolib.markup import ugettext as _
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Mostly we have been doing this on one line:

from openedx.core.djangolib.markup import HTML, ugettext as _

@robrap
Copy link
Contributor

robrap commented Feb 17, 2016

@mushtaqak Getting closer. Please let me know when the final changes are done and when the checks are all passing.

One last issue is if you want to search for ) % and adjust the 2 matches to use the format() syntax, it would be nice to have consistency. Thanks!

@mushtaqak
Copy link
Contributor Author

@robrap all checks passed, one final pass for review :)

link_start='<a href="#" class="action-reload">',
link_end='</a>',
)}</p>
link_start=HTML('<a href="#" class="action-reload">'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Other code had a 4 space tab.

@robrap
Copy link
Contributor

robrap commented Feb 18, 2016

👍 It's up to you if you want to fix the minor indent issue. I don't need to re-review. Thank you.

mushtaqak pushed a commit that referenced this pull request Feb 18, 2016
@mushtaqak mushtaqak merged commit 689bb73 into master Feb 18, 2016
@mushtaqak mushtaqak deleted the mushtaq/fix-tnl4006 branch February 18, 2016 17:52
@robrap robrap mentioned this pull request Feb 19, 2016
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants