NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Notifier] [Bridge] Fix missed messageId for SendMessage object in slack notifier#40955

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:5.2fromWaylandAce:feature/slack_bridge_notifier_message_id

May 7, 2021

Merged

[Notifier] [Bridge] Fix missed messageId for SendMessage object in slack notifier#40955

nicolas-grekas merged 1 commit intosymfony:5.2fromWaylandAce:feature/slack_bridge_notifier_message_id

May 7, 2021

Conversation

Copy link

Contributor

WaylandAce commentedApr 27, 2021

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

There are missed messageId property for SendMessage object in slack notifier.
Regarding slack's documentation:https://api.slack.com/messaging/sending#publishing

One very important piece of information in this response is thets value, which is essentially the ID of the message,

WaylandAce requested a review fromOskarStark as acode owner

April 27, 2021 10:30

carsonbot added Status: Needs Review Bug Notifier labels

Apr 27, 2021

carsonbot added this to the5.2 milestone

Apr 27, 2021

WaylandAce changed the title~~[Notifier] [Bridge] Store message id for slack transport's SendMessage~~[Notifier] [Bridge] Fix missed messageId for SendMessage object in slack notifier

Apr 27, 2021

WaylandAce force-pushed thefeature/slack_bridge_notifier_message_id branch from3fae769 to374788dCompare

April 27, 2021 10:39

OskarStark reviewed

Apr 27, 2021

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Slack/SlackTransport.php


		returnnewSentMessage($message, (string)$this);
		$sentMessage =newSentMessage($message, (string)$this);
		$sentMessage->setMessageId($result['ts']);

Copy link

Contributor

Choose a reason for hiding this comment

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

Shall we check if the key exists in the array?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

We already check for "ok" status. I had looked to another transports/bridges, they did not have even this checks.

Which behavior do you expect, if by some unknown reason slack return response with ok status, but with no messageid specified?

Copy link

Contributor

Choose a reason for hiding this comment

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

Yes, but ok let's keep it

OskarStark reviewed

Apr 27, 2021

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Slack/Tests/SlackTransportTest.php OutdatedShow resolvedHide resolved

OskarStark reviewed

Apr 27, 2021

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Slack/Tests/SlackTransportTest.php OutdatedShow resolvedHide resolved

Copy link

Contributor

OskarStark commentedApr 27, 2021

I would consider this a feature, it is working without the ID and must wait for5.4.

cc@nicolas-grekas

[Notifier] [Bridge] Store message id for slack transport's SendMessage

838f36b

WaylandAce force-pushed thefeature/slack_bridge_notifier_message_id branch from374788d to838f36bCompare

April 27, 2021 17:17

Copy link

ContributorAuthor

WaylandAce commentedApr 27, 2021

I would consider this a feature, it is working without the ID and must wait for5.4.
cc@nicolas-grekas

I though to put this change as usual to 5.x branch, but checked "Maintenance" page (https://symfony.com/doc/current/contributing/code/maintenance.html) and it's sounds like it matches the rules.
Also 1 point to include this featurefix to nearest release, is the fact, that we cant fetch message id to reply/start thread in slack and this can be seen as problem.