Skip to content

fix(Twitter): urls are now formatted correctly for posts and messages #3953

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

Merged
merged 7 commits into from
Apr 30, 2020

Conversation

rowasc
Copy link
Contributor

@rowasc rowasc commented Apr 22, 2020

Using the format https://twitter.com/{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 PR introduces an extra fix basically over my last PR which is that it updates the posts too when they match the message....

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

  • I certify that I ran my checklist

Fixes ushahidi/platform# .

Ping @ushahidi/platform

rowasc added 3 commits April 22, 2020 11:15
An overcomplicated way to say "hey please update the messages when they match the posts and are twee=ts"
@rowasc rowasc changed the title fix(Twitter): urls are now formatted correctly for posts and messages WIP fix(Twitter): urls are now formatted correctly for posts and messages Apr 22, 2020
@rowasc rowasc changed the title WIP fix(Twitter): urls are now formatted correctly for posts and messages fix(Twitter): urls are now formatted correctly for posts and messages Apr 22, 2020
@rowasc
Copy link
Contributor Author

rowasc commented Apr 24, 2020

come on little pull request you can do it.

Anyway, this passes for me locally. Tested up/down migration

disable phpcs for up/down because it ironically hurts readability (more!) to cut the lines even more
@coveralls
Copy link

coveralls commented Apr 24, 2020

Coverage Status

Coverage remained the same at 20.287% when pulling 9495ecd on twitter-urls-fix into 02f2f08 on develop.

@rowasc rowasc requested a review from tuxpiper April 27, 2020 14:53
Copy link
Member

@tuxpiper tuxpiper left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@rowasc
Copy link
Contributor Author

rowasc commented Apr 29, 2020

@Obadha2 please check. thanks!

@theobadha theobadha merged commit 6ff884f into develop Apr 30, 2020
@theobadha
Copy link
Contributor

@rowasc this still fails QA. It seems the merge undid previous changes. New tweets coming in have the link now looking like:
/statuses/{twitterStatusId}
Screenshot 2020-05-04 at 11 39 18 AM

@rowasc
Copy link
Contributor Author

rowasc commented May 5, 2020

@Obadha2 that's odd, maybe something with steve 🤔 Will check what's up with the server since it passed everything locally .

@rowasc
Copy link
Contributor Author

rowasc commented May 5, 2020

agh I got it, it's becuase I forgot to revert your revert , so develop doesn't have the code for it

@tuxpiper tuxpiper deleted the twitter-urls-fix branch March 24, 2021 20:35
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