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

Support msteams workflows #10

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

huNt-FMJ
Copy link
Collaborator

@huNt-FMJ huNt-FMJ commented Dec 5, 2024

Recently we got to know the world of Microsoft Teams workflows.
After playing around a bit, I found that this Gem still works successfully when wanting to post a message using Microsoft's adaptive card format.

However, some minor tweaks are required.
With the introduction/replacement/migration of connector based webhooks to workflow based webhooks, both types of responses. This is because workflow based webhook responses have different properties, i.e. status code and body content.
In a previous implementation, we only had to consider the properties of a connector based webhook responses.
these can be found here.

My current understanding is that each response type has the following properties:

  1. Connector based webhooks
  1. Workflow based webhooks
  • response code = 202
  • response body = "" (empty string)

Adds two helper functions that encapsulate the properties of a
successful connector based webhook and a workflow based webhook.

Up to now, workflow based webhooks lead to a response where there
response code is 202 and the response body is empty.
For connector based webhooks, the status code is 200 and the response
body is set to "1".
@huNt-FMJ huNt-FMJ requested a review from mooikos December 5, 2024 14:56
# Both types must be considered differently to support both types of
# MSTeams webhooks.
##
module MsTeamsWebhookType
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am personally not a fan of multiple modules / classes inside the same file

So I would rather either:

  • move this to a own file
    • it should probably be still inside the MsTeamsHermes module (this is the namespace of the gem itself)
    • it can than be included (using include) as functionalities for class Message
  • (I prefer this one since there is not so much code) make them private methods of class Message

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I'll think about it a bit.
With regards to the second point, I also thought about putting these methods into the message class, but I think it is not its business to handle the response at all. I would even delegate the network call to some service class, but that it not the architecture we currently have.

##
module MsTeamsWebhookType
def self.mst_workflow_webhook_response?(response)
response.code == "202" and response.body.empty?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think rubocop would most likely complain about this,
is rather standard to use && instead

Copy link
Collaborator Author

@huNt-FMJ huNt-FMJ Dec 6, 2024

Choose a reason for hiding this comment

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

I think I checked with Rubocop and it might even be setup in the GitHub actions.
No complaint yet.


raise UnknownError, response.body
end
raise MessageBodyTooLargeError, body_json.bytesize if response.body.include? MSTEAMS_MESSAGE_413_ERROR_TOKEN
Copy link
Collaborator

Choose a reason for hiding this comment

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

I perceive this as a bit inconsistent with the above early returns
Since above we are checking the content of response.body inside dedicated methods,
Here instead we are checking it directly in this method

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see. That's true. I'll see what I can come up with.

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.

2 participants