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.

Sign up for GitHub

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

Jump to bottom

TgDocuments Overhaul #127

Merged
merged 2 commits into from Feb 23, 2019
Merged

TgDocuments Overhaul #127

merged 2 commits into from Feb 23, 2019

Conversation

Tjzabel
Copy link
Member

@Tjzabel Tjzabel commented Feb 22, 2019

The goal of this PR is to take care of:

  1. Issue Image url reveals bot's token #115
  2. Enabling Imgur by default
  3. Overhaul of TgDocumentHandler unit tests
  4. Documentation surrounding these issues

TL;DR

This PR removes the telegram bot's API from showing when images and files are sent over Telegram. Imgur is now enabled by default, and sending documents over Telegram is now set to false by default, and sends a message over to IRC with the document's caption so the IRC side isn't completely left out.

@Tjzabel
Enable Imgur by default.
ec67d9b 
Don't send Telegram documents to IRC to mask bot api key.
@Tjzabel Tjzabel added documentation Improvements or additions to documentation priority:high quality assurance CI tests, unit tests, integration tests, any kind of testing new change Adds new capabilities or functionality labels Feb 22, 2019
@Tjzabel Tjzabel added this to the v1.3 milestone Feb 22, 2019
@Tjzabel Tjzabel requested a review from a team February 22, 2019 04:45
@jwflory
Copy link
Member

jwflory commented Feb 22, 2019

Thanks @Tjzabel, this looks awesome. 💯 I have some additional feedback:

  1. Code review: Changes look good to me, but I'd prefer another @RITlug/teleirc-developers-commit-access developer to review too. I'm not as savvy.
  2. Before merging, a staging test: To test these changes in a pre-existing install, I want to deploy this branch to our development bot in #ritlug-teleirc. (I'll approve the PR once I do this.)
  3. Consolidating commits: Could you squash these commits to 2-3 commits with meaningful messages before we merge? Something like these suggestions.

Thanks for working on this @Tjzabel. 👍

@jwflory jwflory added this to In progress in TeleIRC development via automation Feb 22, 2019
@Tjzabel
Set default document sending to false, and relay captions.
86ba8e8 
Write new unit tests and documentation to support new document-sending
format.

Make sure document message-forming actions lay in GetDocumentMessage.

TgDocumentHandler unit tests are successful.
@Tjzabel
Copy link
Member Author

Tjzabel commented Feb 22, 2019

@jwflory thanks for the review! I've squashed my commits into 2.

As a further note, here is what sending a document over to IRC looks like (disabled and enabled):

Disabled:

Tjzabel21 shared a file on Telegram with caption: 'hello!'

Enabled:

'My caption!' uploaded by Tjzabel21:
https://api.telegram.org/file/bot<api_key>/documents/<file>.jpg
  • The api_key of your Telegram bot will be shown.

@jwflory
Copy link
Member

jwflory commented Feb 23, 2019

We just tested this change on the staging environment in #ritlug-teleirc. Everything is looking good! Merging. 🎬

@jwflory jwflory merged commit 92954d4 into master Feb 23, 2019
TeleIRC development automation moved this from In progress to Done Feb 23, 2019
@jwflory jwflory deleted the api-key-fix branch February 23, 2019 20:47
@Tjzabel Tjzabel restored the api-key-fix branch July 20, 2019 18:45
@Tjzabel Tjzabel deleted the api-key-fix branch July 20, 2019 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thenaterhood thenaterhood thenaterhood approved these changes

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation new change Adds new capabilities or functionality quality assurance CI tests, unit tests, integration tests, any kind of testing
Projects
TeleIRC development
  
Done
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Tjzabel @jwflory @thenaterhood