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

Drools 4648 Define and Implement a DataModel to hold Background Table Data #1254

Merged
merged 23 commits into from Nov 5, 2019

Conversation

gitgabrio
Copy link
Contributor

@gitgabrio gitgabrio commented Oct 28, 2019

@dupliaka @yesamer @danielezonca

Client-side part of multi-repo ticket:

https://issues.jboss.org/browse/DROOLS-4648

see https://github.com/kiegroup/drools/pull/2630

This PR implement a specific Background object inside the data model; that object is completely isolated from Simulation and is managed inside the Background grid.
There is only one single history, where modification of both models are stored/managed.
On undo/redo command, currently the tab are not switched (to be implemented in next ticket).

gitgabrio and others added 12 commits October 18, 2019 15:41
# Conflicts:
#	drools-wb-screens/drools-wb-scenario-simulation-editor/drools-wb-scenario-simulation-editor-businesscentral-client/src/main/java/org/drools/workbench/screens/scenariosimulation/businesscentral/client/editor/ScenarioSimulationEditorBusinessCentralWrapper.java
#	drools-wb-screens/drools-wb-scenario-simulation-editor/drools-wb-scenario-simulation-editor-client/src/main/java/org/drools/workbench/screens/scenariosimulation/client/commands/ScenarioSimulationContext.java
#	drools-wb-screens/drools-wb-scenario-simulation-editor/drools-wb-scenario-simulation-editor-client/src/main/java/org/drools/workbench/screens/scenariosimulation/client/commands/actualcommands/AbstractScenarioSimulationCommand.java
#	drools-wb-screens/drools-wb-scenario-simulation-editor/drools-wb-scenario-simulation-editor-client/src/main/java/org/drools/workbench/screens/scenariosimulation/client/commands/actualcommands/AbstractSelectedColumnCommand.java
#	drools-wb-screens/drools-wb-scenario-simulation-editor/drools-wb-scenario-simulation-editor-client/src/main/java/org/drools/workbench/screens/scenariosimulation/client/editor/ScenarioSimulationEditorPresenter.java
#	drools-wb-screens/drools-wb-scenario-simulation-editor/drools-wb-scenario-simulation-editor-client/src/main/java/org/drools/workbench/screens/scenariosimulation/client/editor/ScenarioSimulationEditorWrapper.java
#	drools-wb-screens/drools-wb-scenario-simulation-editor/drools-wb-scenario-simulation-editor-client/src/main/java/org/drools/workbench/screens/scenariosimulation/client/editor/ScenarioSimulationView.java
#	drools-wb-screens/drools-wb-scenario-simulation-editor/drools-wb-scenario-simulation-editor-client/src/main/java/org/drools/workbench/screens/scenariosimulation/client/editor/ScenarioSimulationViewImpl.java
#	drools-wb-screens/drools-wb-scenario-simulation-editor/drools-wb-scenario-simulation-editor-client/src/main/java/org/drools/workbench/screens/scenariosimulation/client/editor/strategies/AbstractDMNDataManagementStrategy.java
#	drools-wb-screens/drools-wb-scenario-simulation-editor/drools-wb-scenario-simulation-editor-client/src/main/java/org/drools/workbench/screens/scenariosimulation/client/factories/CollectionEditorSingletonDOMElementFactory.java
#	drools-wb-screens/drools-wb-scenario-simulation-editor/drools-wb-scenario-simulation-editor-client/src/test/java/org/drools/workbench/screens/scenariosimulation/client/commands/actualcommands/AbstractScenarioSimulationCommandTest.java
#	drools-wb-screens/drools-wb-scenario-simulation-editor/drools-wb-scenario-simulation-editor-client/src/test/java/org/drools/workbench/screens/scenariosimulation/client/editor/ScenarioSimulationEditorPresenterTest.java
#	drools-wb-screens/drools-wb-scenario-simulation-editor/drools-wb-scenario-simulation-editor-client/src/test/java/org/drools/workbench/screens/scenariosimulation/client/editor/ScenarioSimulationViewImplTest.java
# Conflicts:
#	drools-wb-screens/drools-wb-scenario-simulation-editor/drools-wb-scenario-simulation-editor-backend/src/main/java/org/drools/workbench/screens/scenariosimulation/backend/server/util/DMNSimulationSettingsCreationStrategy.java
#	drools-wb-screens/drools-wb-scenario-simulation-editor/drools-wb-scenario-simulation-editor-client/src/main/java/org/drools/workbench/screens/scenariosimulation/client/editor/ScenarioSimulationEditorPresenter.java
#	drools-wb-screens/drools-wb-scenario-simulation-editor/drools-wb-scenario-simulation-editor-client/src/test/java/org/drools/workbench/screens/scenariosimulation/client/editor/ScenarioSimulationEditorPresenterTest.java
@gitgabrio
Copy link
Contributor Author

jenkins please retest this

/**
* <code>Command</code> to <b>append</b> (i.e. put in the last position) a row
*/
public class ScenarioGridWidgetFocusCommand extends AbstractScenarioSimulationCommand {
Copy link
Member

Choose a reason for hiding this comment

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

Not part of this PR

Comment on lines +34 to +35
int SIMULATION_TAB_INDEX = 0;
int BACKGROUND_TAB_INDEX = 1;
Copy link
Member

Choose a reason for hiding this comment

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

Why are these value here? What about move them to constantHolder or similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danielezonca
Those value are meaningful only for the ScenarioSimulationEditorWrappers, that's why I put them here

Comment on lines 55 to 57
void selectSimulationTab();

void selectBackgroundTab();
Copy link
Member

Choose a reason for hiding this comment

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

Not part of this PR?

Comment on lines +59 to +60


Copy link
Member

Choose a reason for hiding this comment

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

Well done! 👍
😄

Copy link

Choose a reason for hiding this comment

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

I think one new line would be enough. Ideally no change here. Keep in mind more changes you do in commit == more conflicts resolution in future.

@@ -48,7 +48,7 @@ public ScenarioGrid getScenarioGrid() {
* @param scenarioGrid ScenarioGrid to add to the Layer
* @return The Layer
*/
public Layer addScenarioGrid(final ScenarioGrid scenarioGrid) {
public Layer addScesimDataGrid(final ScenarioGrid scenarioGrid) {
Copy link
Member

Choose a reason for hiding this comment

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

Wrong rename

Comment on lines 103 to 113
// to restore when implement tab switching
// @Test
// public void redoNotEmptyDifferentGrid() {
// scenarioCommandRegistry.undoneCommands.push(appendRowCommandMock);
// when(appendRowCommandMock.commonUndoRedoPreexecution(eq(scenarioSimulationContextLocal))).thenReturn(Optional.of(CommandResultBuilder.SUCCESS));
// int currentSize = scenarioCommandRegistry.undoneCommands.size();
// scenarioCommandRegistry.redo(scenarioSimulationContextLocal);
// assertEquals(currentSize, scenarioCommandRegistry.undoneCommands.size());
// verify(scenarioCommandRegistry, never()).commonUndoRedoOperation(eq(scenarioSimulationContextLocal), eq(appendRowCommandMock), eq(false));
// verify(scenarioCommandRegistry, never()).setUndoRedoButtonStatus(eq(scenarioSimulationContextLocal));
// }
Copy link
Member

Choose a reason for hiding this comment

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

Not part of this PR

Comment on lines 242 to 246
// @Test
// public void setFocusedContext() {
// presenter.setFocusedContext(contextMock);
// verify(presenter, times(1)).populateRightDocks(eq(TestToolsPresenter.IDENTIFIER));
// }
Copy link
Member

Choose a reason for hiding this comment

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

Commented code

Comment on lines 794 to 795
// TODO {gcardosi} restore after correction on presenter
// verify(scenarioBackgroundGridWidgetMock, times(1)).setContent(isA(Simulation.class), eq(scenarioSimulationContextLocal.getSettings().getType()));
Copy link
Member

Choose a reason for hiding this comment

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

? not part of this PR?

import static org.mockito.Mockito.when;

@RunWith(LienzoMockitoTestRunner.class)
//@RunWith(LienzoMockitoTestRunner.class)
public class ScenarioSimulationViewProducerTest extends AbstractProducerTest {
Copy link
Member

Choose a reason for hiding this comment

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

To be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -435,13 +436,13 @@ public void testShowContextMenuGivenOrExpect() {
private Simulation getSimulation() {
Simulation toReturn = new Simulation();
SimulationDescriptor simulationDescriptor = toReturn.getSimulationDescriptor();
simulationDescriptor.setType(ScenarioSimulationModel.Type.RULE);
// simulationDescriptor.setType(ScenarioSimulationModel.Type.RULE);
Copy link
Member

Choose a reason for hiding this comment

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

Commented code

@dupliaka
Copy link
Member

jenkins execute full downstream build

@@ -56,9 +57,9 @@
* @return
Copy link
Member

Choose a reason for hiding this comment

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

Add @param settings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dupliaka
done

* @param kieContainer
* @return
*/
@Override
public List<FactMappingValidationError> validate(Simulation simulation, KieContainer kieContainer) {
public List<FactMappingValidationError> validate(Simulation simulation, Settings settings, KieContainer kieContainer) {
Copy link
Member

Choose a reason for hiding this comment

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

I noticed you added settings parameter but do not use it. But if we will set rule group for test scenario background would the rule group validated on this step? Shouldn't we include the validation of settings here?

Copy link
Contributor Author

@gitgabrio gitgabrio Oct 29, 2019

Choose a reason for hiding this comment

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

@dupliaka
As far as I remember there was not the validation of any "setting": we could add one, but I'm not sure what/how we would validate; anyway, I would do that in another ticket.
Beside that, that "Settings" param is there only to fullfill the contract of the declared abstract method

@gitgabrio
Copy link
Contributor Author

@danielezonca @dupliaka @yesamer
Ready for re-evaluation

@gitgabrio
Copy link
Contributor Author

jenkins please retest this

Copy link
Member

@danielezonca danielezonca left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@@ -86,62 +87,44 @@
extends KieService<ScenarioSimulationModelContent>
implements ScenarioSimulationService {

private static final String KIE_VERSION = "kie.version";
private static final String junitActivatorPackageName = "testscenario";
Copy link
Member

Choose a reason for hiding this comment

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

Can you tell if you reject to keep the same style on purpose? Shouldn't we change junitActivatorPackageName on JUNIT_ACTIVATOR_PACKAGE_NAME

case DMN:
return dmnSimulationCreationStrategy.createBackground(context, value);
default:
return null;
Copy link
Member

Choose a reason for hiding this comment

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

Question: can we throw an exception here? Like IlligalStateException? So to have a chance to understand what happened by logs in runtime?

}

/**
* Create an empty column using factMappingType defined. The new column will be added as last column of
* the group (GIVEN/EXPECT) (see findLastIndexOfGroup)
* @param simulationDescriptor
* @param scenarioWithIndex
* @param scesimDataWithIndex
* @param placeholderId
* @param factMappingType
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we add @param columnIndex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dupliaka
done

@gitgabrio
Copy link
Contributor Author

Manual review 01

Question, on background tab, bound instances from main tab are listed, is it expected?
Screenshot from 2019-11-01 08-42-30

@jomarko
How did you create this situation?

@gitgabrio
Copy link
Contributor Author

Manual review 02

Please follow this scenario

  • Import sample mortgages project
  • Crate new a.scesim file, just bound given column to Applicant.age and put value to first data row 60
  • Save and export a.scesim
  • Result of export can be found in jira
  • Close a.scesim
  • Create new b.scesim
  • Import the attached csv file
  • Nothing will happen

@jomarko
The imported csv must "conform" to the grid structure is imported in. So, if you export data from a grid with two specifically mapped columns, and then try to import that same csv in another new -empty - grid, nothing will be imported, because the csv is referring to a specific grid configuration. Does this make sense?

@gitgabrio
Copy link
Contributor Author

Manual review 03

user can not bind instance on background tab see the video

What I see in the video is expected - Background grid has different goal and semantics from scenario one. Maybe it should be somehow better enforced

@gitgabrio
Copy link
Contributor Author

gitgabrio commented Nov 4, 2019

Manual review 04

No error popup about current vs actual value for dmn based scenarios
used dmn and scesim

Screenshot from 2019-11-01 09-08-32

Actually I see the popup

image

@gitgabrio
Copy link
Contributor Author

Manual Review 05

For DMN based scenarios, expect columns are present in background tab, is it expected?
Screenshot from 2019-11-01 09-14-59

@jomarko
Fixed

@gitgabrio
Copy link
Contributor Author

If this PR is just about storing the values on Background tab with no real effect to the scenario run, I am done with manual review @gitgabrio could you please confirm and have a look on my comments, here is a summary of jiras related to DROOLS-4648 I reported till now. If some comments need a separate ticket I am fine with that.

Thank you for the PR

@jomarko
Thanks, for the moment being Background data are ignored - they are just saved (and restored) from the scesim data, but not used anyway. Not even imported/exported, currently. I have to be sure they are correcltly added/removed/updated/stored/retrieved to the background grid. History should also work for them (keep in mind there is a single history containing both scenario and background modifications)

@gitgabrio
Copy link
Contributor Author

Manual review 01

Question, on background tab, bound instances from main tab are listed, is it expected?
Screenshot from 2019-11-01 08-42-30

@jomarko
Fixed

@gitgabrio
Copy link
Contributor Author

gitgabrio commented Nov 4, 2019

Manual review 7
Background initialised with properties for DMN based test scenario
image

@dupliaka
Fixed (duplicate of #1254 (comment) ?)

@dupliaka
Copy link
Member

dupliaka commented Nov 4, 2019

Manual review 7
Background initialised with properties for DMN based test scenario
image

@dupliaka
Fixed (duplicate of #1254 (comment) ?)

I am not sure, My idea was that exactly for DMN we are showing all the properties that we got for DMN in Model, but Background even for DMN should initialise as empty table. Will retest it

@gitgabrio
Copy link
Contributor Author

gitgabrio commented Nov 4, 2019

Manual review 6

Export of simple test scenario is not working:
Values from export file were not shown

"#",Scenario description,GIVEN,EXPECT "#",Scenario description,Driver,Driver "#",Scenario description,age,status 1,TEst scenario 1,12,young 2,TEst scenario 1,12,young 3,TEst scenario 1,12,young

image

Error "A CSV file with an incorrect header layout has been imported. Make sure the imported CSV file has unmodified headers."

@dupliaka See #1254 (comment) - it works if the newly created target grid has the same columns as the original one

@dupliaka
Copy link
Member

dupliaka commented Nov 4, 2019

Manual review 6
Export of simple test scenario is not working:
Values from export file were not shown
"#",Scenario description,GIVEN,EXPECT "#",Scenario description,Driver,Driver "#",Scenario description,age,status 1,TEst scenario 1,12,young 2,TEst scenario 1,12,young 3,TEst scenario 1,12,young
image
Error "A CSV file with an incorrect header layout has been imported. Make sure the imported CSV file has unmodified headers."

@dupliaka See #1254 (comment) - it works if the newly created target grid has the same columns as the original one

Nobody expected this.

@gitgabrio
Copy link
Contributor Author

Manual review 8

Validation error appears instead of warning if I remove DMN instance in DMN file that was used in background scenario.
video

@dupliaka
In the attached video you clicked in different areas, with two different error popup.
Anyway, it is expected to have an error and not a simple warning: that scenario can not possible work anymore because it is referring to properties that does not exists anymore. Does this make sense?

@gitgabrio
Copy link
Contributor Author

Manual review 9

Warning "Command EnableTestToolsCommand failure: ERROR (TypeError) : Cannot read property 'setInstanceAssigned_1_g$' of undefined" instead of dialog of removing outdated object from rule based scenario.

Steps:

  • Create rule based test scenario
  • Add an URL object to the model
  • Add Url property to the Bkg section
  • Save and reopen
  • Go to Bkg section
    Expect : warning dialog about changed model
    Actual: No warnings and message "Command EnableTestToolsCommand failure: ERROR (TypeError) : Cannot read property 'setInstanceAssigned_1_g$' of undefined" if I click on a header

@dupliaka
Sorry can't follow: what is an "URL" object? Where did you exactly add it? Why/when should warn about "changed model" ?

@gitgabrio
Copy link
Contributor Author

Manual review 10

If user rename Data object that was used in test scenario, then validate it in test scenario and goes to data object again he got "Java file could not be parsed, it won’t be possible to open the Editor tab, use the Source tab instead."

Steps:

  • Create rule-based test scenario and add a property from data object
  • Rename that data object
  • Go back to scenario and validate it 0 it should contain validation error
  • Open renamed Data object
    Expected: data object model opens
    Actual: "Java file could not be parsed, it won’t be possible to open the Editor tab, use the Source tab instead."

@dupliaka
@jomarko gives a good explanation. Beside that, it is not part of this PR.

@gitgabrio
Copy link
Contributor Author

gitgabrio commented Nov 4, 2019

Manual review 6
Export of simple test scenario is not working:
Values from export file were not shown
"#",Scenario description,GIVEN,EXPECT "#",Scenario description,Driver,Driver "#",Scenario description,age,status 1,TEst scenario 1,12,young 2,TEst scenario 1,12,young 3,TEst scenario 1,12,young
image
Error "A CSV file with an incorrect header layout has been imported. Make sure the imported CSV file has unmodified headers."

@dupliaka See #1254 (comment) - it works if the newly created target grid has the same columns as the original one

Nobody expected this.

@dupliaka when you import a csv there is a popup warn about header structure; beside that, this is how it has been implemented. @danielezonca wdyt?
@dupliaka
Anyway, it could be a possible refactor, but it is not related to this PR. Could you please open a specific ticket? Thanks!

@jomarko
Copy link

jomarko commented Nov 5, 2019

@gitgabrio
Manual Review 1 reported as https://issues.jboss.org/browse/DROOLS-4726

@jomarko
Copy link

jomarko commented Nov 5, 2019

@gitgabrio
Manual review 3 reported as https://issues.jboss.org/browse/DROOLS-4727, the message is not explaining that user can not bind facts on Background tab.

@dupliaka
Copy link
Member

dupliaka commented Nov 5, 2019

Manual review 9

Warning "Command EnableTestToolsCommand failure: ERROR (TypeError) : Cannot read property 'setInstanceAssigned_1_g$' of undefined" instead of dialog of removing outdated object from rule based scenario.
Steps:

  • Create rule based test scenario
  • Add an URL object to the model
  • Add Url property to the Bkg section
  • Save and reopen
  • Go to Bkg section
    Expect : warning dialog about changed model
    Actual: No warnings and message "Command EnableTestToolsCommand failure: ERROR (TypeError) : Cannot read property 'setInstanceAssigned_1_g$' of undefined" if I click on a header

@dupliaka
Sorry can't follow: what is an "URL" object? Where did you exactly add it? Why/when should warn about "changed model" ?

URL is the via items in Data Object tab
https://issues.jboss.org/secure/attachment/12457177/URL%20type.mov

Copy link

@jomarko jomarko left a comment

Choose a reason for hiding this comment

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

@gitgabrio I checked your resolution to my comments to code and manual review comments. as result of this I reported two more jiras. I am not doing any intensive re-review due to my time resources. Thank you

@dupliaka
Copy link
Member

dupliaka commented Nov 5, 2019

Manual review 8

Validation error appears instead of warning if I remove DMN instance in DMN file that was used in background scenario.
video

@dupliaka
In the attached video you clicked in different areas, with two different error popup.
Anyway, it is expected to have an error and not a simple warning: that scenario can not possible work anymore because it is referring to properties that does not exists anymore. Does this make sense?

Sorry but it should not show error and after that show the unexpected error after i try to change anything in the document.

@gitgabrio
Copy link
Contributor Author

Manual review 8

Validation error appears instead of warning if I remove DMN instance in DMN file that was used in background scenario.
video

@dupliaka
In the attached video you clicked in different areas, with two different error popup.
Anyway, it is expected to have an error and not a simple warning: that scenario can not possible work anymore because it is referring to properties that does not exists anymore. Does this make sense?

Sorry but it should not show error and after that show the unexpected error after i try to change anything in the document.

@dupliaka
It could be a possible refactor, but it is not related to this PR. Could you please open a specific ticket? Thanks!

@dupliaka
Copy link
Member

dupliaka commented Nov 5, 2019

Ok, I gathered the results here https://docs.google.com/spreadsheets/d/1bsQb9_EbOuQFotH41KmfETd8wkQhQU_lgsnKvpnKmF8/edit?usp=sharing
@gitgabrio There only two points left, could you answer it?

@dupliaka
Copy link
Member

dupliaka commented Nov 5, 2019

jenkins execute full downstream build

@gitgabrio
Copy link
Contributor Author

Ok, I gathered the results here https://docs.google.com/spreadsheets/d/1bsQb9_EbOuQFotH41KmfETd8wkQhQU_lgsnKvpnKmF8/edit?usp=sharing
@gitgabrio There only two points left, could you answer it?

Ok, I gathered the results here https://docs.google.com/spreadsheets/d/1bsQb9_EbOuQFotH41KmfETd8wkQhQU_lgsnKvpnKmF8/edit?usp=sharing
@gitgabrio There only two points left, could you answer it?

@dupliaka
One of the question I think is related to DROOLS-4726/DROOLS-4727.
About importing scesim: could you attach the failing scesim here?

@gitgabrio
Copy link
Contributor Author

Manual review 9

Warning "Command EnableTestToolsCommand failure: ERROR (TypeError) : Cannot read property 'setInstanceAssigned_1_g$' of undefined" instead of dialog of removing outdated object from rule based scenario.
Steps:

  • Create rule based test scenario
  • Add an URL object to the model
  • Add Url property to the Bkg section
  • Save and reopen
  • Go to Bkg section
    Expect : warning dialog about changed model
    Actual: No warnings and message "Command EnableTestToolsCommand failure: ERROR (TypeError) : Cannot read property 'setInstanceAssigned_1_g$' of undefined" if I click on a header

@dupliaka
Sorry can't follow: what is an "URL" object? Where did you exactly add it? Why/when should warn about "changed model" ?

URL is the via items in Data Object tab
https://issues.jboss.org/secure/attachment/12457177/URL%20type.mov

@dupliaka
Just followed the steps you mentioned without issue: is there something else missing?

Copy link
Member

@dupliaka dupliaka left a comment

Choose a reason for hiding this comment

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

@yesamer yesamer removed their request for review November 5, 2019 15:39
@danielezonca danielezonca merged commit 445979d into kiegroup:master Nov 5, 2019
@gitgabrio gitgabrio deleted the DROOLS-4648 branch December 2, 2019 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants