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

Search hackery #1187

Merged
merged 23 commits into from Aug 31, 2021
Merged

Search hackery #1187

merged 23 commits into from Aug 31, 2021

Conversation

knirirr
Copy link
Contributor

@knirirr knirirr commented Jul 29, 2021

This code causes search parameters to be modified if they match certain (known) patterns from the old system.

This ticket lists some particular URLs which have been used in a paper recently and so must be intercepted, as readers of the paper will encounter the new site and try to use old URLs with it.

#1186

Look in src/router/hackSearch.js for details of the re-writing. It's called any time anyone visits a search url (see src/router/index.js). The modified parameter should ensure that the hack is only called once.

Of course, queries formatted for the new system should pass through hackSearch.js untouched.

@knirirr knirirr linked an issue Jul 29, 2021 that may be closed by this pull request
@knirirr knirirr marked this pull request as ready for review August 18, 2021 15:15
Copy link
Contributor

@hoseinmirian hoseinmirian left a comment

Choose a reason for hiding this comment

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

Thanks, it works fine very good but there is a minor issue. For the first visit or after refreshing the page whilst having an invalid query string it works fine but after the page is loaded and we press back on the browser it will include and accept the query as a valid query in the chips holder, please check both URL and also chips holder situation:

image

@knirirr
Copy link
Contributor Author

knirirr commented Aug 23, 2021

I'm not sure that this is something that can be prevented; this whole PR is a bit of a hack in any case.
Do you have any suggestions as to how to accomplish this? If there happened to be some way to call next() without the page with incorrect params ending up in history it would be useful, but I don't think there is here:

if (modified) {
next({name: 'search', query: query});
}

@hoseinmirian
Copy link
Contributor

I'm not sure that this is something that can be prevented; this whole PR is a bit of a hack in any case.
Do you have any suggestions as to how to accomplish this? If there happened to be some way to call next() without the page with incorrect params ending up in history it would be useful, but I don't think there is here:

if (modified) {
next({name: 'search', query: query});
}

I tried some code today to see how I can fix it. still no luck

@hoseinmirian
Copy link
Contributor

hoseinmirian commented Aug 23, 2021

@knirirr

My Final review

I think no user can add those edge-cases filters into the URL by using our UI, and the only way they might be redirected would be an old link given to them or shared with them(which prevent them your code properly in these cases), then the code you already had written I believe is enough and its very rare that someone enters a new filter(query) in the URL or press back. Besides we are not responsible for all sort of data user enters in the query if no data is returned then there will be no problem in this regard as well.

Due to the above reasons I approve your branch.

@knirirr
Copy link
Contributor Author

knirirr commented Aug 24, 2021

Thanks, sounds reasonable.
I'll not merge this yet, in case anyone else wants to comment. In the absence of any further suggestions I'll do so before we switch to the new system so that those old links will be caught.

@knirirr knirirr merged commit 67c8c92 into dev Aug 31, 2021
@knirirr knirirr deleted the search_hackery branch August 31, 2021 15:23
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.

Outdated URLs
2 participants