3723 – Fix Kwai parser #387
Merged
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
vasconsaurus
force-pushed
the
fix-3723-fix-kwai-parsing
branch
from
2c8cc15
to
0c88a92
Compare
The parser stopped working, the classes we were using to parse the doc don't seem to be there anymore. There doesn't seem to be something we could directly replace them. But there is a ldjson available, so we are now getting our data from it.
right now I'm keeping the old behavior and adding the new one as a fallback. not too sure about this yet.
the title we were getting from the ld+json could be the same for multiple videos from the same author, so we are setting the title to the url instead and since what we get for the title and description might vary, in the integration test we only check if those two are strings
vasconsaurus
force-pushed
the
fix-3723-fix-kwai-parsing
branch
from
0c88a92
to
473706b
Compare
The url fallback is handled by Media. I wanted to test that, and the Kwai parser directly, so I made two tests: test "assigns description and username to hash from the json+ld" "assigns values to hash from the json+ld and falls back to url as title" The second test had worked locally, but failed on Travis, then failed in weird ways locally, but now seems to be successful. I'll run it on Travis again and see how this behaves.
|
Code Climate has analyzed commit bd60776 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 98.6%. View more on Code Climate. |
caiosba
approved these changes
Sep 11, 2023
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
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.
Description
Our Kwai test has been failing for a while, since it had been flaky before we assumed it was just flaky again. However, it seems that the parsing is indeed broken: we search the doc for '.info .title' and for '.name', and both of those are returning nil.
We can get data from the jsonld script. Besides the
name(we can get that fromcreator name), it doesn't return the same exact data as before. We can usedescriptionortranscriptionfordata['description']. Fortitlewe don't have a good substitute, so we are just using the default fallback fromMedia(url).We are leaving the old behavior and adding the data from the jsonld script as a fallback. Kwai has changed their HTML and changed it back again before. So we think it's worth keeping the old behavior, at least for now.
References: 3723, 3716
How has this been tested?
I updated the integration test to use the new values we are fetching (
rails test test/models/parser/kwai_test.rb:4), and added two unit tests:rails test test/models/parser/kwai_test.rb:50)rails test test/models/parser/kwai_test.rb:61)