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

[actions] Add notification builder & Mark some actions deprecated as replaced by better APIs #351

Merged
merged 10 commits into from
Jun 28, 2024

Conversation

florian-h05
Copy link
Contributor

@florian-h05 florian-h05 commented Jun 26, 2024

This adds builders to create broadcast, log and standard notifications and allow easy usage of the new functionality provided by openhab/openhab-addons#16934 without having to deal with method param orders.

It also marks several actions as deprecated, because their functionality was replaced by better and often pure JS APIs.

…some actions

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Copy link
Contributor Author

florian-h05 commented Jun 26, 2024

To Dos:

  • Add JSDoc for onClickAction, actionButton1 etc. once action format docs have been released.
  • Extend README for onClickAction, actionButton1 etc. once action format docs have been released.
  • Regenerate type defs.

@digitaldan
Copy link
Contributor

Awesome !

README.md Outdated
Comment on lines 797 to 800
- `.logNotificationBuilder(message)`: Creates a builder to send a log notification with the given message.
- `.withIcon(icon)`: Sets the icon of the notification.
- `.withSeverity(link)`: Sets the severity of the notification.
- .`send()`: Sends the notification.
Copy link
Member

Choose a reason for hiding this comment

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

I'd replace this builder with a function logOnly() on notificationBuilder(). This way the media attachment can be shown in the notification list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But please note that media attachment cannot be set for log notifications with the current NotificationActions in openhab/openhab-addons#16934.

Copy link
Member

Choose a reason for hiding this comment

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

@digitaldan Maybe drop the three user-facing types (log, broadcast, normal) completely and make everything a "normal" notification? Then log notifications are just like normal ones with the same properties, but don't push.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Copy link
Contributor Author

@mueller-ma I have implemented your proposals, updated the unit tests, updates the README and added the missing JSDoc.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05 florian-h05 marked this pull request as ready for review June 26, 2024 18:43
@florian-h05 florian-h05 requested a review from a team as a code owner June 26, 2024 18:43
@florian-h05
Copy link
Contributor Author

@digitaldan Do you want to review or should I self-merge?

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05 florian-h05 added the enhancement New feature or request label Jun 26, 2024
@florian-h05 florian-h05 added this to the to be released milestone Jun 26, 2024
@digitaldan
Copy link
Contributor

LGTM, thanks !!!!!!

@florian-h05
Copy link
Contributor Author

I will test this soon in production and then publish a new release and update the add-on.

@florian-h05
Copy link
Contributor Author

Works fine for me, however I can only test the message due to client limitations (iOS App).
Would be great if you could give it a try once I published a new version.

@florian-h05 florian-h05 merged commit 9ec692c into openhab:main Jun 28, 2024
4 checks passed
@florian-h05 florian-h05 deleted the notification-builder branch June 28, 2024 05:57
@florian-h05 florian-h05 changed the title [actions] Add notification builders & Mark some actions deprecated as replaced by better APIs [actions] Add notification builder & Mark some actions deprecated as replaced by better APIs Jun 28, 2024
@digitaldan
Copy link
Contributor

Will try it today! I'm hoping to get a test flight build soon of the new IOS app.

florian-h05 added a commit that referenced this pull request Jul 3, 2024
…ications (#353)

Follow-up for #351.
Refs openhab/openhab-addons#16934.

- Renames severity to tag.
- Adds support for specifying `referenceId`.
- Adds support for hiding notifications.

---------

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants