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

Fix message rendering bug of MS Teams in potential actions #242

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ArcticXWolf
Copy link

Hello,

since some days a couple of our messagecards are not rendered anymore in updated MS Teams with the following error message:

image

I've debugged the issue and apparently Microsoft changed the schema to require the actions field in PotentialActionActionCard.

To reproduce, the following messagecard is not rendered in the new MS Teams (just send it against a webhook):

{
    "@type": "MessageCard",
    "@context": "https://schema.org/extensions",
    "summary": "Test",
    "title": "Test",
    "themeColor": "FFFF00",
    "potentialAction": [
        {
            "@type": "ActionCard",
            "name": "Show",
            "inputs": [
                {
                    "@type": "TextInput",
                    "id": "textinput",
                    "title": "Textinput"
                }
            ]
        }
    ]
}

While this one is:

{
    "@type": "MessageCard",
    "@context": "https://schema.org/extensions",
    "summary": "Test",
    "title": "Test",
    "themeColor": "FFFF00",
    "potentialAction": [
        {
            "@type": "ActionCard",
            "name": "Show",
            "inputs": [
                {
                    "@type": "TextInput",
                    "id": "textinput",
                    "title": "Textinput"
                }
            ],
            "actions": []
        }
    ]
}

The PR should fix the issue, however I could not test it properly (since I failed to hook up my fork of this library to my application, I am not very versed in Golang).

@ArcticXWolf
Copy link
Author

Ah, if you need the Teams version that is affected by this bug:
"You have Microsoft Teams Version 1.6.00.28557 (64-bit). It was last updated on 02.11.2023."

@atc0005 atc0005 self-assigned this Nov 3, 2023
@atc0005 atc0005 added bug Something isn't working card format/adaptivecard Adaptive Card support labels Nov 3, 2023
@atc0005 atc0005 added this to the v2.8.1 milestone Nov 3, 2023
@atc0005 atc0005 added card format/messagecard MessageCard (aka, "legacy actionable message card") support and removed card format/adaptivecard Adaptive Card support labels Nov 3, 2023
@atc0005
Copy link
Owner

atc0005 commented Nov 3, 2023

@ArcticXWolf: The PR should fix the issue

Thanks for submitting this PR.

I've looked at the Adaptive Card format and it doesn't appear to have the same requirement that you've encountered for the MessageCard format. I'm currently digging for a reference that we can use to document the requirement change you've encountered. If you come across it, please let me know.

I could not test it properly (since I failed to hook up my fork of this library to my application, I am not very versed in Golang).

Not a problem.

You can update the go.mod file for your application to reference your fork and the specific commit you want to use and then build using that alternative dependency.

That process would look something like:

cd /path/to/your/application
go mod edit -replace github.com/atc0005/go-teams-notify/v2=github.com/ArcticXWolf/go-teams-notify/v2@fe4c92373b031a9338caebf8ac850c6b6a608537
go mod tidy

and then build your application and test.

I used the commit hash in the above example based on this commit:

EDIT: I used https://www.jvt.me/posts/2022/07/07/go-mod-fork/ as a reference for the replace directive. I've not done this in a while and that blog post was a great find.

@ArcticXWolf
Copy link
Author

ArcticXWolf commented Nov 3, 2023

I've looked at the Adaptive Card format and it doesn't appear to have the same requirement that you've encountered for the MessageCard format. I'm currently digging for a reference that we can use to document the requirement change you've encountered. If you come across it, please let me know.

I've looked at those docs too, but sadly they neither documented any changes, nor do they specify for all fields if they are required or not. My suspicion is that this is a regression on the side of MS Teams, that they introduced this bug during their MS Teams rewrite. They probably did not expect anyone to use those potentialActionActionCards without any action, during rendering they just access the field without null-checking and then the renderer fails.

Sadly I was not able to see anything in the MS Teams logs (they are really big).

You can update the go.mod file for your application to reference your fork and the specific commit you want to use and then build using that alternative dependency.
...
EDIT: I used https://www.jvt.me/posts/2022/07/07/go-mod-fork/ as a reference for the replace directive. I've not done this in a while and that blog post was a great find.

Thanks a lot, that works! The PR works and solves the problem, however there is a caveat. You must specify the empty array in the PotentialActionActionCard, without it it renders null and that does not work:

Does not work:

messagecard.PotentialAction{
  {
	  Type: "ActionCard",
	  Name: "Show check output",
	  PotentialActionActionCard: messagecard.PotentialActionActionCard{
		  Inputs: []messagecard.PotentialActionActionCardInput{
			  {
				  ID:   "check-output",
				  Type: "TextInput",
				  PotentialActionActionCardInputTextInput: messagecard.PotentialActionActionCardInputTextInput{
					  IsMultiline: true,
				  },
				  Title: "Check Output",
			  },
		  },
	  },
  },
}

// Renders to:
"potentialAction": [
                {
                        "@type": "ActionCard",
                        "name": "Show check output",
                        "inputs": [
                                {
                                        "@type": "TextInput",
                                        "id": "check-output",
                                        "title": "Check Output",
                                        "isMultiline": true
                                }
                        ],
                        "actions": null
                },
]

Works:

messagecard.PotentialAction{
  {
	  Type: "ActionCard",
	  Name: "Show check output",
	  PotentialActionActionCard: messagecard.PotentialActionActionCard{
		  Inputs: []messagecard.PotentialActionActionCardInput{
			  {
				  ID:   "check-output",
				  Type: "TextInput",
				  PotentialActionActionCardInputTextInput: messagecard.PotentialActionActionCardInputTextInput{
					  IsMultiline: true,
				  },
				  Title: "Check Output",
			  },
		  },
		  Actions: []messagecard.PotentialActionActionCardAction{}, // <----------------------------------- THIS HERE
	  },
  },
}

// Renders to:
"potentialAction": [
                {
                        "@type": "ActionCard",
                        "name": "Show check output",
                        "inputs": [
                                {
                                        "@type": "TextInput",
                                        "id": "check-output",
                                        "title": "Check Output",
                                        "isMultiline": true
                                }
                        ],
                        "actions": []
                },
]

I dont know if there is something we can do with Golang Json Tags to make this more convenient.

EDIT: The linting error seems to concern unchanged code, so I leave those intact?

@atc0005
Copy link
Owner

atc0005 commented Nov 6, 2023

EDIT: The linting error seems to concern unchanged code, so I leave those intact?

More info below, but the short version is that the linting errors identified are existing issues, so nothing you need to worry with.


I opted to setup CI tasks using a set of "unstable", "stable" and "oldstable" images. Those images mapped to Go versions with similar labeling. Rather confusingly (I now see), I opted to apply some additional linters in the unstable image to test them before including them within the "stable" and "oldstable" images.

I apply those same CI tasks across all Go projects that I work with. While I've marked the "unstable" image as not required (not blocking merges, not failing CI jobs), having the task failure indication listed alongside the other results can be misleading.

I recently learned that there is a golangci-lint flag to indicate that it should only evaluate changes (and not the full files). I hope to investigate that further as it would make linting output much easier to review.

@ArcticXWolf
Copy link
Author

Ahh, good to know, thanks for the explanation.

As an aside: In the AdaptiveCard documentation by Microsoft they write "The MessageCard format is still supported but is now de-emphasized. Office connectors and Microsoft Teams connectors do not currently support the Adaptive Card format." [1] However from the examples in this repo I get the impression that you CAN send AdaptiveCards to a Teams Webhook. Is that correct? If not, where do you use this librarys AdaptiveCards?

[1] https://learn.microsoft.com/en-us/outlook/actionable-messages/adaptive-card

@atc0005
Copy link
Owner

atc0005 commented Nov 6, 2023

@ArcticXWolf: My suspicion is that this is a regression on the side of MS Teams, that they introduced this bug during their MS Teams rewrite. They probably did not expect anyone to use those potentialActionActionCards without any action, during rendering they just access the field without null-checking and then the renderer fails.

This makes sense. If you later learn of any published notes about this please share and we can reference it from documentation in this project.

Does not work:

messagecard.PotentialAction{
  {
	  Type: "ActionCard",
	  Name: "Show check output",
	  PotentialActionActionCard: messagecard.PotentialActionActionCard{
		  Inputs: []messagecard.PotentialActionActionCardInput{
			  {
				  ID:   "check-output",
				  Type: "TextInput",
				  PotentialActionActionCardInputTextInput: messagecard.PotentialActionActionCardInputTextInput{
					  IsMultiline: true,
				  },
				  Title: "Check Output",
			  },
		  },
	  },
  },
}

Are you writing this code manually or using the library? If using the library, can you share some example code?

Works:

messagecard.PotentialAction{
  {
	  Type: "ActionCard",
	  Name: "Show check output",
	  PotentialActionActionCard: messagecard.PotentialActionActionCard{
		  Inputs: []messagecard.PotentialActionActionCardInput{
			  {
				  ID:   "check-output",
				  Type: "TextInput",
				  PotentialActionActionCardInputTextInput: messagecard.PotentialActionActionCardInputTextInput{
					  IsMultiline: true,
				  },
				  Title: "Check Output",
			  },
		  },
		  Actions: []messagecard.PotentialActionActionCardAction{}, // <----------------------------------- THIS HERE
	  },
  },
}

You must specify the empty array in the PotentialActionActionCard, without it it renders null and that does not work

I had to refresh my memory, but this looks to be the way Go JSON support works.

Refs:

Either the library needs to provide construction behavior to ensure that the Actions field is non-nil/non-empty or the client code constructing the type(s) needs to do so.

The current validation behavior of the library may also need to be extended to cover this scenario.

@ArcticXWolf
Copy link
Author

Are you writing this code manually or using the library? If using the library, can you share some example code?

It is basically an excerpt from my application here: https://github.com/ArcticXWolf/sensu-teams-handler/blob/master/cmd/sensu-teams-handler/main.go#L135
It is a connector for a monitoring system called sensu that sends MS Teams notifications. If you want to test it (without setting up the monitoring system), then you can pull that repo, set a WEBHOOK via export TEAMS_WEBHOOK_URL="..." and just run make test. It uses a dummy message of the monitoring system, constructs the MessageCard and sends it against the Webhook.

I've built a workaround for the current problem by just giving all PotentialActionCard a dummy button opening a webpage.

Either the library needs to provide construction behavior to ensure that the Actions field is non-nil/non-empty or the client code constructing the type(s) needs to do so.

We could either leave the PR like it is now or I believe we could also overwrite the MarshalJSON behavior of PotentialActionCard to check for null and insert an empty array.

The current validation behavior of the library may also need to be extended to cover this scenario.

You mean here? https://github.com/atc0005/go-teams-notify/blob/master/messagecard/messagecard.go#L419

@atc0005
Copy link
Owner

atc0005 commented Nov 6, 2023

Ahh, good to know, thanks for the explanation.

Welcome.

As an aside: In the AdaptiveCard documentation by Microsoft they write "The MessageCard format is still supported but is now de-emphasized. Office connectors and Microsoft Teams connectors do not currently support the Adaptive Card format." [1] However from the examples in this repo I get the impression that you CAN send AdaptiveCards to a Teams Webhook. Is that correct?

I've found that some of the documentation for the two formats is out of sync. In some cases it will note that something is not supported, but in another note that it is. I found this to be quite frustrating when I was trying to determine what level of support was available when submitting messages via webhook connnectors.

In short, yes, sending AdaptiveCards to Teams via a webhook connector is supported.

From this page:

Adaptive Cards can be sent through Incoming Webhooks. For more information, see Send Adaptive Cards using Incoming Webhooks.

adaptive cards can be sent through incoming webhooks

Some references:

Additional notes are included here:

Seeing how they render here, I see that I need to update the formatting to prevent them from running together in a big text blob.

If not, where do you use this librarys AdaptiveCards?

Some examples:

If you're asking for where I personally use it, here is a small CLI app I wrote which uses this library:

I switched that app over from generating messages using the MessageCard format to AdaptiveCard format once support for the newer message format was added here.

All of this said, if you can supply code where you generate the working and not working examples you posted we can evaluate how best to resolve the issue you encountered for the MessageCard format.

@ArcticXWolf
Copy link
Author

ArcticXWolf commented Nov 6, 2023

Wow, thanks for all this detailed information! Sad that the documentation is so neglected on their part :/ (Same goes by the way concerning the maximum character length of a JSON message to a Webhook. I've had multiple requests denied without a good return code because my MessageCard was too long.)

I can put together a minimal Go application that sends both working and non-working examples to a webhook later this week. For me, this PR alone would solve the whole issue, it is just that we should think about documentation and the usability for other devs (concerning the null vs empty array).

[EDIT: And long-term I will rewrite my application to use AdaptiveCards then. Apparently Microsoft wants to phase out MessageCard someday.]

@atc0005
Copy link
Owner

atc0005 commented Nov 6, 2023

I've built a workaround for the current problem by just giving all PotentialActionCard a dummy button opening a webpage.

From this response (https://stackoverflow.com/a/56201087/903870):

// option 1
productConstructed.BrandMedals = make([]string, 0)

// option 2
productConstructed.BrandMedals = []string{} 

This may work well if you are just wanting to ensure that the field is initialized (and not provide a placeholder/dummy button).

We could either leave the PR like it is now or I believe we could also overwrite the MarshalJSON behavior of PotentialActionCard to check for null and insert an empty array.

That is a good point.

I'm not sure what the best approach would be:

  • silently take that step and work around the issue
  • have validation catch the issue and report it back to client code to handle

The first seems to be more flexible.

Perhaps the best next step is to accept the PR as it is, file a GH issue to summarize the problem (heavily quoting from provided notes here) and then consider further steps.

@ArcticXWolf
Copy link
Author

I have created a bug report at MS Teams in the hope of either getting updated docs, fixing of the bug or at least an official response :)
I hope that was the correct github repo for MS Teams.

MicrosoftDocs/msteams-docs#9858

@atc0005
Copy link
Owner

atc0005 commented Nov 8, 2023

@ArcticXWolf That's a solid bug report. Thanks for filing that and linking to it. I've subscribed to that GH issue for further updates.

My apologies also; I worded my "next steps" post poorly.

When I said this:

file a GH issue to summarize the problem (heavily quoting from provided notes here)

I was (unclearly) making a note to remind myself what I needed to do before considering this complete. I wasn't passing that off to you to handle and my apologies if it came across that way.

The PR is useful on its own, but further work is needed to resolve the issue and I wanted to collect those details on a GH issue in this repo for further evaluation.

Your suggestion here:

We could either leave the PR like it is now or I believe we could also overwrite the MarshalJSON behavior of PotentialActionCard to check for null and insert an empty array.

may be needed if Microsoft determines that the recent change on their end is intentional/correct. I think we can likely add that as a follow-up PR once we hear further from Microsoft on MicrosoftDocs/msteams-docs#9858.

Thoughts?

@ArcticXWolf
Copy link
Author

ArcticXWolf commented Nov 8, 2023

Oh, no worries. I did not create this issue because of your post, but because we had other issues with the Teams Update anyway and also so that we maybe get some official documentation or statement concerning this problem.

The communication with you here has been great :)

And I agree, we should wait to see if Microsoft reverts their changes or determines that the current behavior is intentional now.
For the meantime we have the workaround of just adding a dummy button to the actions.


Also as a small side info, since you posted that workaround:

From this response (https://stackoverflow.com/a/56201087/903870):

// option 1
productConstructed.BrandMedals = make([]string, 0)

// option 2
productConstructed.BrandMedals = []string{} 

This did not work for me with the non-PR version of this library. Apparently the omitempty tag will strip an empty array anyway during marshalling. So the idea you showed here only works if you apply my PR.

@atc0005
Copy link
Owner

atc0005 commented Nov 8, 2023

Also as a small side info, since you posted that workaround:

From this response (https://stackoverflow.com/a/56201087/903870):

// option 1
productConstructed.BrandMedals = make([]string, 0)

// option 2
productConstructed.BrandMedals = []string{} 

This did not work for me with the non-PR version of this library. Apparently the omitempty tag will strip an empty array anyway during marshalling. So the idea you showed here only works if you apply my PR.

That's what I meant, just didn't communicate it well. After the PR is merged the workaround I noted was to be used in place of providing a button that you might not otherwise wish to to include in the MS Teams payload.

@atc0005 atc0005 modified the milestones: v2.9.1, Future Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working card format/messagecard MessageCard (aka, "legacy actionable message card") support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants