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

Dynamic Simulation - Curve Configuration - Add some expert filter's criteria for Load and Generator and new operators IN, NOT_IN #78

Conversation

thangqp
Copy link
Contributor

@thangqp thangqp commented Nov 17, 2023

  1. Add criteria voltage_level_id for injection equipments, tested with LOAD and GENERATOR
  2. Add two new operators, i.e. IN and NOT_IN, which supports criteria values in multiple
  3. The implementation done in this PR correct voluntarily the bug 'persist null' after merge Adds a new EXISTS expert filter operator and PLANNED_ACTIVE_POWER_SET… #80 which adds EXIST operator

Related PRs:
Front-end :
gridsuite/gridstudy-app#1658 (with demo)
Back-end :
gridsuite/study-server#476
gridsuite/network-map-server#147

@thangqp thangqp added the WIP label Nov 17, 2023
@thangqp thangqp removed the WIP label Nov 23, 2023
@thangqp thangqp self-assigned this Nov 27, 2023
@thangqp thangqp added the WIP label Nov 27, 2023
@EstherDarkish EstherDarkish self-requested a review November 27, 2023 22:58
@thangqp thangqp changed the title Dynamic Simulation - Curve Configuration - Add some expert filter's criteria for Load and Generator Dynamic Simulation - Curve Configuration - Add some expert filter's criteria for Load and Generator and new operators IN, NOT_IN Nov 28, 2023
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

88.8% 88.8% Coverage
0.0% 0.0% Duplication

@thangqp thangqp removed the WIP label Nov 29, 2023
Comment on lines +75 to +77
if (isMultipleCriteriaOperator(this.getOperator())) { // multiple values
return this.getValues().stream().map(String::valueOf).collect(Collectors.joining(","));
} else { // single value or absence
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe return an empty string, null or String.valueOf(Collections.<String>emptySet()) if this.values is null ?
This is to prevent a NPE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should manage at DTO level by a validator to do a cross validation between two fields. This will be corrected in another PR

Comment on lines +45 to +47
if (isMultipleCriteriaOperator(this.getOperator())) { // multiple values
return String.join(",", this.getValues());
} else { // single value or absence
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, throws a NPE if this.getValues() is null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idem! Should manage at DTO level by a validator to do a cross validation between two fields. This will be corrected in another PR

Comment on lines +67 to +68
case IN -> this.getValues().contains(identifiableValue);
case NOT_IN -> !this.getValues().contains(identifiableValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential NPE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idem! Should manage at DTO level by a validator to do a cross validation between two fields. This will be corrected in another PR

Comment on lines +67 to +68
case IN -> this.getValues().contains(identifiableValue);
case NOT_IN -> !this.getValues().contains(identifiableValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential NPE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idem! Should manage at DTO level by a validator to do a cross validation between two fields. This will be corrected in another PR

Copy link
Contributor

@EstherDarkish EstherDarkish left a comment

Choose a reason for hiding this comment

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

Tests OK, code OK and remarks will be adressed in another PR.

@thangqp thangqp requested review from jonenst and geofjamg November 29, 2023 16:18
@thangqp thangqp merged commit 6ddd2ab into main Nov 29, 2023
4 checks passed
@thangqp thangqp deleted the dynamic_simulation_curve_configuration_add_some_filter_expert_criteria branch November 29, 2023 16:33
thangqp added a commit to gridsuite/gridstudy-app that referenced this pull request Jan 10, 2024
…er (#1658)

This PR hasn't any functional evolution on curve configuration. The GUI is unchanged

Related PRs:

Back-end :
gridsuite/study-server#476
gridsuite/network-map-server#147
gridsuite/filter-server#78
thangqp added a commit to gridsuite/study-server that referenced this pull request Jan 10, 2024
thangqp added a commit to gridsuite/network-map-server that referenced this pull request Jan 10, 2024
lilian-houdelet pushed a commit to gridsuite/gridstudy-app that referenced this pull request Jan 18, 2024
…er (#1658)

This PR hasn't any functional evolution on curve configuration. The GUI is unchanged

Related PRs:

Back-end :
gridsuite/study-server#476
gridsuite/network-map-server#147
gridsuite/filter-server#78
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