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

141/fix response system #147

Merged
merged 7 commits into from
Apr 4, 2018
Merged

141/fix response system #147

merged 7 commits into from
Apr 4, 2018

Conversation

McEileen
Copy link
Member

Hi @vkoves, could you please review these changes? In response to issue #141, I added an @ before the educatee's handle and fixed the code so Tweechable won't tweet at itself. Thanks!

fixes #141

Copy link
Contributor

@vkoves vkoves left a comment

Choose a reason for hiding this comment

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

Generally looks good, just have a quick question!

@@ -8,18 +8,19 @@ def self.retrieve_mentions
twitter = Twitter_API.new
@client = twitter.client
mentions = @client.mentions_timeline
@account_name = @client.user.screen_name
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be an instance variable? It seems to only be used here and within the each block, so I think a normal variable without the @ would make more sense

@vkoves
Copy link
Contributor

vkoves commented Mar 26, 2018

Additional question: Can we write any more tests to ensure that the issue is fully resolved and stays that way?

@McEileen
Copy link
Member Author

McEileen commented Apr 2, 2018

Hey @vkoves, could you please help me get unstuck on the test I'm trying to write? There was a test helper method meant to generate a tweet with user_mentions, called generate_tweet_with_author_and_mentions. However, when I ran it the resulting tweet object didn't have any user_mentions. (The generate author functionality does seem to work fine.) I looked up the documentation on user_mentions and added the indices as parameters for each user in user_mentions, but still, no dice!
Any help would be appreciated, thanks.

@vkoves
Copy link
Contributor

vkoves commented Apr 2, 2018

@McEileen - it took a good bit of digging, but I think I got the issue resolved. The user_mentions seem to be needed to be passed inside of an entities hash, which I discovered by digging through the tests of the Twitter gem and finding this line that generates a tweet with mentions. Their documentation didn't seem to reference this, but I think that's probably because they don't expect folks to be making full tweet objects with user_mentions like we are.That's just a guess though, since I am not too familiar with the Twitter gem.

@McEileen
Copy link
Member Author

McEileen commented Apr 3, 2018

@vkoves Thanks for all of the digging! The user_mentions formatting issue was messing up other test_helper methods, but I refactored to make the test coverage more complete.

Now I'm having a hard time figuring out how to test that the bot sends a tweet which includes the potential educatee's handle with an @ before it. I hoped the If a mention is well formatted, it will trigger a tweet that contains the educatee's handle with an @ symbol before it test would work, but the issue is that Mention.reply_mentions returns mentions, not tweets. I'm not sure how to test the tweet content, to_send, that is generated on line 58. In JUnit I'd use a captor, but I'm not sure how to do something for Ruby on Rails. Any ideas? Thanks!

@vkoves
Copy link
Contributor

vkoves commented Apr 4, 2018

@McEileen - that is a great question, but unfortunately I've been unable to figure out how to do that. Especially since the Mention.reply_mentions method generates a new Twitter_API object itself, it would be tricky to do, but I am also not sure how to handle spying on functions in Ruby either 😞

I found this article that should be helpful for this, but I couldn't get it to work; perhaps you can figure it out though?

@McEileen
Copy link
Member Author

McEileen commented Apr 4, 2018

That article looks helpful, thank you! I want to deploy the fixes, so I am going to merge in the changes
(including the commented out failing test) and deploy, then close issue 141. However, I am going to make a new, spin-off issue for testing that the tweet content includes @ before the educatee's handle. I'll assign myself to that issue, too. (I'm not sure if spin-off issues are best practice for OSS..... Please let me know if you don't agree.)

@McEileen McEileen merged commit 40f7f6b into master Apr 4, 2018
@McEileen McEileen deleted the 141/fix-response-system branch April 4, 2018 01:32
@vkoves
Copy link
Contributor

vkoves commented Apr 4, 2018

That seems fine. Since this is such a small change and unlikely to break again, I think manually testing it and then taking the testing on later should be sufficient.

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.

Current response system is broken
2 participants