Skip to content

[test ui] Encode URLs in free response#9632

Merged
mehalshah merged 2 commits into
stagingfrom
encodeUrl
Jul 21, 2016
Merged

[test ui] Encode URLs in free response#9632
mehalshah merged 2 commits into
stagingfrom
encodeUrl

Conversation

@mehalshah

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread apps/src/code-studio/reporting.js Outdated
}
var queryString = queryItems.join('&');

var queryString = encodeURI(queryItems.join('&'));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you want to encode each item in queryItems before joining? I believe we should use encodeURIComponent instead.

@joshlory

Copy link
Copy Markdown
Contributor

A heads up, the UI test you modified isn't run by CircleCI:

UI tests for ChromeLatestWin7_free_response_submittable skipped (0:15 minutes)
We didn't actually run any tests, did you mean to do this?

@mehalshah

Copy link
Copy Markdown
Contributor Author

Encoded the items individually, and with encodeURIComponent

Ran the UI test locally (it passed).

Comment thread apps/src/code-studio/reporting.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Whoops - fixed

@joshlory

Copy link
Copy Markdown
Contributor

LGTM. Long term it would be good to figure out this issue so we're not building the form data by hand:

// jQuery can do this implicitly, but when url-encoding it, jQuery calls a method that
// shows the result dialog immediately

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