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

fix: ignore null items in API responses #240

Merged
merged 9 commits into from
Apr 8, 2024

Conversation

eatyourgreens
Copy link
Contributor

@eatyourgreens eatyourgreens commented Mar 28, 2024

- configure the talk client to use the production API for tests.
- test a talk production search that is known to break the API client.
- ignore null response data, so that the tests pass.
@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Mar 28, 2024

@mcbouslog this fixes a bug that Alisa found earlier this week.

@mcbouslog mcbouslog self-requested a review March 29, 2024 19:30
@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Apr 1, 2024

Now with mocks, so that the tests aren't running searches against the live API. The mocks are simply JSON snapshots from real Talk searches, including the search that's broken.

Includes a real Talk production search query that only returns null results, which seems to be a bug in the API itself.

@eatyourgreens
Copy link
Contributor Author

zooniverse/Panoptes-Front-End#7071 installs this version of the client in PFE.

It needs to be deployed to staging, but here's a link to one of the broken searches:

https://pr-7071.pfe-preview.zooniverse.org/talk/search?query=depression&env=production

@mcbouslog mcbouslog self-assigned this Apr 3, 2024
Copy link
Contributor

@mcbouslog mcbouslog left a comment

Choose a reason for hiding this comment

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

Changes look good, mock talkClient and testing additions look great.

Confirmed using zooniverse/Panoptes-Front-End#7071 locally that existing error message "There was an error with your search. Please try again." is fixed with these changes 👍 .

On page 3 of the results (https://local.zooniverse.org:3735/talk/search?env=production&page=3&query=depression) the pagination shows a page 4 and 5, which are the null response data pages, which is a little confusing, but that's ok and beyond the scope of these changes, just noting for myself.

@eatyourgreens
Copy link
Contributor Author

There's definitely something weird going on with that search. After the first 29 results, the remaining 13 are all null.

@mcbouslog mcbouslog merged commit 9db4b43 into zooniverse:main Apr 8, 2024
3 checks passed
@eatyourgreens eatyourgreens deleted the ignore-null-comments branch April 8, 2024 15:49
@shaunanoordin shaunanoordin mentioned this pull request Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants