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

Implementation of SumState-Alerting (BREAKING UI+Backend/Odoo changes!) #2260

Merged
merged 39 commits into from
Feb 20, 2024

Conversation

da-Kai
Copy link
Contributor

@da-Kai da-Kai commented Jul 10, 2023

Description:

Notify specific users of the persistent presence of a negative sumState.

Planed Features

  • React and send mail on sumState
  • Improved data structure for growing alerting requirements and settings
  • Unit-Tests
  • UI settings

Odoo changes

OpenEMS/odoo-openems#3

Comment on lines 139 to 145
final var edgeOpt = this.metadata.getEdge(edgeId);
edgeOpt.ifPresent(edge -> {
if (level.isAtLeast(Level.FAULT)) {
this.tryAddEdge(edge, level);
} else {
this.tryRemoveEdge(edge);
}
Copy link
Contributor

@DerStoecki DerStoecki Jul 14, 2023

Choose a reason for hiding this comment

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

Hey @da-Kai

Thank you for providing your wip.
I was able to compare your implementation to mine and I got a good idea of your intention with your implementation.

I know that you stated in your general code you will "Improved data structure for growing alerting requirements and settings"
I just wanted to ask if the line 141

level.isAtLeast(Level.FAULT) 

will be configurable in future iterations (so that different users can be notified on different sumStates)?

Thank you in advance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @DerStoecki

I have inserted this filter for test purposes only. Later this will be configurable.

Kai J. added 7 commits August 31, 2023 12:20
Move "io.openems.edge.common.test.TimeLeapClock" to "io.openems.common.test.TimeLeapClock";
+ Integration tests for Offline- and SumState-Alerting;
+ Adapt UI
+ Change Odoo-Model Settings
# Conflicts:
#	io.openems.backend.metadata.odoo/src/io/openems/backend/metadata/odoo/MetadataOdoo.java
@OpenEMS OpenEMS deleted a comment from github-actions bot Sep 14, 2023
@OpenEMS OpenEMS deleted a comment from github-actions bot Sep 14, 2023
@OpenEMS OpenEMS deleted a comment from github-actions bot Sep 14, 2023
@OpenEMS OpenEMS deleted a comment from github-actions bot Sep 14, 2023
# Conflicts:
#	io.openems.backend.alerting/test/io/openems/backend/alerting/handler/TestOfflineEdgeHandler.java
#	io.openems.edge.battery.fenecon.home/test/io/openems/edge/battery/fenecon/home/BatteryFeneconHomeImplTest.java
#	io.openems.edge.controller.ess.timeofusetariff.discharge/test/io/openems/edge/controller/ess/timeofusetariff/discharge/ControllerEssTimeOfUseTariffDischargeImplTest.java
DerStoecki added a commit to opernikus-common/openems that referenced this pull request Oct 30, 2023
This commits refactors the generation of JsonArrays in the JsonUtils class in io.openems.common.utils.

This commit is one of many prepare PRs / commits for a bigger PR:
SumStateAlert OpenEMS#2260
By Seperating the big OpenEMS#2260 into smaller commits we would like to make the review easier.

Co-authored-by:  Kai Jeschek <[email protected]>
DerStoecki added a commit to opernikus-common/openems that referenced this pull request Oct 30, 2023
This commit adds a constructor, with an Instant Option only.

This commit is one of many prepare PRs / commits for a bigger PR:
SumStateAlert OpenEMS#2260
By Seperating the big OpenEMS#2260 into smaller commits we would like to make the review easier.

Co-authored-by:  Kai Jeschek <[email protected]>
DerStoecki added a commit to opernikus-common/openems that referenced this pull request Oct 30, 2023
* EventBuilder received a Build method for new Events and
* EventReader received toString, hashCode and equals method

This commit is one of many prepare PRs / commits for a bigger PR:
SumStateAlert OpenEMS#2260
By Seperating the big OpenEMS#2260 into smaller commits we would like to make the review easier.

Co-authored-by:  Kai Jeschek <[email protected]>
DerStoecki added a commit to opernikus-common/openems that referenced this pull request Oct 30, 2023
This commit is one of many prepare PRs / commits for a bigger PR:
SumStateAlert OpenEMS#2260
By Seperating the big OpenEMS#2260 into smaller commits we would like to make the review easier.

Co-authored-by:  Kai Jeschek <[email protected]>
DerStoecki added a commit to opernikus-common/openems that referenced this pull request Oct 30, 2023
This commit is one of many prepare PRs / commits for a bigger PR:
SumStateAlert OpenEMS#2260
By Seperating the big OpenEMS#2260 into smaller commits we would like to make the review easier.

Co-authored-by:  Kai Jeschek <[email protected]>
DerStoecki added a commit to opernikus-common/openems that referenced this pull request Oct 30, 2023
This doesn't do anything yet. But will in the future, when AlertingSettings get refactored to UserAlertingSettings and OdooHandler calls different db table.
This commit is one of many prepare PRs / commits for a bigger PR:
SumStateAlert OpenEMS#2260
By Seperating the big OpenEMS#2260 into smaller commits we would like to make the review easier.

Co-authored-by:  Kai Jeschek <[email protected]>
DerStoecki added a commit to opernikus-common/openems that referenced this pull request Oct 30, 2023
This commit is one of many prepare PRs / commits for a bigger PR:
SumStateAlert OpenEMS#2260
By Seperating the big OpenEMS#2260 into smaller commits we would like to make the review easier.

Co-authored-by:  Kai Jeschek <[email protected]>
DerStoecki added a commit to opernikus-common/openems that referenced this pull request Oct 30, 2023
The AlertingSettings are only added, and provide no functionality yet.
The defined Metadata methods are only added too and provide no functionality yet.
This commit is one of many prepare PRs / commits for a bigger PR:
SumStateAlert OpenEMS#2260
By Seperating the big OpenEMS#2260 into smaller commits we would like to make the review easier.

Co-authored-by:  Kai Jeschek <[email protected]>
DerStoecki added a commit to opernikus-common/openems that referenced this pull request Oct 30, 2023
* Changed Comments mostly on some alerting classes
* Changed getAllEdges to a stream() method -> e.g. MetaDataOdoo calls a Stream of Edges and filters for Edge::isOffline
This commit is one of many prepare PRs / commits for a bigger PR:
SumStateAlert OpenEMS#2260
By Seperating the big OpenEMS#2260 into smaller commits we would like to make the review easier.

Co-authored-by:  Kai Jeschek <[email protected]>
sfeilmeier pushed a commit that referenced this pull request Dec 6, 2023
…rtingSettings (#2414)

* Refactored Name of AlertingSetting to UserAlertingSettings

This is just a refactor of the Name. This does not provide any functionality, but will improve the readability of the SumStateAlert PR in the future.
This commit is one of many prepare PRs / commits for a bigger PR:
SumStateAlert #2260
By Seperating the big #2260 into smaller commits we would like to make the review easier.

Co-authored-by: Kai Jeschek <[email protected]>
# Conflicts:
#	io.openems.backend.alerting/src/io/openems/backend/alerting/handler/OfflineEdgeHandler.java
#	io.openems.backend.alerting/src/io/openems/backend/alerting/message/OfflineEdgeMessage.java
#	io.openems.backend.alerting/test/io/openems/backend/alerting/Dummy.java
#	io.openems.backend.alerting/test/io/openems/backend/alerting/OfflineEdgeAlertingTest.java
#	io.openems.backend.alerting/test/io/openems/backend/alerting/handler/TestOfflineEdgeHandler.java
#	io.openems.backend.common/src/io/openems/backend/common/jsonrpc/request/SetUserAlertingConfigsRequest.java
#	io.openems.backend.common/src/io/openems/backend/common/jsonrpc/response/GetUserAlertingConfigsResponse.java
#	io.openems.backend.common/src/io/openems/backend/common/metadata/Metadata.java
#	io.openems.backend.common/src/io/openems/backend/common/metadata/UserAlertingSettings.java
#	io.openems.backend.common/src/io/openems/backend/common/test/DummyMetadata.java
#	io.openems.backend.metadata.dummy/src/io/openems/backend/metadata/dummy/MetadataDummy.java
#	io.openems.backend.metadata.file/src/io/openems/backend/metadata/file/MetadataFile.java
#	io.openems.backend.metadata.odoo/src/io/openems/backend/metadata/odoo/MetadataOdoo.java
#	io.openems.backend.metadata.odoo/src/io/openems/backend/metadata/odoo/odoo/OdooHandler.java
#	io.openems.backend.uiwebsocket/src/io/openems/backend/uiwebsocket/impl/OnRequest.java
Copy link

Code Coverage

# Conflicts:
#	io.openems.backend.common/src/io/openems/backend/common/test/DummyMetadata.java
#	io.openems.backend.metadata.odoo/src/io/openems/backend/metadata/odoo/MetadataOdoo.java
#	io.openems.backend.metadata.odoo/src/io/openems/backend/metadata/odoo/odoo/OdooHandler.java
Copy link

github-actions bot commented Jan 2, 2024

Code Coverage

Copy link

Code Coverage

@da-Kai da-Kai marked this pull request as ready for review January 10, 2024 11:04
@DerStoecki
Copy link
Contributor

Hey @da-Kai

I will have a look into this PR,
test this in our environment and start a review.
It'll take some time though.

Thank you for your patience.

Copy link
Contributor

@DerStoecki DerStoecki left a comment

Choose a reason for hiding this comment

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

In short:

I merged this PR with our repo + did some test with existing edges (e.g. deployed this to our dev backend and dev ui + enabled Alerting and set the sumstate on test edges )
and the alerting worked.

Send mail only once
Filter recipients
check the sumstate
etc


Some thoughts to keep in mind:

There may be a potential issue with edges, that "toggle" their state periodically, and no notficiation will be send
(e.g. toggle their state every 30 seconds between ok/fault or warning/fault)

Copy link
Contributor

@sfeilmeier sfeilmeier left a comment

Choose a reason for hiding this comment

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

Thanks for pushing this topic forward. 🥳

Be aware though, that this is a breaking change for OpenEMS UI as well as requires a - not yet published - change to the OpenEMS Odoo addon.

Can you please provide the updated Odoo addon in time for the next release (1st March). Please also provide SQL scripts (or Odoo automated update-scripts?!) to update the existing Odoo database schema to not loose existing settings). Please link them to this PR.

Specifically internally at FENECON we need to be careful when updating a UI and Backend with this PR.

I am merging now, but if the other files are not provided in time, I'll have to remove it again before release 2024.3.0.

@sfeilmeier sfeilmeier changed the title Implementation of SumState-Alerting Implementation of SumState-Alerting (BREAKING UI+Backend/Odoo changes!) Feb 20, 2024
Copy link

Code Coverage

@sfeilmeier
Copy link
Contributor

Can you please provide the updated Odoo addon in time for the next release (1st March). Please also provide SQL scripts (or Odoo automated update-scripts?!) to update the existing Odoo database schema to not loose existing settings). Please link them to this PR.

Ah. I just realized, that the Odoo addon is already merged: OpenEMS/odoo-openems#3. Sorry!

We (and other users of OpenEMS Backend with Odoo) will still need some migration scripts, right?

@mlang97: FYI; this will be released in FEMS backend soon, as well

@sfeilmeier sfeilmeier merged commit 9848e2a into OpenEMS:develop Feb 20, 2024
2 checks passed
@da-Kai da-Kai deleted the feature/sumStateAlerting branch March 18, 2024 07:02
fanass-dev pushed a commit to fanass-dev/openems that referenced this pull request May 6, 2024
…!) (OpenEMS#2260)

Notify specific users of the persistent presence of a negative sumState.

- react and send mail on sumState
- Improved data structure for growing alerting requirements and settings
- Unit-Tests
- UI settings

Requires updated Odoo addon: OpenEMS/odoo-openems#3

Co-authored-by: Kai Jeschek <[email protected]>
Co-authored-by: Stefan Feilmeier <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants