Add parser for twitter search URLs #420
Conversation
jayjay-w
force-pushed
the
CV2-3708-Look-into-and-add-search-endpoint-to-Twitter-parser
branch
from
8d72005
to
77334de
Compare
Add parser for twitter search URLs. Currently, these URLs are failing and throwing errors. Related to: CV2-3708
jayjay-w
force-pushed
the
CV2-3708-Look-into-and-add-search-endpoint-to-Twitter-parser
branch
from
77334de
to
bd72351
Compare
jayjay-w
requested review from
caiosba,
melsawy and
vasconsaurus
as code owners
| class TwitterSearchItem < Base | ||
| include ProviderTwitter | ||
|
|
||
| TWITTER_SEARCH_ITEM_URL = /https:\/\/twitter\.com\/search[^\s"]*/ |
There was a problem hiding this comment.
Request: I suggest we make this regular expression broader:
- We should always expect at least the
qparameter - We should support HTTP
- We need to be sure it starts with http
- We should support other subdomains (www, mobile, etc.)
That said, I suggest to use the host part we have here: https://github.com/meedan/pender/blob/develop/app/models/parser/twitter_item.rb#L5
Suggestion: We could extract the host part of that regular expression into a TWITTER_HOST_URL constant defined in ProviderTwitter and reused by the other Twitter modules.
There was a problem hiding this comment.
Thank you, these were great suggestions. I have implemented everything except the last suggestion, which I will move later.
| title: search_term, | ||
| search_term: search_term, | ||
| src: src, | ||
| description: "Twitter search for #{search_term}" |
There was a problem hiding this comment.
Request: Testing locally, I got two items with different URLs to match because of this description, so I suggest we keep it simple and just return the URL (unescaped, for better readability) here.
There was a problem hiding this comment.
Done as well. The description is now just the decoded URL.
| # Main function for class | ||
| def parse_data_for_parser(_doc, _original_url, _jsonld_array) | ||
| handle_exceptions(StandardError) do | ||
| params = URI.parse(url).query.split('&').map { |pair| pair.split('=') }.to_h |
There was a problem hiding this comment.
Request: Testing locally, I noticed that in some cases the URLs we receive contain escaped characters, so I suggest we unescape when parsing the URL here.
Add reviewer feedback: - Escape URLs sent to parser - Handle different domains (m.twitter.com and mobile.twitter.com) - Move src to api
|
Code Climate has analyzed commit 4b1e2a9 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% (0.0% change). View more on Code Climate. |
|
@caiosba this is ready for another review |
|
Thank you! |
jayjay-w
deleted the
CV2-3708-Look-into-and-add-search-endpoint-to-Twitter-parser
branch
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Description
Add parser for twitter search URLs. Currently, these URLs are failing and throwing errors.
Related to: CV2-3708
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can verify the changes. Please describe whether or not you implemented automated tests.
Things to pay attention to during code review
Please describe parts of the change that require extra attention during code review, for example:
Checklist