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

Redirect to author page if query excacty pauseid #1258

Conversation

oiami
Copy link
Contributor

@oiami oiami commented Jul 26, 2014

I think this is useful and make sense feature when user search with the PAUSE ID and click on feeling lucky, they should get author instead of distribution page.

@rwstauner
Copy link
Contributor

There's one problem with this: If the query is a match for a single author, but not exactly a pauseid, we get a 404.
For example, searching for "stauner" returns only a single result, however, my pauseid is actually "rwstauner", so "/author/stauner" is a 404.

So I see two possible solutions:

  • Use the value that came back in the query; Change:

    $c->res->redirect( '/author/' . uc($query) );
    

    to

    $c->res->redirect( '/author/' . $author->{results}->[0]->{pauseid} );
    
  • Require an exact match in the condition (and otherwise let the logic fall through): Change:

    if ( $author->{total} == 1 ) {
    

    to

    if ( $author->{total} == 1 && uc($query) eq $author->{results}->[0]->{pauseid} ) {
    

@neilbowers since the idea was yours, do you have a suggestion?
Anybody else?

@neilb
Copy link
Contributor

neilb commented Jul 27, 2014

@rwstauner: I prefer your second option, as that was what I proposed: if you enter a single word, hit shift+enter, and the word is a valid PAUSE username.

I'd drop the uc though, to require that the PAUSE username is entered in upper-case.

And I also realised that there's potential for someone to be searching for a module using an acronym that has also been used as a PAUSE id. Quick example: XDR is both a valid PAUSE username, module name and dist name.

No-one has a username like 'XML' or 'SQL' at the moment, but we could easily end up with one.

Personally, I'd still go with this, but you may want to reconsider.

@rwstauner
Copy link
Contributor

To be honest the only reason I considered this is because it's only when the "feeling lucky" feature has been used, and I don't consider that "useful" so much as a novelty.
On the other hand, similar to the "single-result redirect" fiasco, we're making the behavior of a long-standing feature less consistent: There are now cases (which as you mentioned, could be accidental) where you no longer go straight to a module page.

@oiami
Copy link
Contributor Author

oiami commented Jul 29, 2014

I've brought both of your suggestion into the code, I think I agree with @neilbowers in that user should search with all upper case and should exactly match the PAUSE id. Anyway in the case when the module name and author name are the same is something that I concern. I think how about if we change condition to match the distribution first in the case if that query exactly match module/distribution name ? Unless try to find the author instead of the distribution name. Not sure if I explained clearly. For example , (luckily I found the keyword that the name of both author and module) The key word is Win32, there's distribution named Win32 and author named WIN32. My offered solutions are

  • If the query is Win32 try to find module first if we found that exactly match then redirect to /pod/Win32
  • If the query is WIN32 these all upper case, user probably mean to author instead of distribution name in my opinion so we redirect them to /author/WIN32
  • If the query is win32 this not exactly match neither distribution nor module but so let redirect them to distribution page if there are results return. I think if they want to search for author they probably search with upper case instead.
  • If user didn't search with upper case and no module result is found then they should get 404.

This solution is a bit more expensive than the previous one because we have to send query to api twice if query isn't exactly match distribution name but I think it a smart solution (as far as I can come up with ;) )

I personally like to add more feature the app even though it won't be used or it will get good or bad feed back. This issue not a time consumed task anyway. I'm ready to roll it back it if people don't like it. Even If it got many complains at least I know that many people are using it so than I can know what should I priotize and trying to find better thing (I'll appreciate if got suggestion with the complain too) :D

Any comment ?

@neilb
Copy link
Contributor

neilb commented Jul 29, 2014

How about this approach:

If a shift+enter search matches /^[A-Z0-9]+$/ then it's a potential user match

  • check whether there's a PAUSE user with that id, and if there's a module with that name.
  • If there's both, then show a regular search result, don't go straight to either page. Would be good if
    a comment could be displayed, along the lines of "search was ambiguous".
  • If there's a valid user and no module, go to the author page
  • Otherwise do what you do now

@rwstauner
Copy link
Contributor

Sorry, @oiami, I didn't realize you had pushed another commit to this.
I think that logic works, and I appreciate the thoroughness of your test cases.
I'm ok with this (module if exact match, else author if exact match, else module first result).

I'm not inclined to disable the lucky feature for the "search was ambiguous" case, because that's pretty much always been true (search is likely ambiguous, but hey, you said you were "feeling lucky").

So, any last comments? Otherwise I'm prepared to merge this.

@oiami
Copy link
Contributor Author

oiami commented Sep 15, 2014

I think this is done as my intention if there're any more tweak on this could be another issue. I'll just rebase is since this branch is a bit outdated.

@rwstauner
Copy link
Contributor

Lovely, thanks!

On Sun, Sep 14, 2014 at 8:52 PM, Pattawan Kaewduangdee <
notifications@github.com> wrote:

I think this is done as my intention if there're any more tweak on this
could be another issue. I'll just rebase is since this branch is a bit
outdated.


Reply to this email directly or view it on GitHub
#1258 (comment)
.

@oiami oiami force-pushed the Redirect_to_author_page_on_click_lucky_if_pauseid_found branch from 7074a5d to fadd09f Compare September 15, 2014 04:15
@rwstauner
Copy link
Contributor

Thanks, @oiami.
Would you mind updating the tests to use single quotes instead of double quotes (in the urls passed to the GET function)? Perl critic is complaining because double quotes are used when there's no interpolation there.
If you push that change to this branch I think travis will be happy and we can merge this :-)

@oiami oiami force-pushed the Redirect_to_author_page_on_click_lucky_if_pauseid_found branch from fadd09f to 407ea90 Compare September 21, 2014 10:08
@oiami
Copy link
Contributor Author

oiami commented Sep 21, 2014

@rwstauner changed double quotes to single quotes and squashed the commit, should be fixed now.

rwstauner added a commit that referenced this pull request Sep 27, 2014
…lucky_if_pauseid_found

Redirect to author page if query exactly pauseid
@rwstauner rwstauner merged commit 6cc12d9 into metacpan:master Sep 27, 2014
@rwstauner
Copy link
Contributor

LGTM. Thanks!

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.

None yet

3 participants