-
Notifications
You must be signed in to change notification settings - Fork 0
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
[GridDyna] Using gridsuite filter library for mapping #96
Conversation
src/main/java/org/gridsuite/mapping/server/DynamicMappingException.java
Outdated
Show resolved
Hide resolved
src/main/java/org/gridsuite/mapping/server/controller/MappingController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/gridsuite/mapping/server/controller/ParameterController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/gridsuite/mapping/server/controller/ParameterController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/gridsuite/mapping/server/service/implementation/ParameterServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/org/gridsuite/mapping/server/service/implementation/ParameterServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/org/gridsuite/mapping/server/service/implementation/ParameterServiceImpl.java
Outdated
Show resolved
Hide resolved
src/test/java/org/gridsuite/mapping/server/ParameterControllerTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/gridsuite/mapping/server/service/client/filter/FilterClientTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/gridsuite/mapping/server/utils/FilterClientMockUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/org/gridsuite/mapping/server/controller/ParameterController.java
Outdated
Show resolved
Hide resolved
return filters; | ||
} | ||
@Schema(description = "Filter is dirty or not") | ||
private boolean filterDirty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this boolean about ?
This should stay on client side nope ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dirty is info received from front-end to help the back dot not update all filters in the mapping. I dont want send only touched filters to back because filter is inside a rule and we can have other modification in rule, like model, setGroup etc. Bref, I would like send a snapshot of the whole mapping from front-end to backend with some additional helping info
@Schema(description = "Filters") | ||
private List<AbstractFilter> filters; | ||
@JsonIgnore | ||
private UUID filterUuid; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That information is redondant. You have the filter id in filter nope ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use this field in dto when enrich dto's rules with filter charged from filter-server before return to front-end, see filterUuid, see https://github.com/gridsuite/dynamic-mapping-server/pull/96/files#diff-45ca266d2e1cd29d2cb9577adfa3f0dde3314d37d32cf2d511333c8caff3f053R68-R87
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally, filterUuid is removed as suggested: DONE in cd797b3
@@ -39,3 +39,7 @@ databaseChangeLog: | |||
- include: | |||
file: changesets/changelog_20231130T170646Z.xml | |||
relativeToChangelogFile: true | |||
|
|||
- include: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some data to migrate ? Or it's too complicated and not so benefic ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with PO: no migration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So complicated to migrate, all previous mappings should be deleted..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the sonar warnings
Three sonars waning are false positive on method name in parameterized test . Only one Annotate this method with JUnit5 '@org.junit.jupiter.api.BeforeEach' instead of JUnit4 '@Before'. that relates to migration JUnit, we need adapt the test, if not test will be fail.. I will do it in a separated TS PR |
|
only read through it, nothing stands out as blocking |
Related PRs:
gridsuite/filter-server#111
gridsuite/dynamic-simulation-server#95
#96
PR powsybl-dynawo:
https://github.com/powsybl/powsybl-dynawo/pull/349 (released 2.5.0-alpha-1)