Skip to content

Conversation

DaanMeijer
Copy link
Contributor

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Fixes #3898
Implement the Slack App way of posting messages, as well as allow for the updating of previous messages. Features a complete restructure of the way Slack messages get created

Type of change

  • User interface (UI)
  • New feature (non-breaking change which adds functionality)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings

Screenshots (if any)

@DaanMeijer DaanMeijer changed the title Feature/slack alarm Feature: allow slack messages to be updated Oct 15, 2023
@DaanMeijer

This comment was marked as outdated.

@louislam
Copy link
Owner

Please note that some changes may be duplicate of #3394.

@louislam louislam added this to the 2.1.0 milestone Oct 16, 2023
@DaanMeijer
Copy link
Contributor Author

Please note that some changes may be duplicate of #3394.

I could remove the part regarding duration. It's not necessary for this functionality, but I thought it would be a nice bonus (and it takes into account the possibility of the duration being unknown, because of the limitations with heartbeats being stored for a limited time).

@DaanMeijer
Copy link
Contributor Author

What's also worth noting, is that this pull request is based on #3886, as it expands on the implemented functionality in that pull request.

@chakflying chakflying added the A:notifications Issues or PRs related to notifications label Dec 2, 2023
@CommanderStorm CommanderStorm added the pr:please address review comments this PR needs a bit more work to be mergable label May 19, 2024
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Please note that some changes may be duplicate of #3394.

I could remove the part regarding duration. It's not necessary for this functionality, but I thought it would be a nice bonus

Sorry for the late review. After rereading the content, the messaging here was not clear.

=> Please remove the changes regarding the duration.
This feature should be handled at a higher level and not just in this monitor, making future work more difficult

Also note that there are a lot of changes in this PR. I don't think most are intentional => please remove those who are not

@CommanderStorm CommanderStorm marked this pull request as draft May 19, 2024 21:44
@github-actions github-actions bot added pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again and removed pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again labels May 19, 2024
@github-actions github-actions bot added pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again and removed pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again labels May 23, 2024
@DaanMeijer DaanMeijer force-pushed the feature/slack-alarm branch from d83b897 to bb43e6c Compare October 8, 2024 13:02
@DaanMeijer DaanMeijer marked this pull request as ready for review October 8, 2024 13:10
@DaanMeijer
Copy link
Contributor Author

@CommanderStorm I've restructured the code for this pull request to be more legible. There is a lot of code, which actually is necessary, because this feature requires a different method of authenticating with slack.

@matttheus
Copy link

What is the status of this PR?

@CommanderStorm CommanderStorm added pr:needs review this PR needs a review by maintainers or other community members and removed pr:please address review comments this PR needs a bit more work to be mergable labels Oct 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A:notifications Issues or PRs related to notifications needs:resolve-merge-conflict pr:needs review this PR needs a review by maintainers or other community members pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[slack] Updating messages instead of posting new ones

5 participants