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

Alerting: Fixing mobile notifications in Microsoft Teams #11484

Merged
merged 2 commits into from
Jun 14, 2018

Conversation

manacker
Copy link
Contributor

@manacker manacker commented Apr 4, 2018

Apparently the summary field in a Microsoft Teams webhook request field gets used for the text displayed in mobile push notifications. If that field contains only whitespace the push notification displays the nonsensical text

Card - access it on https://go.skype.com/cards.unsupported

Therefore this commit mirrors the notification title in the summary field.
It also removes the unsupported fields Mention and Recipient from the notifier data struct.

@codecov-io
Copy link

codecov-io commented Apr 4, 2018

Codecov Report

Merging #11484 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #11484      +/-   ##
==========================================
- Coverage    51.9%   51.85%   -0.05%     
==========================================
  Files         359      359              
  Lines       26066    26058       -8     
  Branches     1509     1508       -1     
==========================================
- Hits        13530    13513      -17     
- Misses      11796    11803       +7     
- Partials      740      742       +2

@torkelo
Copy link
Member

torkelo commented May 8, 2018

looks strange to have summary and title be the same (Is title shown in mobile notifications), should summary not use message when there is a message and some fallback like "NA" when no message (so no empty string). Is summary used in other clients?

@manacker
Copy link
Contributor Author

manacker commented May 8, 2018

Summary and title seem to be used in different contexts:
teams-alert
Summary seems to be used for the activity feed and notifications, while title is used to render the actual chat message.
Here are the iOS notifications top is with empty summary, bottom with a non-empty summary.
img_8009

Hence I think mirroring the title is better than giving no summary at all.

@manacker
Copy link
Contributor Author

manacker commented Jun 7, 2018

@torkelo Do you need anything else from me to get this PR merged?

@marefr marefr merged commit 81d3413 into grafana:master Jun 14, 2018
@marefr
Copy link
Contributor

marefr commented Jun 14, 2018

Thank you for contributing to Grafana!

@marefr marefr added this to the 5.2 milestone Jun 14, 2018
marefr added a commit that referenced this pull request Jun 14, 2018
bergquist added a commit that referenced this pull request Jun 15, 2018
* master: (84 commits)
  docs: adds info about grafana-dev container
  changelog: add notes about closing #12282
  Added Litre/min and milliLitre/min in Flow (#12282)
  remove papaparse dependency
  list name is deleteDatasources, not delete_datasources
  remove internal influx ifql datasource
  Document the endpoint for deleting an org
  tests: rewrite into table tests
  influxdb: adds mode func to backend
  changelog: add notes about closing #11484
  changelog: add notes about closing #11233
  Remove import
  Fix PR feedback
  Removed papaparse from external plugin exports
  Karma to Jest: query_builder
  dsproxy: move http client variable back
  Karma to Jest: threshold_mapper
  Expose react and slate to external plugins
  Karma to Jest: threshold_manager
  Karma to Jest: query_def, index_pattern
  ...
@ying-jeanne ying-jeanne added the pr/external This PR is from external contributor label Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/external This PR is from external contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants