-
Notifications
You must be signed in to change notification settings - Fork 17
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
base: master
Are you sure you want to change the base?
Fix message rendering bug of MS Teams in potential actions #242
Conversation
Ah, if you need the Teams version that is affected by this bug: |
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.
Not a problem. You can update the 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 |
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).
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:
Works:
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? |
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. |
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 |
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.
Are you writing this code manually or using the library? If using the library, can you share some example code?
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 The current validation behavior of the library may also need to be extended to cover this scenario. |
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 I've built a workaround for the current problem by just giving all PotentialActionCard a dummy button opening a webpage.
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.
You mean here? https://github.com/atc0005/go-teams-notify/blob/master/messagecard/messagecard.go#L419 |
Welcome.
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:
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.
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. |
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.] |
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).
That is a good point. I'm not sure what the best approach would be:
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. |
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 :) |
@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:
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:
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? |
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. Also as a small side info, since you posted that workaround:
This did not work for me with the non-PR version of this library. Apparently the |
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. |
Hello,
since some days a couple of our messagecards are not rendered anymore in updated MS Teams with the following error message:
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):
While this one is:
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).