Add UI Toolkit to the platform #11799

andy-armstrong · 2016-03-12T19:01:06Z

https://openedx.atlassian.net/browse/UITK-75

openedx-webhooks · 2016-03-12T19:01:09Z

PR Cover Letter

This adds the UI Toolkit to edx-platform, and then starts to convert the platform to use the new HtmlUtils class. I've primarily converted the Teams UI, along with the shared helper classes and Underscore templates that they depend upon.

In addition, the static path for the Pattern Library has been made /static/edx-pattern-library instead of just /static/pattern-library. This change was already made in the Pattern Library so it was made here too for consistency.

Note that this PR can't merge until Underscore.js has been upgraded by https://github.com/edx/edx-platform/pull/11631.

Sandbox

https://andy-armstrong.sandbox.edx.org/

Testing

Unit, integration, acceptance tests as appropriate

Reviewers

If you've been tagged for review, please check your corresponding box once you've given the 👍.

Code review Put back in the blocks for Advanced Settings. #1: @cahrens
Code review Fix js that prevented single step OE problems (ie only self assessment) ... #2: @robrap

FYI: @AlasdairSwan @dan-f @dsjen @bjacobel

Post-review

Squash commits

TNL-4243

Properly escape mixed content. text + html.

…t-shown render progress status while problem content rendering

The fork actually does include a commit that is not in the PyPI version. ECOM-3833

Reverted rfc6266 change

…gistration." This reverts commit 178f5a6.

Revert ECOM-2947.

Release Candidate rc/2016-03-16

Merge release back to master.

This will leverage the nose attrib plugin to allow us to break tests apart on our build system to run in parallel on separate nodes.

robrap · 2016-03-18T18:43:53Z

lms/static/js/views/fields.js

-                'inProgress': '<i class="fa fa-spinner fa-pulse message-in-progress" aria-hidden="true"></i><span class="sr">' + gettext("In Progress") + '</span>',
-                'success': '<i class="fa fa-check message-success" aria-hidden="true"></i><span class="sr">' + gettext("Success") + '</span>',
-                'plus': '<i class="fa fa-plus placeholder" aria-hidden="true"></i><span class="sr">' + gettext("Placeholder")+ '</span>'
+                'canEdit': HtmlUtils.HTML(


interpolateHtml so the getText() is escaped.

Rename canEdit to canEditHtml if possible, or move to a template.

andy-armstrong · 2016-03-18T19:08:26Z

Thanks for the thorough (and ongoing) review, @robrap and @cahrens.

One theme that keeps coming up is why I used HtmlUtils.interpolateHtml instead of StringUtils.interpolate for strings that don't have HTML. My thinking at the time was that the value was being passed to a field that accepts HTML, so it seemed logical to provide it as such. We have also discussed renaming all such fields to fooHtml to make it clearer that that the field supports HTML.

Reflecting on it more, we are saying that HTML fields will always allow raw strings to be passed, because the best practice says that templates will use a helper that will escape raw strings while passing HTML snippets through. Given this, I agree with you both that I should use StringUtils.interpolate whenever possible so that I am not escaping too early. It is up to each template to be responsible for escaping its values appropriately.

The naming convention of fooHtml should indicate that a template allows HTML to be passed in addition to raw strings. A client of a template shouldn't pass HTML snippets to a field that is not named fooHtml because the code could be using <%- to do its escaping and hence would double escape the HTML. This argues for being very strict about the naming convention, which I'm happy to go with if that's what folks think, but it would be an even more onerous change. If we don't do this, then developers can risk passing HTML snippets to fields that aren't named fooHtml, but then it is their responsibility to write tests that verify that the result isn't double escaped.

I'll make a second pass through this PR once you guys complete your reviews. Thanks.

robrap · 2016-03-18T19:32:29Z

lms/static/js/views/image_field.js

@@ -183,13 +184,13 @@

            showUploadInProgressMessage: function () {
                this.$('.u-field-upload-button').css('opacity', 1);
-                this.$('.upload-button-icon').html(this.iconProgress);
+                this.$('.upload-button-icon').html(this.iconProgressHtml.toString());


Throwing out some ideas here:

this.$('.upload-button-icon').html(HtmlUtils.forJQuery(this.iconProgressHtml));

or

this.$('.upload-button-icon').html(HtmlUtils.for$(this.iconProgressHtml));

Again, these are just other alternative names for escape(), but maybe you'll like them more and we can have something consistent for the linter. For example, this could still work with interpolateHtml() as such...

this.$('.upload-button-icon').html(HtmlUtils.for$(HtmlUtils.interpolateHtml(...)));

Another possibility is:

this.$('.upload-button-icon').html(HtmlUtils.$html(this.iconProgressHtml));

Does this mean that the toString method does some sort of escaping? I'm confused.

robrap · 2016-03-18T19:54:10Z

@andy-armstrong I doubt we will ever be able to be strict about naming, because the linting rules would get too complex, but we should have best practices. It sounds like you are proposing we use the name titleHtml for a variable the could be either HTML or plain text. I think that's fair. Again, I'm not sure that we'll be able to be strict about it.

Upgrade Underscore.js and Underscore.string.js

cahrens · 2016-03-18T20:44:19Z

@andy-armstrong I'm done with my pass, but there are still some pretty fundamental things that I don't understand about when/how to escape HTML (that should be HTML). I think I will need some intervention on Monday.

robrap · 2016-03-18T20:54:17Z

@andy-armstrong @cahrens @nedbat I put together a Confluence page because I wanted to collect my thoughts. We can discuss there as well, and adjust the page as necessary:
https://openedx.atlassian.net/wiki/display/TNL/HtmlUtils+Helpers+for+JavaScript

UITK-75

andy-armstrong · 2016-03-20T22:06:08Z

Closing in favor of a new PR against master.

andy-armstrong force-pushed the andya/add-ui-toolkit branch 4 times, most recently from 5ce548c to 1028dd1 Compare March 14, 2016 04:27

muzaffaryousaf and others added 11 commits March 14, 2016 19:52

Properly escape mixed content. text + html.

d43f33d

TNL-4243

Merge pull request #11802 from edx/mzfr/notes-bug

415c0ea

Properly escape mixed content. text + html.

render progress status while problem content rendering

167efea

Merge pull request #11811 from edx/mrehan/tnl-4219-progress-detail-no…

31cd7af

…t-shown render progress status while problem content rendering

Clear out usages of deprecated vendor prefixes.

e2b8df7

Reverted rfc6266 change

ae0a128

The fork actually does include a commit that is not in the PyPI version. ECOM-3833

Merge pull request #11814 from edx/clintonb/req-fix

aba4e58

Reverted rfc6266 change

Update problem-builder to version 2.0.3

2a5fce9

Revert "Fixed the other half of forgot password flow accourding to lo…

a32ee42

…gistration." This reverts commit 178f5a6.

Merge pull request #11823 from edx/releng/revert-ecom-2947

cac35ee

Revert ECOM-2947.

Merge pull request #11793 from edx/rc/2016-03-16

3d95bd5

Release Candidate rc/2016-03-16

andy-armstrong mentioned this pull request Mar 16, 2016

Improve HTML snippet handling for edx-platform openedx/edx-ui-toolkit#34

Merged

andy-armstrong force-pushed the andya/add-ui-toolkit branch 2 times, most recently from 580f4ec to 8324a75 Compare March 16, 2016 21:06

andy-armstrong force-pushed the fedx/upgrade-libraries branch from 7c67d79 to 00d6963 Compare March 16, 2016 21:09

andy-armstrong force-pushed the andya/add-ui-toolkit branch 2 times, most recently from f2278a3 to b6da5c0 Compare March 16, 2016 23:39

tobz and others added 8 commits March 16, 2016 21:11

Merge pull request #11824 from edx/release

9b8ff77

Merge release back to master.

Removed duplicate lettuce test from data_download.feature

79c5a79

Removed duplicate lettuce test from course-export.feature

079742d

Removed duplicate luttuce test from courses.feature

8323811

Update translations (autogenerated message)

6883f21

Removed duplicate lettuce test from component.feature

cb0bb1f

Removed annotatable.feature and events.feature and related py files

792bca8

Create a third shard for lms unit tests.

c504029

This will leverage the nose attrib plugin to allow us to break tests apart on our build system to run in parallel on separate nodes.

robrap reviewed Mar 18, 2016
View reviewed changes

Merge pull request #11631 from edx/fedx/upgrade-libraries

76b8e2e

Upgrade Underscore.js and Underscore.string.js

Add UI Toolkit to the platform

fd2421e

UITK-75

andy-armstrong force-pushed the andya/add-ui-toolkit branch from dae215a to fd2421e Compare March 20, 2016 22:05

andy-armstrong closed this Mar 20, 2016

andy-armstrong mentioned this pull request Mar 20, 2016

Start using the HtmlUtils safe template functions #11856

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UI Toolkit to the platform #11799

Add UI Toolkit to the platform #11799

andy-armstrong commented Mar 12, 2016

openedx-webhooks commented Mar 12, 2016

robrap Mar 18, 2016

andy-armstrong commented Mar 18, 2016

robrap Mar 18, 2016

robrap Mar 18, 2016

cahrens Mar 18, 2016

robrap commented Mar 18, 2016

cahrens commented Mar 18, 2016

robrap commented Mar 18, 2016

andy-armstrong commented Mar 20, 2016

Add UI Toolkit to the platform #11799

Add UI Toolkit to the platform #11799

Conversation

andy-armstrong commented Mar 12, 2016

openedx-webhooks commented Mar 12, 2016

PR Cover Letter

Sandbox

Testing

Reviewers

Post-review

robrap Mar 18, 2016

Choose a reason for hiding this comment

andy-armstrong commented Mar 18, 2016

robrap Mar 18, 2016

Choose a reason for hiding this comment

robrap Mar 18, 2016

Choose a reason for hiding this comment

cahrens Mar 18, 2016

Choose a reason for hiding this comment

robrap commented Mar 18, 2016

cahrens commented Mar 18, 2016

robrap commented Mar 18, 2016

andy-armstrong commented Mar 20, 2016