search

nusmodifications / nusmods

🏫 Official course planning platform for National University of Singapore.
https://nusmods.com
MIT License
585 stars 319 forks source link

"Click here to try again" button deletes the 'q' query parameter #3288

Closed btzy closed 3 years ago

btzy commented 3 years ago

Describe the bug

When there is a network/server issue and we can't load modules, the "Click here to try again" button deletes our query, although all filters are retained.

To Reproduce

Steps to reproduce the behavior:

  1. Search for a module at a busy time or somehow prevent connection to the server, so that the "Oh no... We can't load the module information" page appears (for example, we might go to https://nusmods.com/modules?q=geh1042&no-exam=true)
  2. Click on the "Click here to try again" button.
  3. Observe that the 'q' parameter of the query is deleted (so we are sent to https://nusmods.com/modules?no-exam=true). This also happens if there were no additional selection parameters after the 'q' parameter.

Expected behavior

Don't clear the 'q' parameter.

Screenshots

https://user-images.githubusercontent.com/6948096/122679513-bf484080-d21d-11eb-8966-b6303ad75ea2.mp4

Desktop (please complete the following information):

chrisgzf commented 3 years ago

Thanks for the bug report @btzy!

Triage Outcome

This bug occurs in the ElasticSearch-related parts of our module search. This bug is especially prominent in recent times, when our ElasticSearch index is less stable (we are still trying to figure out how to make it more stable).

Cause of bug

Offending function: resetSearchFn in component ModuleFinderApiError

This function gets called when the user presses the "Click to try again" button. Somehow this causes the query params to disappear.

Steps to resolve

Difficulty: Low

NoHits component from searchkit (the library we use to interact with ES) in ModuleFinderContainer encapsulates the logic for what happens when there is an API failure. It currently controls the ModuleFinderApiError component, so it's a black-box to us what it is doing, and if it is passing the right resetSearchFn to ModuleFinderApiError.

You will have to figure out what searchkit is doing in NoHits (this might require you diving into the source code of searchkit, which is OSS), and if we can customise / patch the resetSearchFn to ideally:

  1. NOT remove the query params (this is what is broken atm)
  2. Retry the search (this could already be provided by the searchkit libs alr)
    • just FYI: the search for ES's search box is performed via handleSearch in SearchkitSearchBox

If it is difficult to perform a search retry (or very messy), we can consider a full page reload, which should work if the query params aren't removed. But note that this is not ideal as our webapp is a SPA and we should try to avoid full page reloads.

Tests

If you know how to write unit tests, write a unit test to use react-testing-library's renderWithRouter to render the error page with some query params, then assert that the query params should still exist after clicking the button.

Testing

Change elasticsearchBaseUrl in website/src/config/app-config.json to something that doesn't exist, and you should be able to test the 404 page.

cc: @taneliang you have more context about the ES stuff here lmk if there's anything I've incorrectly mentioned

If anyone wants to take it up, do leave a comment here!