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-5511: Focus test scenario grid when collection editor is saved… #1400

Merged
merged 1 commit into from Aug 11, 2020

Conversation

jomarko
Copy link

@jomarko jomarko commented Jul 17, 2020

… and closed

For more details see https://issues.redhat.com/browse/DROOLS-5511

@jomarko
Copy link
Author

jomarko commented Jul 17, 2020

The coverage needs to be provided, then draft will be transitioned to regular PR.

@jomarko
Copy link
Author

jomarko commented Jul 21, 2020

Jenkins please retest this

@jomarko jomarko marked this pull request as ready for review July 23, 2020 09:02
@jomarko
Copy link
Author

jomarko commented Jul 23, 2020

Jenkins do full downstream build

widget.addSaveEditorEventHandler(event -> {
flush();
gridPanel.setFocus(true);
});
Copy link
Member

@yesamer yesamer Jul 23, 2020

Choose a reason for hiding this comment

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

@jomarko Not related to your changes, but it could be an improvement. Currently if you press Close button the launched actions are:

  • destroyResources()
  • gridLayer.batch()
  • gridPanel.setFocus(true)
  • collectionEditorDOMElement.stopEditingMode()

While, for the Save button, the actions are:

  • flush() (with contains the destoyResource() logic)
  • gridPanel.setFocus(true)

Not sure about the effect of calling gridLayer.batch(), but I suspect we should at least call collectionEditorDOMElement.stopEditingMode(); after Save Button is pressed. Most probably the current behavior could lead to a bug, if the state of editing mode is wrong. Can you please check it?

Copy link
Author

Choose a reason for hiding this comment

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

The reason this is so tricky is the CollectionEditorSingletonDOMElementFacotry extends a class that was not designed for editors/popups with such complexity (buttons for saving, scrolling, switching two modes).

Other gird popups/editors are all saved by pressing TAB, ENTER (if it is not multiline text area) and canceled by ESC.

If we would able to redesign collection editor to not have any special buttons for saving, cancelling, then I think (not tried manually) we would simply remove this part of code and appformer framework would handle everything.

    protected void commonCloseHandling(final CollectionEditorDOMElement collectionEditorDOMElement) {
        destroyResources();
        gridLayer.batch();
        gridPanel.setFocus(true);
        collectionEditorDOMElement.stopEditingMode();
    }

    @Override
    public void registerHandlers(final CollectionViewImpl widget, final CollectionEditorDOMElement widgetDomElement) {
        widget.addCloseCompositeEventHandler(event -> commonCloseHandling(widgetDomElement));
        widget.addSaveEditorEventHandler(event -> {
            flush();
            gridPanel.setFocus(true);
        });
    }

However such rewriting is not much probable from my point of view, so back to your comment.

I think adding batch will not harm - documented here

collectionEditorDOMElement.stopEditingMode(); is not needed. The save handler call flush() what leads to invocation of CollectionEditorDOMElement.flush() where we already stop editing mode. It is expected, otherwise right now in business central we would see cell in edit mode even when collection editor is saved.

Copy link
Member

Choose a reason for hiding this comment

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

@jomarko Thank you for your exhaustive comment.

@jomarko
Copy link
Author

jomarko commented Jul 23, 2020

Jenkins do full downstream build

@jomarko
Copy link
Author

jomarko commented Jul 23, 2020

Jenkins do full downstream build

@jomarko
Copy link
Author

jomarko commented Jul 24, 2020

failed tests:

org.jbpm.process.workitem.mavenembedder.MavenEmbedderWorkitemHandlerTest.testCleanInstallSimpleProjecSynctWithCLOptions
org.jbpm.process.workitem.camel.karaf.itests.CamelWorkitemIntegrationTest.testSingleFileWithHeaders
org.jbpm.process.workitem.camel.karaf.itests.CamelWorkitemIntegrationTest.testSingleFileProcess
org.jbpm.process.workitem.mavenembedder.MavenEmbedderWorkitemHandlerTest.testCleanInstallSimpleProjectAsync

@jomarko
Copy link
Author

jomarko commented Jul 24, 2020

Jenkins do full downstream build

1 similar comment
@yesamer
Copy link
Member

yesamer commented Jul 31, 2020

Jenkins do full downstream build

@jomarko
Copy link
Author

jomarko commented Aug 4, 2020

Jenkins do full downstream build

@sonarcloud
Copy link

sonarcloud bot commented Aug 4, 2020

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

66.7% 66.7% Coverage
0.0% 0.0% Duplication

warning The version of Java (1.8.0_202) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

@jomarko
Copy link
Author

jomarko commented Aug 4, 2020

Jenkins do full downstream build

@jomarko
Copy link
Author

jomarko commented Aug 6, 2020

@jstastny-cz builds passed, PR is ready for your review when you have a moment.

@jomarko
Copy link
Author

jomarko commented Aug 11, 2020

@kiegroup/gatekeepers please merge

@karreiro karreiro merged commit 02daa6a into kiegroup:master Aug 11, 2020
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