Safe Template Errors Fixes #12741

kevinjkim · 2016-06-13T21:22:10Z

Description

TNL-4796

Changes to fix the safe template errors in the user account settings/preferences page.

Sandbox

Build a sandbox for your branch and add a link here
https://kevinjkim.sandbox.edx.org/

Testing

safecommit shows 0 violations
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: @jcdyer
Code review: @efischer19

Devops assistance

Don't think I need this

FYI: @robrap

Post-review

Squash commits

robrap · 2016-06-15T13:01:46Z

lms/static/js/student_account/views/account_settings_fields.js

@@ -41,10 +41,11 @@
            EmailFieldView: FieldViews.TextFieldView.extend({
                fieldTemplate: field_text_account_template,
                successMessage: function () {
-                    return this.indicators.success + StringUtils.interpolate(
+                    return HtmlUtils.joinHtml(
+                        this.indicators.success, StringUtils.interpolate(


I would rename all the indicators from indicators.success to indicators.successHtml.
Also, could you put StringUtils.interpolate() param on the next line, indented with the first parameter?
Note: I cleaned up these comments once I reviewed the indicators.

this.indicators.success does contain HTML. It didn't give any errors in the safe template run, but once I started to change other things around, this didn't work as is.

robrap · 2016-06-15T13:45:27Z

@kevinjkim Nice start. Thanks. I made a first pass.

robrap · 2016-06-15T18:22:48Z

lms/static/js/views/fields.js

            },

            showCanEditMessage: function(show) {
                if (!_.isUndefined(show) && show) {
-                    this.showNotificationMessage(this.getMessage('canEdit'));
+                    this.showNotificationMessage(this.getMessageHTML('canEdit'));


I don't know if I'd rename getMessage to getMessageHTML without renaming all the others. I don't like the idea of having 2 methods, getMessage and getMessageHTML.

If we did keep getMessageHTML, it should be getMessageHtml.

kevinjkim · 2016-06-15T18:25:27Z

jenkins run js

robrap · 2016-06-15T18:29:12Z

lms/static/js/views/fields.js

            },

            showSuccessMessage: function () {
                var successMessage = this.getMessage('success');
-                this.showNotificationMessage(successMessage);
+                this.showNotificationMessage(this.getMessageHTML('success'));


Why did this function need to change? As is, the variable successMessage is set and then unused. Maybe you should just change the earlier line to the following, if you just wanted the name change:

var successMessage = this.getMessageHtml('success');

andy-armstrong · 2016-06-17T15:48:35Z

@kevinjkim I don't know how you would feel about this, but I'm wondering if you could take over my PR too since it is resolving a lot of the same issues. I never seem to have time to get back to it:

https://github.com/edx/edx-platform/pull/11856

jcdyer · 2016-06-20T14:07:43Z

@andy-armstrong We might be able to get @kevinjkim working on that issue, but we need to get the time zone work merged first. It might be another sprint before we can get him started on it.

robrap · 2016-06-20T21:43:05Z

lms/static/js/views/fields.js

@@ -116,8 +141,8 @@
            },

            showSuccessMessage: function () {
-                var successMessage = this.getMessage('success');
-                this.showNotificationMessage(successMessage);
+                var successMessage = this.getMessage('success').toString();


Should this line be deleted?

+1, why are we calling getMessage('success') twice in succession here? Either remove this line, or use the variable in the following line.

Currently, this.getMessage() returns a HtmlSnippet while view.getNotificationMessage() returns a string. Additionally, showNotificationMessage() takes a HtmlSnippet which is why line 145 has the repeated getMessage() call since line 144 has the toString() call.

I could have both getMessage() and getNotificationMessage() return a string, but then showNotificationMessage() would have to change all strings to HtmlSnippets.

I could have both getMessage() and getNotificationMessage() return a HtmlSnippet, but the functions sound like they should return strings instead of HTML code.

Which would you guys prefer?

Ah, I see. They should definitely be standardized to be the same thing I think. Is there actual html content in the return value of getMessage()? If so, HtmlSnippets would be best, but if there's no html to be seen, strings would be easier.

getMessage() does have html so i'll go with HtmlSnippets.

efischer19 · 2016-06-21T13:18:40Z

lms/static/js/student_account/views/account_settings_fields.js

+                                HtmlUtils.joinHtml(
+                                    view.indicators.error,
+                                    gettext('You must sign out and sign back in ' +
+                                        'before your language changes take effect.')


This sentence needs to remain all on one line to ensure correct translation. If that's causing line-too-long quality failures, you can make an exception in jshint like this: https://github.com/edx/edx-platform/blob/master/lms/static/js/certificates/views/certificate_invalidation_view.js#L49

efischer19 · 2016-06-21T13:24:03Z

Looking good so far. Is there a sandbox up? The biggest thing I've found with these types of PRs is manual confirmation of the affected pages after fixing linter errors.

kevinjkim · 2016-06-21T14:09:58Z

No sandbox yet. I'll make one after we decide what to do with the getMessage/getNotificationMessage thing.

kevinjkim · 2016-06-21T18:19:56Z

sandbox up

efischer19 · 2016-06-22T14:13:11Z

Sandbox looks good, I was able to click around all the affected pages and everything seemed to load fine, no js errors.

👍 , assuming the coveralls failure goes away after squash/rebasing. LMK if you need a hand with that.

jcdyer · 2016-06-22T15:05:55Z

👍 Don't forget to squash.

@efischer19 This branch is not merging to master, but back to the main feature branch, which has its own PR. I think the coverage issue can be resolved on that branch.

efischer19 · 2016-06-22T15:07:17Z

Right on, sounds like a plan

kevinjkim force-pushed the kkim/safe_template branch 2 times, most recently from 2d0abfd to 3bcf3c7 Compare June 14, 2016 18:51

robrap reviewed Jun 15, 2016
View reviewed changes

kevinjkim force-pushed the kkim/safe_template branch 2 times, most recently from 94c6df8 to 7fadff7 Compare June 15, 2016 18:14

robrap reviewed Jun 15, 2016
View reviewed changes

robrap reviewed Jun 20, 2016
View reviewed changes

efischer19 reviewed Jun 21, 2016
View reviewed changes

Fixes safe template errors on Account Settings and Account Profile page.

ed9e1eb

kevinjkim force-pushed the kkim/safe_template branch from 6eea7b6 to ed9e1eb Compare June 22, 2016 16:17

kevinjkim merged this pull request into kkim/tz_pref_AS Jun 22, 2016

kevinjkim deleted the kkim/safe_template branch June 22, 2016 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Safe Template Errors Fixes #12741

Safe Template Errors Fixes #12741

kevinjkim commented Jun 13, 2016 •

edited

robrap Jun 15, 2016 •

edited

kevinjkim Jun 15, 2016

robrap commented Jun 15, 2016

robrap Jun 15, 2016

kevinjkim commented Jun 15, 2016

robrap Jun 15, 2016

andy-armstrong commented Jun 17, 2016

jcdyer commented Jun 20, 2016

robrap Jun 20, 2016

efischer19 Jun 21, 2016

kevinjkim Jun 21, 2016

efischer19 Jun 21, 2016

kevinjkim Jun 21, 2016

efischer19 Jun 21, 2016

efischer19 commented Jun 21, 2016

kevinjkim commented Jun 21, 2016

kevinjkim commented Jun 21, 2016

efischer19 commented Jun 22, 2016

jcdyer commented Jun 22, 2016

efischer19 commented Jun 22, 2016

Safe Template Errors Fixes #12741

Safe Template Errors Fixes #12741

Conversation

kevinjkim commented Jun 13, 2016 • edited

Description

Sandbox

Testing

Reviewers

Devops assistance

Post-review

robrap Jun 15, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robrap commented Jun 15, 2016

Choose a reason for hiding this comment

kevinjkim commented Jun 15, 2016

Choose a reason for hiding this comment

andy-armstrong commented Jun 17, 2016

jcdyer commented Jun 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efischer19 commented Jun 21, 2016

kevinjkim commented Jun 21, 2016

kevinjkim commented Jun 21, 2016

efischer19 commented Jun 22, 2016

jcdyer commented Jun 22, 2016

efischer19 commented Jun 22, 2016

kevinjkim commented Jun 13, 2016 •

edited

robrap Jun 15, 2016 •

edited