geneontology / amigo

AmiGO is the public interface for the Gene Ontology.
http://amigo.geneontology.org
BSD 3-Clause "New" or "Revised" License
29 stars 17 forks source link

In bookmarked searches with spaces, they are rendered as %20, preventing the search from running properly #585

Closed kltm closed 4 years ago

kltm commented 4 years ago

In bookmarked searches with spaces, they are rendered as %20, preventing the search from running properly. For example:

http://amigo.geneontology.org/amigo/search/ontology?q=neural%20crest*&fq=idspace:%22GO%22&fq=is_obsolete:%22false%22&sfq=document_category:%22ontology_class%22

This is not actually an error in the search--search works fine if you just type it in--it is an issue in the bookmarking and how URLs are decoded (or further rendered internally). It is likely the result of a hasty patch in order to pass an LBL security review:

https://github.com/geneontology/amigo/issues/497

Also related: https://github.com/geneontology/amigo/issues/575

Either the spaces need to be passed (or they need to be re-rendered as spaces internally, less good).

kltm commented 4 years ago

Reported by @elserj

elserj commented 4 years ago

@cmungall @jaiswalp @cooperl09

Per the previous discussion over email with @kltm, this was deemed "not terribly pressing for the moment", but after discussing it with the group, we'd like to see if this could be fixed before we do another release in about a month.

I don't know about your users, but I think ours do use the links from the medial_search rather than the dropdown from the quick search and so this might be a higher priority for use than you.

I can still try to fix it myself, but wanted to rope in everyone just to make sure we are on the same page, and if you guys can fix it before I can, great!

kltm commented 4 years ago

@elserj While a definite nasty little "affects usability" bug, this has not been reported by our users so far so has not been prioritized. While I'm currently under a fair pile at the moment, I can probably take a look at how things stand next week. Fortunately, the fix is just likely to be a line or two; as well, we have the diff to see how it works https://github.com/geneontology/amigo/commit/005e713e7843f94b5cd6cbe387f60dd72c7f71a5 (too bad that 90% of that rather clear diff is trailing space removal). My guess is that either adding an option to skip space or decoding space after the sanitizer for the bookmark and query would fix it handily. It you want to take a pass at it first I can check. Otherwise, me acting on it would take a little patience, but hopefully not too long.

elserj commented 4 years ago

That sounds good. I wasn't expecting you to drop everything to work on this, but we were hoping it could be fixed before we do another release, expected in mid-late February.

I will try to work on it, and will let you know if I have a solution.

cooperl09 commented 4 years ago

Any update on getting this resolved?

kltm commented 4 years ago

Nothing at my end yet--I've been pinned down recently.

elserj commented 4 years ago

@kltm, I think I've found a solution, can you please check it over and let me know if this will break something else I'm not thinking of: Add the following as line #1129 to perl/lib/AmiGO/WebApp/HTMLClient.pm $params->{q} =~ tr/ /+/;

This seems to fix it on my dev site. Sorry I can't send a proper patch, for some reason c/p is broken from the terminal on my machine.

kltm commented 4 years ago

@elserj Cool! Have you been running this locally or publicly yet?

cooperl09 commented 4 years ago

@kltm It appears to be working on our development site. You can test it here: http://dev.planteome.org/amigo/search/ontology

kltm commented 4 years ago

@elserj @cooperl09 Very nice! I've been unable to break it so far and seems to be nothing but an improvement. My only feedback is here: https://github.com/elserj/amigo/commit/f53e3ed8a8c16750686b33b34e6850204f38f602#commitcomment-37733590