Maxim/bring heartbeats back to UI #2550
Conversation
maskin25
added
the
pr:no public docs
There was a problem hiding this comment.
thanks for helping out with this 🙏
I checked things locally and it mostly looks good to me. Few comments/questions.
- Can we add some sort of confirmation popup notification when you click "Update" in the heartbeat settings drawer? imo, at the moment it's not very clear to the user that anything has happened.
- I think this is worth adding basic e2e tests for. Without something like this, there's nothing to prevent us from regressing into the broken state again. For example we could test that you're able to update an integration's heartbeat interval + are able to send a request to the integration's heartbeat URL + see that in the UI this is represented to you as the green heart symbol. Just some ideas. There are similar e2e tests here for testing an integration's maintenance mode.
Which type of confirmation are you asking about? Like below or just green notification at the right top corner? 
Good idea, I'll add |
The green one would be nice to show the user things have been saved successfully. Not sure if we also want to close the drawer on save? |
| this.items = { | ||
| ...this.items, | ||
| [id]: alertReceiveChannel, | ||
| [id]: omit(alertReceiveChannel, 'heartbeat'), |
There was a problem hiding this comment.
can we store heartbeat on the objects within items? why do we need alertReceiveChannelToHeartbeat`? I believe a heartbeat always belongs to a single alert receive channel.
There was a problem hiding this comment.
in my mind heartbeat is a separate entity (it even has it's own id and ideally it should have separate endpoint), that's why we have 'models/heartbeat', we need alertReceiveChannelToHeartbeat to save appropriate heartbeat Id for each alert receive channel, it's one to one mapping. Yes we can store heartbeat id just in alert receive channel object, but I did not want to pollute alert receive channel object
|
@joeyorlando I've added a couple of integration tests for heartbeats and green notification on heartbeat settings update |
| OnCall, do the following: | ||
| <span | ||
| dangerouslySetInnerHTML={{ | ||
| __html: heartbeat.instruction, |
There was a problem hiding this comment.
This instructions are moved to the docs. No need to add them here as they will be removed from the API https://grafana.com/docs/oncall/latest/integrations/alertmanager/#configuring-oncall-heartbeats-optional
# What this PR does Bring heartbeats back to UI ## Which issue(s) this PR fixes ## Checklist - [ ] Unit, integration, and e2e (if applicable) tests updated - [ ] Documentation added (or `pr:no public docs` PR label added if not required) - [ ] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required) --------- Co-authored-by: Joey Orlando <joey.orlando@grafana.com>
What this PR does
Bring heartbeats back to UI
Which issue(s) this PR fixes
Checklist
pr:no public docsPR label added if not required)CHANGELOG.mdupdated (orpr:no changelogPR label added if not required)