[Drools-3838] Improve error reporting in scenario result #1125

yesamer · 2019-04-23T10:58:05Z

@kkufova @gitgabrio @danielezonca @jomarko can you please test it and review it? Thanks!

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

This PR contains the client sidechanges required for DROOLS-3838.
Related PR on drools-wb which contains backend change is here kiegroup/drools#2335

PRs list:
kiegroup/drools#2335

…/FactMappingValue

…ecated variable. TO be managed in a different ticket.

…o remove simulationDescription parameter

kkufova · 2019-04-25T11:02:03Z

@yesamer, I noticed the following things:

First of all, I thought there should be a small red icon added directly to the cell for displaying the message. These plans have changed?
Secondly, I discovered a serious bug: if the expected value is null (and the test fails because the entered value is different), the following unexpected error is thrown every time you hover over the cell:

Uncaught exception: Exception caught: Exception caught: (TypeError) : Cannot read property 'toString' of undefined Caused by: Exception caught: (TypeError) : Cannot read property 'toString' of undefined Caused by: (TypeError) : Cannot read property 'toString' of undefined

The same things happens with imported test scenarios. If you import a .scesim file, instead of displaying the message, an unexpected error is thrown when there's an error.
I noticed that in the window, the Apply button is dark grey as opposed to the original design. Can this be fixed?

yesamer · 2019-05-02T08:00:42Z

@kkufova Points 2,3,4 can be easily managed. Point 1 could be a little bit harder to implement.

danielezonca · 2019-05-02T08:05:40Z

@kkufova @yesamer Please manage point 1 with a different ticket

…nor change on CSS

yesamer · 2019-05-07T13:33:24Z

I created https://issues.jboss.org/browse/DROOLS-3989 to manage point 1.
Point 2,3 and 4 are fixed.

kkufova · 2019-05-07T14:10:37Z

@yesamer, I found the following issues:

If a cell is highlighted, you fix the value and rerun the tests, the cell is still highlighted even though the tests are now passing. Can this be fixed?
I noticed that Impossible to parse as boolean message keeps getting displayed instead of an actual help. For example, I used falsee instead of false. I think in this case we definitely need The expected value is 'false' but the actual one is 'falsee'. Another scenario: if the expected value is null but I enter [] instead, the error message is Error with this value: Impossible to parse as boolean. It doesn't say anything about the actual value being null.
If the expected value is null and you use something else (such as true) and then click Apply, instead of adding null, the cell is empty.

All three bugs can be seed here.

yesamer · 2019-05-08T14:46:30Z

@jomarko Hi Jozef, I fixed points 1 and 3 raised by @kkufova. Point 2 should be managed in another ticket in my opinion. Please consider that the changes contained in https://github.com/kiegroup/drools/pull/2335 are mandatory to test it.
Thanks.

jomarko

Code review notes, manual check pending.

jomarko · 2019-05-09T12:08:20Z

...ava/org/drools/workbench/screens/scenariosimulation/client/handlers/CommonOnMoveHandler.java

+    protected ErrorReportPopoverPresenter errorReportPopupPresenter;
+
+    protected Integer currentlyShownHeaderRowIndex = -1;
+    protected Integer currentlyShownHeaderColumnIndex = -1;


currentlyShownHeaderRowIndex and currentlyShownHeaderColumnIndex are never used, is it intentional?

At the moment these are not used, you right. Could be used in the future when a popover for the header cells is required. But I agree with you, they can be removed.
Done!

jomarko · 2019-05-09T12:12:42Z

...h/screens/scenariosimulation/client/handlers/AbstractScenarioSimulationGridPanelHandler.java

+        if (isHeader) {
+            return manageHeaderCoordinates(uiColumnIndex, scenarioGridColumn, gridClickPoint);
+        } else {
+            return (uiRowIndex == null || uiColumnIndex == null) ? manageBodyCoordinates(-1, -1) : manageBodyCoordinates(uiRowIndex, uiColumnIndex);


I would change this line to simple return manageBodyCoordinates(uiRowIndex, uiColumnIndex) and add the check for null values inside of manageBodyCoordinates. This wil facilitate invocation from other places (if needed in future), what do you think?

Agree, DONE!

jomarko · 2019-05-09T12:14:42Z

...ava/org/drools/workbench/screens/scenariosimulation/client/handlers/CommonOnMoveHandler.java

+    @Override
+    protected boolean manageBodyCoordinates(Integer uiRowIndex, Integer uiColumnIndex) {
+        /* In this case, the mouse is out ot the GridLayer, then return false, without perform any action */
+        if (uiColumnIndex == -1 || uiRowIndex == -1) {


Related to comment in AbstractScenarioSimulationGridPanelHandler. This if would change to uiColumnIndex == null || uiRowIndex == null

Agree, DONE!

jomarko · 2019-05-09T12:17:47Z

...ava/org/drools/workbench/screens/scenariosimulation/client/handlers/CommonOnMoveHandler.java

+                currentlyShownBodyColumnIndex = uiColumnIndex;
+                /* It calculates the coordinates */
+                final GridColumn<?> column = scenarioGrid.getModel().getColumns().get(uiColumnIndex);
+                Point2D xYCell = retrieveCellMiddleXYPosition(column, uiRowIndex);


xYCell can be also final

jomarko · 2019-05-09T12:21:03Z

...ava/org/drools/workbench/screens/scenariosimulation/client/handlers/CommonOnMoveHandler.java

+                } else {
+                    errorReportPopupPresenter.show(ScenarioSimulationEditorConstants.INSTANCE.errorReason(),
+                                                   ScenarioSimulationEditorConstants.INSTANCE.errorPopoverMessageFailedWithException(
+                                                           factMappingValue.getExceptionMessage()),


Do we need to check null value here as we do for FAILED_WITH_ERROR and if test is positive use constant NULL?

Do you mean to test if factMappingValue.getStatus() is null?
Yes, I agree to add it, but in another point. In my opinion, if that value is NULL, the popover shouldn't be visible at all. I put it in line 87.

jomarko · 2019-05-09T12:27:34Z

...java/org/drools/workbench/screens/scenariosimulation/client/popover/AbstractPopoverView.java

+    }
+
+    @Override
+    public void show(final Optional<String> editorTitle, final int mx, final int my, Position position) {


Just for consistency, please make also the last parameter final: final Position position

jomarko · 2019-05-09T12:36:33Z

...a/org/drools/workbench/screens/scenariosimulation/client/popover/ErrorReportPopoverView.java

+                         final String keepText,
+                         final int mx,
+                         final int my,
+                         final Position position) {


Just formatting - indent of parameters.

jomarko · 2019-05-09T12:39:52Z

...creens/scenariosimulation/client/resources/i18n/ScenarioSimulationEditorConstants.properties

+close=Close
+apply=Apply
+errorPopoverMessageFailedWithError=The expected value is ''{0}'' but the actual one is ''{1}''.
+errorPopoverMessageFailedWithException=Error with this value: {0}


Not sure if this comes form UX mockups, but I would expect something more like - "Exception with detailed message: {0}"

In my knowledge, this case is not handled in UX mockups.
Fine for me to put yours.
DONE!

jomarko · 2019-05-09T12:40:56Z

...org/drools/workbench/screens/scenariosimulation/client/handlers/CommonOnMoveHandlerTest.java

+    @Mock
+    private FactMappingValue factMappingValueMock;
+    @Mock
+    private AbsolutePanel scrollPanel;


to be consistent, please use name scrollPanelMock

jomarko · 2019-05-09T12:41:11Z

...org/drools/workbench/screens/scenariosimulation/client/handlers/CommonOnMoveHandlerTest.java

+    @Mock
+    private AbsolutePanel scrollPanel;
+    @Mock
+    private Element element;


To be consistent, please use name elementMock

jomarko · 2019-05-10T04:09:56Z

jenkins execute full downstream build

jomarko · 2019-05-10T08:31:24Z

@yesamer thank you for the PR, works very nice. I can confirm pints 1. and 3. from comment are fixed. The point 2. will be addressed by DROOLS-3992. There remain two points that I would like to discuss:

The text we show to user when some exception occurred during parsing cell value. The popover title is definitely fine - Error reason. However I think we should still improve the prefix of the exception message to make it maybe shorter and more clear? I think it is question to UX of your scrum team? What do you think? [1]
If some issue was in multiple ceels of one row, and user decides to fix one value (Apply button is pressed) whole row highlight is lost. See [2]. Is this expected or not? if not, should I file a separate jira or worth of adding into this PR?

[1]

[2]
https://drive.google.com/file/d/1c-Hgc6ePD2OttI-ddVLhqj9UU0psrLPF/view?usp=sharing

yesamer · 2019-05-10T08:39:06Z

@jomarko Thank you!
About your considerations:

It could be a solution to involve UX team to find a better message. We can handle this in DROOLS-3992, in my opinon.
I didn't manage the case you reported, then I agree with you to open a new ticket. Please assign it to me, I will work on it in the next sprint.
Thank you for the review and the test

danielezonca

Fine for me

gitgabrio

@yesamer
Fine for me, but I would prefer to have a green build for that before merging

danielezonca · 2019-05-10T16:27:28Z

jenkins execute full downstream build

yesamer · 2019-05-13T07:05:30Z

Hi @manstis, can you please merge this PR and kiegroup/drools#2335?
Thanks,

gitgabrio and others added 26 commits April 9, 2019 15:11

[DROOLS-3838] Add expected/retrieved information inside ResultWrapper…

cd73396

…/FactMappingValue

Merge remote-tracking branch 'origin/master' into DROOLS-3838

5d2ecf2

[DROOLS-3838] Trying to reuse existing popover code

12f62ee

Merge remote-tracking branch 'origin/master' into DROOLS-3838

05dff58

[DROOLS-3838] Add to DOM

84d18fb

[DROOLS-3838] Fixed popover structure

c8a3689

[DROOLS-3838] Binding popover to onmousemove event.

dc6959e

Merge remote-tracking branch 'origin/master' into DROOLS-3838

19eb004

[DROOLS-3838] Popover position fixed.

788d456

[DROOLS-3838] Popover position fixed.

64e5faa

[DROOLS-3838] Minor issue fixed.

1ad6161

[DROOLS-3838] Compilation fixed + tests added.

0dde4f1

[DROOLS-3838] Code cleaning

6177702

[DROOLS-3838] Code cleaning

24bf80a

Merge remote-tracking branch 'origin/master' into DROOLS-3838

fef3455

[DROOLS-3838] Logic completed

806e3a0

[DROOLS-3838] Logic completed

a425a74

[DROOLS-3838] Existing tests fixed.

288f086

[DROOLS-3838] CommonOnMoveHandlerTest addded

6fd965c

Merge remote-tracking branch 'origin/master' into DROOLS-3838

1b55aa8

[DROOLS-3838] Reverted changes on Scenario.simulationDescription depr…

d403964

…ecated variable. TO be managed in a different ticket.

[DROOLS-3838] Minor changes

90abac5

[DROOLS-3838] Minor changes + restored changes on Scenario in order t…

de1891d

…o remove simulationDescription parameter

[DROOLS-3838] Minor changes

badb6e8

[DROOLS-3838] Scrollpanel managed

8508b8d

[DROOLS-3838] Scrollpanel managed

52fa7fb

Merge remote-tracking branch 'origin/master' into DROOLS-3838

82d1e3f

yesamer added 6 commits May 6, 2019 12:10

[DROOLS-3838] Handling null values for expected and error fields + mi…

a1eb572

…nor change on CSS

[DROOLS-3838] Handling null values for expected and error fields

1f2a24a

[DROOLS-3838] Handling exception on error cells.

4eebab8

[DROOLS-3838] Minor changes.

7cfcda3

[DROOLS-3838] Typo fixed.

72bfaaa

Merge remote-tracking branch 'origin/master' into DROOLS-3838

98c4c33

yesamer added 5 commits May 7, 2019 16:27

[DROOLS-3838] Null handling fixed.

1ec9a11

[DROOLS-3838] Improved CommonOnMoveHandler + fixes of reported issues

c6ac23a

[DROOLS-3838] Tests added + comments

8725de1

Merge remote-tracking branch 'origin/master' into DROOLS-3838

258a0cb

Merge remote-tracking branch 'origin/master' into DROOLS-3838

7a31270

yesamer mentioned this pull request May 8, 2019

[Drools-3838] Improve error reporting in scenario result apache/incubator-kie-drools#2335

Merged

jomarko reviewed May 9, 2019

View reviewed changes

Fixed reported review issues.

6c35ad7

Reverted part of the previous commit

146ed57

jomarko approved these changes May 10, 2019

View reviewed changes

danielezonca approved these changes May 10, 2019

View reviewed changes

gitgabrio approved these changes May 10, 2019

View reviewed changes

manstis merged commit b84164a into kiegroup:master May 13, 2019

yesamer deleted the DROOLS-3838 branch June 11, 2019 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Drools-3838] Improve error reporting in scenario result #1125

[Drools-3838] Improve error reporting in scenario result #1125

yesamer commented Apr 23, 2019 •

edited

Loading

kkufova commented Apr 25, 2019 •

edited

Loading

yesamer commented May 2, 2019

danielezonca commented May 2, 2019

yesamer commented May 7, 2019

kkufova commented May 7, 2019

yesamer commented May 8, 2019

jomarko left a comment

jomarko May 9, 2019

yesamer May 9, 2019

jomarko May 9, 2019

yesamer May 9, 2019

jomarko May 9, 2019

yesamer May 9, 2019

jomarko May 9, 2019

yesamer May 9, 2019

jomarko May 9, 2019

yesamer May 9, 2019 •

edited

Loading

jomarko May 9, 2019

yesamer May 9, 2019

jomarko May 9, 2019

yesamer May 9, 2019

jomarko May 9, 2019

yesamer May 9, 2019

jomarko May 9, 2019

yesamer May 9, 2019

jomarko May 9, 2019

yesamer May 9, 2019

jomarko commented May 10, 2019

jomarko commented May 10, 2019

yesamer commented May 10, 2019 •

edited

Loading

danielezonca left a comment

gitgabrio left a comment •

edited

Loading

danielezonca commented May 10, 2019

yesamer commented May 13, 2019

[Drools-3838] Improve error reporting in scenario result #1125

[Drools-3838] Improve error reporting in scenario result #1125

Conversation

yesamer commented Apr 23, 2019 • edited Loading

kkufova commented Apr 25, 2019 • edited Loading

yesamer commented May 2, 2019

danielezonca commented May 2, 2019

yesamer commented May 7, 2019

kkufova commented May 7, 2019

yesamer commented May 8, 2019

jomarko left a comment

Choose a reason for hiding this comment

Code review notes, manual check pending.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yesamer May 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jomarko commented May 10, 2019

jomarko commented May 10, 2019

yesamer commented May 10, 2019 • edited Loading

danielezonca left a comment

Choose a reason for hiding this comment

gitgabrio left a comment • edited Loading

Choose a reason for hiding this comment

danielezonca commented May 10, 2019

yesamer commented May 13, 2019

yesamer commented Apr 23, 2019 •

edited

Loading

kkufova commented Apr 25, 2019 •

edited

Loading

yesamer May 9, 2019 •

edited

Loading

yesamer commented May 10, 2019 •

edited

Loading

gitgabrio left a comment •

edited

Loading