[Notifier] [Telegram] Extend options for `location`, `document`, `audio`, `video`, `venue`, `photo`, `animation`, `sticker` & `contact` #51717

igrizzli · 2023-09-22T00:54:15Z

Q	A
Branch?	6.4
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets
License	MIT
Doc PR	symfony/symfony-docs#...

Extends #49986 for sending local files as photo

Add possibility to send point on map in notification.

OskarStark

Overall this PR looks good to me 👍🏻

src/Symfony/Component/Notifier/Bridge/Telegram/README.md

src/Symfony/Component/Notifier/Bridge/Telegram/TelegramTransport.php

src/Symfony/Component/Notifier/Bridge/Telegram/TelegramOptions.php

OskarStark · 2023-09-27T14:27:38Z

Do you know if there is an option to send documents?

igrizzli · 2023-09-27T14:52:04Z

Do you know if there is an option to send documents?

Yes, i can add this feature on this weekend

OskarStark · 2023-09-27T14:55:20Z

Oh that would be great 😍

igrizzli · 2023-09-28T17:29:08Z

@OskarStark sending photo, location and document are exclusive options, it's can be combined in single message between themselves. Should we validate it, to avoid implicit behavior? If we should - Which level is better to use for validation TelegramOptions or TelegramTransport?

OskarStark · 2023-09-28T18:04:41Z

I really like to validate such things, but we can't validate all the things plus we tightly couple the transport to the API, in this case, if things get changed we need to release new code, that's not ideal.

So let's not validate it, document it properly with a note/warning and let the API throw the error message if someone wants to send all of them together.

igrizzli · 2023-09-28T21:28:35Z

@OskarStark I have added document and all other telegram send types, can you look implementation? And if it ok i will finish it with tests and docs.

Also i have question about tests, since we are mocking real telegram api in tests, would it be ok to use single sample file for all types in tests, to avoid adding large binary to repo?

OskarStark · 2023-09-29T05:49:07Z

Also i have question about tests, since we are mocking real telegram api in tests, would it be ok to use single sample file for all types in tests, to avoid adding large binary to repo?

Yes

Can you please update PR title, header, CHANGELOG etc. and mention also the document? Thanks

OskarStark · 2023-09-29T05:53:00Z

src/Symfony/Component/Notifier/Bridge/Telegram/TelegramTransport.php

@@ -115,6 +126,14 @@ private function getPath(array $options): string
            isset($options['message_id']) => 'editMessageText',
            isset($options['callback_query_id']) => 'answerCallbackQuery',
            isset($options['photo']) => 'sendPhoto',
+            isset($options['location']) => 'sendLocation',
+            isset($options['audio']) => 'sendAudio',
+            isset($options['document']) => 'sendDocument',


If I get this right, there is only one Endpoint for document, but we have uploadDocument($filepath) and document($url), can you explain why we need both?

However, what I would check inside the methods without upload prefix, is, that the file exists. Please also add unit tests for this.

Telegram supports 3 ways for sending files - http url, file_id and uploading via api request

docs for photo for example - https://github.com/symfony/symfony/pull/51717/files#diff-d5c6f680237182e620c35f9de66750b1d9363cd6cd69032b68286945990fb4caR84

We can't detect is value file_id or file_path for upload. Before this pull request we had method photo which worked with http url and file_id, for avoiding BC we have to add uploadPhoto. For all other types i added same 2 methods to keep methods consistent for all types.

We can try to detect is argument file_path by checking file_exists, and if not pass value as file_id, but in this case if user will try to pass file_path to not existing file (because some application issues), he will not catch correct api error.

Instead of using extra methods for uploading (uploadPhoto, uploadDocument and etc.) we can use flag upload:

(new TelegramOptions()) ->document('files/test.xlsx') ->upload();

In this case if upload flag exists then will handle argument as filePath if not as fileId or httpUrl

I understand, from my POV we can go with the 2 methods, it's a clear API. 👍

OskarStark · 2023-09-29T05:53:57Z

I saw you added a lot more, like audio, venue etc. very nice.

Please keep the CHANGELOG short and add a list of newly added capabilities. I will update the PR title

src/Symfony/Component/Notifier/Bridge/Telegram/CHANGELOG.md

src/Symfony/Component/Notifier/Bridge/Telegram/TelegramOptions.php

src/Symfony/Component/Notifier/Bridge/Telegram/Tests/TelegramTransportTest.php

igrizzli · 2023-10-01T21:01:57Z

Tests and docs done.
I have also added validation if options targeted to different endpoints, because adding it in a future will be BC.

I have used in tests single file for all endpoints because in real we just need any binary file for testing, and adding multiple fixture files, especially videos, to repo can significantly increase repo size and provide legal issue. But if you think that is critical i will replace it with matched type files.

nicolas-grekas · 2023-10-02T14:52:57Z

src/Symfony/Component/Notifier/Bridge/Telegram/TelegramOptions.php

+        if (null !== $emoji) {
+            $this->options['emoji'] = $emoji;
+        } else {
+            if (isset($this->options['emoji'])) {
+                unset($this->options['emoji']);
+            }
+        }


what about removing all those if/else from the class (aka just do `$this->options['emoji'] = $emoji;) and add an array_filter in the class that turns the options into a URL?

nicolas-grekas · 2023-10-02T14:53:06Z

src/Symfony/Component/Notifier/Bridge/Telegram/CHANGELOG.md

+ * Add support for `sendLocation`, `sendAudio`, `sendDocument`, `sendVideo`, `sendAnimation`, `sendVenue`, `sendContact` and `sendSticker` API methods
+ * Add support for sending local files
+
+


extra line :)

OskarStark · 2023-10-09T18:42:46Z

Good to go @nicolas-grekas ?

…io`, `video`, `venue`, `photo`, `animation`, `sticker` & `contact`

fabpot · 2023-10-10T05:32:43Z

Thank you @igrizzli.

The PR * symfony#51717 introduced a new exception in the Notifier component. I tested this locally and faced the following error: ``` Class "Symfony\Component\Notifier\Exception\MultipleExclusiveOptionsUsedException" not found ``` The version bump of the notifier was missing. I also added tests for the exception class.

…e (OskarStark) This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [Notifier] [Telegram] Fix version and exception signature | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | -- | License | MIT The PR * #51717 introduced a new exception in the Notifier component. I tested this locally and faced the following error: ``` Class "Symfony\Component\Notifier\Exception\MultipleExclusiveOptionsUsedException" not found ``` The version bump of the notifier was missing. I also added tests for the exception class. I am wondering about the parameter signature, and I am not sure if the second parameter should be nullable: https://github.com/symfony/symfony/blob/c868be44ab61ac02e978dd18438915bcaed1aa8d/src/Symfony/Component/Notifier/Exception/MultipleExclusiveOptionsUsedException.php#L17-L31 `@igrizzli` is there anything I am missing or can e adjust the signature: ```diff - public function __construct(array $usedExclusiveOptions, array $exclusiveOptions = null, \Throwable $previous = null) + public function __construct(array $usedExclusiveOptions, array $exclusiveOptions, \Throwable $previous = null) `` Commits ------- 74e0c9c [Notifier] [Telegram] Fix version and exception signature

igrizzli requested a review from OskarStark as a code owner September 22, 2023 00:54

igrizzli force-pushed the notifier/telegram-local-photo branch from 078c582 to 08a89a1 Compare September 22, 2023 00:56

OskarStark approved these changes Sep 22, 2023

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Telegram/README.md Outdated Show resolved Hide resolved

src/Symfony/Component/Notifier/Bridge/Telegram/TelegramTransport.php Outdated Show resolved Hide resolved

OskarStark mentioned this pull request Sep 22, 2023

[Notifier] Add options to Telegram Bridge for sending Location #51716

Closed

igrizzli changed the title ~~[Notifier] Telegram Bridge support sending local photo~~ [Notifier] Telegram Bridge add options for sending location and uploading photo Sep 22, 2023

igrizzli force-pushed the notifier/telegram-local-photo branch from 987b6ed to 4db96b7 Compare September 22, 2023 08:19

igrizzli commented Sep 22, 2023

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Telegram/TelegramOptions.php Show resolved Hide resolved

igrizzli force-pushed the notifier/telegram-local-photo branch from 4db96b7 to 0ba9064 Compare September 22, 2023 08:25

nicolas-grekas added Feature Notifier labels Sep 25, 2023

nicolas-grekas added this to the 6.4 milestone Sep 25, 2023

igrizzli force-pushed the notifier/telegram-local-photo branch from d8dd5e9 to 612eb3d Compare September 28, 2023 21:25

OskarStark requested review from fabpot and stof September 29, 2023 05:49

carsonbot added the Status: Needs Review label Sep 29, 2023

OskarStark reviewed Sep 29, 2023

View reviewed changes

OskarStark changed the title ~~[Notifier] Telegram Bridge add options for sending location and uploading photo~~ [Notifier][Telegram] Add more options for location, document, audio, video, venue, photo Sep 29, 2023

OskarStark changed the title ~~[Notifier][Telegram] Add more options for location, document, audio, video, venue, photo~~ [Notifier][Telegram] Add more options for location, document, audio, video, venue, photo, animation, sticker & contact Sep 29, 2023

OskarStark reviewed Sep 29, 2023

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Telegram/CHANGELOG.md Outdated Show resolved Hide resolved

OskarStark reviewed Sep 29, 2023

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Telegram/TelegramOptions.php Outdated Show resolved Hide resolved

OskarStark reviewed Sep 29, 2023

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Telegram/Tests/TelegramTransportTest.php Outdated Show resolved Hide resolved

OskarStark approved these changes Oct 2, 2023

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 2, 2023

OskarStark requested review from stof and nicolas-grekas October 2, 2023 14:48

carsonbot added Status: Needs Review and removed Status: Reviewed labels Oct 2, 2023

nicolas-grekas reviewed Oct 2, 2023

View reviewed changes

igrizzli requested a review from nicolas-grekas October 7, 2023 22:16

OskarStark added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 9, 2023

fabpot approved these changes Oct 10, 2023

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 10, 2023

[Notifier] [Telegram] Extend options for location, document, `aud…

e223f8e

…io`, `video`, `venue`, `photo`, `animation`, `sticker` & `contact`

fabpot force-pushed the notifier/telegram-local-photo branch from 3633cf3 to e223f8e Compare October 10, 2023 05:32

fabpot merged commit cdf30c5 into symfony:6.4 Oct 10, 2023

fabpot mentioned this pull request Oct 10, 2023

[Notifier] [Telegram] Extend options for location, document, `audio… symfony/symfony-docs#19007

Closed

OskarStark mentioned this pull request Oct 11, 2023

[Notifier] [Telegram] Fix version and exception signature #51994

Merged

This was referenced Oct 21, 2023

Release v6.4.0-BETA1 #52220

Merged

Release v7.0.0-BETA1 #52224

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Notifier] [Telegram] Extend options for `location`, `document`, `audio`, `video`, `venue`, `photo`, `animation`, `sticker` & `contact` #51717

[Notifier] [Telegram] Extend options for `location`, `document`, `audio`, `video`, `venue`, `photo`, `animation`, `sticker` & `contact` #51717

igrizzli commented Sep 22, 2023 •

edited

Loading

OskarStark left a comment

OskarStark commented Sep 27, 2023

igrizzli commented Sep 27, 2023

OskarStark commented Sep 27, 2023

igrizzli commented Sep 28, 2023 •

edited

Loading

OskarStark commented Sep 28, 2023

igrizzli commented Sep 28, 2023

OskarStark commented Sep 29, 2023

OskarStark Sep 29, 2023

igrizzli Sep 29, 2023

igrizzli Sep 29, 2023

OskarStark Sep 29, 2023

OskarStark commented Sep 29, 2023

igrizzli commented Oct 1, 2023 •

edited

Loading

nicolas-grekas Oct 2, 2023

nicolas-grekas Oct 2, 2023

OskarStark commented Oct 9, 2023

fabpot commented Oct 10, 2023

		* Add support for `sendLocation`, `sendAudio`, `sendDocument`, `sendVideo`, `sendAnimation`, `sendVenue`, `sendContact` and `sendSticker` API methods
		* Add support for sending local files

[Notifier] [Telegram] Extend options for location, document, audio, video, venue, photo, animation, sticker & contact #51717

[Notifier] [Telegram] Extend options for location, document, audio, video, venue, photo, animation, sticker & contact #51717

Conversation

igrizzli commented Sep 22, 2023 • edited Loading

OskarStark left a comment

Choose a reason for hiding this comment

OskarStark commented Sep 27, 2023

igrizzli commented Sep 27, 2023

OskarStark commented Sep 27, 2023

igrizzli commented Sep 28, 2023 • edited Loading

OskarStark commented Sep 28, 2023

igrizzli commented Sep 28, 2023

OskarStark commented Sep 29, 2023

OskarStark Sep 29, 2023

Choose a reason for hiding this comment

igrizzli Sep 29, 2023

Choose a reason for hiding this comment

igrizzli Sep 29, 2023

Choose a reason for hiding this comment

OskarStark Sep 29, 2023

Choose a reason for hiding this comment

OskarStark commented Sep 29, 2023

igrizzli commented Oct 1, 2023 • edited Loading

nicolas-grekas Oct 2, 2023

Choose a reason for hiding this comment

nicolas-grekas Oct 2, 2023

Choose a reason for hiding this comment

OskarStark commented Oct 9, 2023

fabpot commented Oct 10, 2023

[Notifier] [Telegram] Extend options for `location`, `document`, `audio`, `video`, `venue`, `photo`, `animation`, `sticker` & `contact` #51717

[Notifier] [Telegram] Extend options for `location`, `document`, `audio`, `video`, `venue`, `photo`, `animation`, `sticker` & `contact` #51717

igrizzli commented Sep 22, 2023 •

edited

Loading

igrizzli commented Sep 28, 2023 •

edited

Loading

igrizzli commented Oct 1, 2023 •

edited

Loading