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(Twitter): urls are now formatted with the std https://twitter.com… #3925

Merged
merged 11 commits into from
Apr 21, 2020

Conversation

rowasc
Copy link
Contributor

@rowasc rowasc commented Apr 9, 2020

…/{userid}/status/{tweetId}

  • This change was necessary because the old /statuses path stopped working at some point, which means all the twitter urls we grabbed from that point onwards were broken. Not super great
  • There doesn't seem to be any way to get the correct URL from the API response we get from /search API, which is very dissapointing because if the format changes again this will stop working. The only thing that saves us here is that because this is a more user facing url, its likely that it will be redirected even if it changes, rather than just stop working

This pull request makes the following changes:

  • Changes twitter urls to be in the format /user/{twitterUserId}/status/{twitterStatusId}. Migrates old twitter URLs as well as generates new ones in this format

Test checklist

BEFORE you merge this PR, ensure that you

  • Have a deployment with tweets
  • Check that the tweet's link doesn't work (ie it's a 404)

After you merge this PR and it lands in steve

  • Check some old tweets and verify the links work now
  • Get new tweets from the twitter datasource, the links should work

@Obadha2 this is important for context see above the checklist

  • I certify that I ran my checklist

Fixes ushahidi/platform# .

Ping @ushahidi/platform

…/{userid}/status/{tweetId}

- This change was necessary because the old /statuses path stopped working at some point, which means all the twitter urls we grabbed from that point onwards were broken. Not super great
- There doesn't seem to be any way to get the correct URL from the API response we get from /search API, which is very dissapointing because if the format changes again this will stop working. The only thing that saves us here is that because this is a more user facing url, its likely that it will be redirected even if it changes, rather than just stop working
*/
public function change()
{
$sql = "UPDATE messages
Copy link
Contributor Author

@rowasc rowasc Apr 9, 2020

Choose a reason for hiding this comment

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

I had to do replace rather than set because I wanted to play it safe in case there is some scenario where message doesn't conform with just having a single tweet as the content and has other content that we shouldn't just erase with a SET
Light concern: this change should be fine, but it may be pretty heavy in some deployments... like my ridiculous testing one with like 300k tweeets ...

* API itself in the v1.1 search endpoint.
* Fun fact: if the user id is wrong, twitter still takes you to the correct Tweet... they just use the tweet id
**/
'message' => "https://twitter.com/$user_id/status/$id",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

couldn't find a way to get twitter urls... so I'm using this more obvious user facing URL instead of /statuses... which I suppose gives me more peace of mind... but still, would prefer if twitter actually gave us the url to use, you know?

Copy link
Member

Choose a reason for hiding this comment

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

I'd so much love that too!

@rowasc rowasc requested a review from tuxpiper April 9, 2020 23:59
@rowasc
Copy link
Contributor Author

rowasc commented Apr 10, 2020

@tuxpiper this is medium-gross, but there wasn't a good way to get the URL so I suppose is the best we can do right now (unless we figure out a better way ? lol). Not super happy about the fact that this may become broken if twitter decides to deprecate this url format without redirects like they did for /statuses tho

{
$sql = "UPDATE messages
INNER JOIN contacts on contacts.id = messages.contact_id
SET messages.message = REPLACE(messages.message, concat('https://twitter.com/statuses/', messages.data_source_message_id), concat('https://twitter.com/', contacts.contact, '/status/', messages.data_source_message_id))
Copy link
Member

Choose a reason for hiding this comment

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

I thought this couldn't be done, because when I was checking a database with tweet messages, I couldn't see the tweet user handles anywhere. Turns out that we are storing the numeric ID for the user.

It also turns out that twitter at this point in time now will accept that numeric ID in the tweet URL (i.e. https://twitter.com/:userNumId/status/:tweetId ) and redirect to the URL with the user handle.

So that's good now, it makes me queasy though 🤷 Should we start storing the screen name, should we start storing both? Big questions for a Friday

@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 20.295% when pulling 3241f62 on twitter-urls-fix into 3037be1 on develop.

@AmTryingMyBest AmTryingMyBest merged commit 8c4e0f9 into develop Apr 21, 2020
@AmTryingMyBest
Copy link
Contributor

@rowasc only new tweets coming in have working links.
Old ones before I merged remain broken.

@rowasc
Copy link
Contributor Author

rowasc commented Apr 21, 2020

@Obadha2 oh that's unfortunate :/ Checking

@rowasc
Copy link
Contributor Author

rowasc commented Apr 21, 2020

@Obadha2 which deployment did you use? just so I go into it

@AmTryingMyBest
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants