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

Fix wrong event subscription in CommunicationManager #3529

Merged
merged 2 commits into from
Apr 7, 2023
Merged

Fix wrong event subscription in CommunicationManager #3529

merged 2 commits into from
Apr 7, 2023

Conversation

florian-h05
Copy link
Contributor

#3141 introduced an ItemStateUpdatedEvent and updated CommunicationManager to only receive updates on this event.
However, it was forgotten to update the subscribed event types, so no updated were received at all.

The CommuncationManagerOSGiTest was not able to detect this regression, because in the test the event is received via a method call, but not an event subscription/the event bus.

This also renames the method for creating a GroupStateUpdatedEvent for more clarity.

#3141 introduced an ItemStateUpdatedEvent and updated CommunicationManager to require this for sending state updates.
However, it was forgotten to update the subscribed event types.

Signed-off-by: Florian Hotze <[email protected]>
@J-N-K
Copy link
Member

J-N-K commented Apr 7, 2023

LGTM, why is this a draft?

@florian-h05
Copy link
Contributor Author

I wanted to test this on my production, but I am finished now 👍

This fixes openhab/openhab-addons#14753 according to my tests, which is logical: state updates are now again relayed to the thing and therefore send to the KNX bus.

@florian-h05 florian-h05 marked this pull request as ready for review April 7, 2023 11:01
@florian-h05 florian-h05 requested a review from a team as a code owner April 7, 2023 11:01
@J-N-K J-N-K added bug An unexpected problem or unintended behavior of the Core regression labels Apr 7, 2023
Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@florian-h05
Copy link
Contributor Author

The CI / Build action seems unstable, it already failed two times because of the org.openhab.core.internal.service.WatchServiceImplTest.

@J-N-K
Copy link
Member

J-N-K commented Apr 7, 2023

CI will be more stable after #3518 is merged.

@florian-h05
Copy link
Contributor Author

CI finally succeeded 🥳

@J-N-K J-N-K merged commit 633002c into openhab:main Apr 7, 2023
@J-N-K J-N-K added this to the 4.0 milestone Apr 7, 2023
@florian-h05 florian-h05 deleted the fix-event-subscr branch April 7, 2023 12:27
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
* Fix wrong event subscription in CommunicationManager

Signed-off-by: Florian Hotze <[email protected]>
GitOrigin-RevId: 633002c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of the Core regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants