Fix fetching search results and handle fetching errors #2618
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openverse-bot
added
🟧 priority: high
|
Size Change: +506 B (0%) Total Size: 839 kB
ℹ️ View Unchanged
|
8 tasks
obulat
force-pushed
the
fix/media_error_handling
branch
7 times, most recently
from
672cbb3
to
99ab42e
Compare
obulat
force-pushed
the
fix/media_error_handling
branch
from
99ab42e
to
48790cd
Compare
obulat
changed the title
obulat
force-pushed
the
fix/media_error_handling
branch
from
48790cd
to
ced64dd
Compare
obulat
force-pushed
the
fix/media_error_handling
branch
4 times, most recently
from
d8a1a0b
to
9915ead
Compare
obulat
force-pushed
the
fix/media_error_handling
branch
from
d2dd848
to
b2460a2
Compare
obulat
force-pushed
the
fix/media_error_handling
branch
2 times, most recently
from
30f314e
to
82e52b6
Compare
obulat
force-pushed
the
fix/media_error_handling
branch
2 times, most recently
from
99af1d3
to
5862fda
Compare
AetherUnbound
approved these changes
Jul 21, 2023
Comment on lines
+123
to
+130
| /** | ||
| * Search middleware runs when the path changes. This watcher | ||
| * is necessary to handle the query changes. | ||
| * | ||
| * It updates the search store state from the URL query, | ||
| * fetches media, and scrolls to top if necessary. | ||
| */ |
There was a problem hiding this comment.
Thank you for adding these comments!
obulat
force-pushed
the
fix/media_error_handling
branch
from
5862fda
to
fb8f0d2
Compare
|
Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR: @dhruvkb Excluding weekend1 days, this PR was ready for review 3 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)2. @obulat, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
dhruvkb
requested changes
Jul 24, 2023
Right, I haven't thought about it. Thank you, Dhruv! |
obulat
force-pushed
the
fix/media_error_handling
branch
from
fb8f0d2
to
e8e08d9
Compare
obulat
force-pushed
the
fix/media_error_handling
branch
5 times, most recently
from
10fb0c7
to
421bafc
Compare
I updated the PR to set the error to |
obulat
force-pushed
the
fix/media_error_handling
branch
2 times, most recently
from
89efb8e
to
989d8f5
Compare
obulat
force-pushed
the
fix/media_error_handling
branch
from
989d8f5
to
6880335
Compare
dhruvkb
approved these changes
Jul 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
Fixes #2588 by @obulat
Description
This PR moves the fetching of search results from the
asyncDatato thesearchmiddleware and theuseFetchhook in thesearch.vuepage.Fetching search results on the server
Just like in the single result pages, on the server, we make sure that we can fetch the data or redirect to the Nuxt error page. In the
search.vuepage, we setfetchOnServerto false so that the data that was fetched on the server (if the search page was opened directly, when you go to https://openverse.org/search/?q=cat) isn't refetched when the page is hydrating.Fetching search results on the client
If we navigate to the search page from other pages (homepage or 404 page), the
useFetchcallsfetchMediainsearch.vueto get the search results. If the route changes (either the path, i.e. when the search type changes, or the query, i.e., when the search term, or filters change), the router watch callsfetchMediainsearch.vue.Updating the provider filters
Previously,
asyncDataupdated the search state and provider filters. Here, this is moved to thesearchmiddleware.Fetching errors
When there are no search results for a query, we receive a 200 response with an empty result list. In the
mediastore, we setfetchingErrorto 404 status in case there are no results, to show the 404 page.fetchingError.messagetoNO_RESULTto show the No results page.All content errors
This PR updates the handling of the "All content" errors: if there is even a single 429 error, we set the combined
allMediaErrorstatus to 429, because it means that all client searches will be throttled.allMediaErroris set to 404 status only if thefetchingErrors for all media types is 404, so we only show a 404 page for "All content" if both audio and image had no results.allMediaErrorto set aNO_RESULTerror message if there are no results for all supported media types. Otherwise, we show the available media type results.Testing Instructions
Searching from the homepage and 404 page, as well as from the search page's header should work correctly.
Clicking "Load more" should load the next page of the results for all search types.
For all media, check cases with only one type of results, such as "ecommerce image".
Also, try queries that don't have any results (such as "querywithnoresults").
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin