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

Fix gh-2160: Search with % #2161

Merged
merged 3 commits into from Oct 15, 2018

Conversation

@chen-robert
Copy link
Contributor

commented Oct 9, 2018

Resolves #2160

Changes:

Because query terms only sometimes need to be decoded, we attempt URI decoding and silence the error if it fails. This lets us still search with queries like cat%25cat when we want to, but now falls back to no-decoding when an invalid string is presented (e.g. 100%pen).

@chen-robert chen-robert changed the title Gh2160 search with percent Fix gh-2160: Search with % Oct 9, 2018

@benjiwheeler benjiwheeler self-requested a review Oct 9, 2018

term = decodeURIComponent(term.split('+').join(' '));
try {
term = decodeURIComponent(term);
} catch (e){

This comment has been minimized.

Copy link
@benjiwheeler

benjiwheeler Oct 9, 2018

Contributor

i know this is a tiny quibble, but could you put a space before the { ?

This comment has been minimized.

Copy link
@chen-robert

chen-robert Oct 10, 2018

Author Contributor

I've fixed it. This didn't show any errors in my ESLint though?

@@ -78,7 +78,12 @@ class Search extends React.Component {
while (term.indexOf('&') > -1) {
term = term.substring(0, term.indexOf('&'));
}
term = decodeURIComponent(term.split('+').join(' '));
try {

This comment has been minimized.

Copy link
@benjiwheeler

benjiwheeler Oct 9, 2018

Contributor

Why was it safe to take out the .split('+').join(' ') ? Because that's part of decoding uri's anyway? Do we know why it was there? Would be good to test a few strings and confirm they decode as we expect

This comment has been minimized.

Copy link
@chen-robert

chen-robert Oct 10, 2018

Author Contributor

I believe it is because we have already decoded the URI. Regardless, including that piece of code introduces a bug when we try searching with +. For example, cat+dog gets translated to cat dog.

@benjiwheeler

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2018

This looks good, thanks for identifying this case that we weren't handling!

@chen-robert

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2018

@benjiwheeler I've fixed the issues. Could you please review the pr again?

@benjiwheeler

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2018

Will do!

FYI, one piece of our workflow (I just joined the team recently, this is new to me and was not obvious) is that we use github's "assignee" and "labels" info to tell each other "I updated, can you take a look" or "this needs revision". So after you updated, you could assign me, take off the "needs work" label, and put on the "needs review" label.

@chen-robert

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2018

I don't think I have permission to edit the labels or assignees, sorry about that. I'm not part of the Scratch team - I just contribute to this repos in my spare time.

@thisandagain thisandagain added this to the October 2018 milestone Oct 12, 2018

@benjiwheeler

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2018

So I tested this out. It's a big improvement for some searches, like your example of "100%pen" (or even "100% pen" -- both crash the page right now, but not with your improvement).

I'm not sure we should stop parsing the + symbol, but I agree with you that parsing it into a space introduces a bug, and from my testing, there's no search that would be prevented if we take that parsing out -- you can always use %20 for a space in the querystring. I did a quick search for existing search links in the wild, by googling site:https://scratch.mit.edu/search/projects, and didn't find any that use a +. I agree that we should take it out.

Before approving, however, I just want to run this by @thisandagain .

@thisandagain

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

This seems like an ok change to me. Thanks @chen-robert

@benjiwheeler

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

Awesome! I'm merging it! Thanks, @chen-robert !

@benjiwheeler benjiwheeler merged commit 24d07f6 into LLK:develop Oct 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@paulkaplan paulkaplan added the needs-qa label Nov 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.