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 missing stateDescriptionProvider in Group items #3144

Merged
merged 3 commits into from
Nov 6, 2022

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Nov 6, 2022

Fixes #2449

The stateDescriptionProvider was not properly passed to the base item. Because of that units set in the state description were not properly used when setting the griup state after calculating aggregation functions.

Signed-off-by: Jan N. Klug github@klug.nrw

The stateDescriptionProvider was not properly passed to the base item. Because of that units set in the state description were not properly used when setting the griup state after calculating aggregation functions.

Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K J-N-K added the bug An unexpected problem or unintended behavior of the Core label Nov 6, 2022
@J-N-K J-N-K requested a review from a team as a code owner November 6, 2022 14:05
@cweitkamp
Copy link
Contributor

Nice catch. I am wondering if we should override the getter methods too and return the baseItems StateDescriptionor Command provider?

@J-N-K
Copy link
Member Author

J-N-K commented Nov 6, 2022

I guess that's not needed as they are the same. The base item is not allowed to be accessed except for instance checks (according to the JavaDoc).

@cweitkamp
Copy link
Contributor

cweitkamp commented Nov 6, 2022

Hm, are you sure? If we override the setter and do not set the internal property it will be null. Wouldn't it?

In ItemRegistryImplTest is a nice place to add a test case.

public void assertCommandDescriptionServiceGetsInjected() {

@cweitkamp
Copy link
Contributor

Forget my comment. I missed the super calls.

@J-N-K
Copy link
Member Author

J-N-K commented Nov 6, 2022

But we call super.setXXX, which sets XXX to the group itself.

Copy link
Contributor

@cweitkamp cweitkamp 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.

Will you add a unit test?

J-N-K added 2 commits November 6, 2022 19:53
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
@cweitkamp cweitkamp added this to the 3.4 milestone Nov 6, 2022
@cweitkamp cweitkamp merged commit 2bceba6 into openhab:main Nov 6, 2022
@J-N-K J-N-K deleted the bug-fixmissingstatedescription branch November 6, 2022 21:32
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
* Fix missing stateDescriptionProvider in Group items

The stateDescriptionProvider was not properly passed to the base item. Because of that units set in the state description were not properly used when setting the group state after calculating aggregation functions.

Signed-off-by: Jan N. Klug <github@klug.nrw>
GitOrigin-RevId: 2bceba6
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Group SUM aggregation does not inherit the member's UoM
2 participants