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

KOGITO-1183: BPMN Editor - Imports not working. #3241

Merged
merged 1 commit into from Apr 9, 2020

Conversation

romartin
Copy link
Contributor

@romartin romartin commented Mar 24, 2020

Hey @inodeman @domhanak @LuboTerifaj

Here is the fix for BOTH: KOGITO-1183 and JBPM-9076

Thanks!

@romartin
Copy link
Contributor Author

Jenkins execute full downstream build

2 similar comments
@romartin
Copy link
Contributor Author

Jenkins execute full downstream build

@romartin
Copy link
Contributor Author

Jenkins execute full downstream build

@romartin
Copy link
Contributor Author

romartin commented Mar 26, 2020

@LuboTerifaj @domhanak

This PR is ready for testing. I should be quite easy from the UI to test, I already did again to ensure it works:

  1. just create a new process
  2. add some imports
  3. save,
  4. reopen
  5. check those imports are still there.

That will ensure the marshalling support that was missing for imports is now present. Please also remeber BC should be also retested.

PS: Also remember to first click on the canvas, before doing any action, to prevent this bug, which is already PR'ed.. sorry for the inconveniences

Thanks!!

Copy link
Contributor

@LuboTerifaj LuboTerifaj left a comment

Choose a reason for hiding this comment

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

Hello @romartin ,

I've found two issues. They are not caused by this PR, however because of the second issue, it is not possible to check the behaviour of Imports for all cases.

  • When you delete empty Data Type Imports that follows custom one, wrong element is deleted.

  • It is not possible to save the changes in Properties panel at all.

Can you please update headers from 2019 to 2020 too?

Thanks!

@LuboTerifaj
Copy link
Contributor

Reported issue: https://issues.redhat.com/browse/KOGITO-1621

@romartin
Copy link
Contributor Author

Hey @LuboTerifaj

I'd been rebasing this PR, I think that's the reason why you was not able to save any change in the process. I've also tested it on top of latest master upstream and upload some example video, which shows how properties are updated and saved properly, see this comment.

Also, I was not able to reproduce the issue you mention about the dropdown list and the remove button - what you mean by an "empty" data data type? Can you please give me some steps to reproduce? Anyway, if that's still reproducible, maybe we can report this issue and let this PR go, which at least makes the imports properly to work again. WDYT?

Thanks!

@romartin
Copy link
Contributor Author

Jenkins execute full downstream build

Copy link
Contributor

@inodeman inodeman left a comment

Choose a reason for hiding this comment

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

Hi @romartin Looks good to me :-)

@romartin romartin removed the request for review from domhanak March 31, 2020 20:59
@romartin
Copy link
Contributor Author

romartin commented Apr 1, 2020

Hey @LuboTerifaj this is ready to test again please

BTW the sonarcloud duplication checks are ok, we've duplication code on the marshallers, so nothing to do there for now.

thanks!

@LuboTerifaj
Copy link
Contributor

Hey @romartin , please see my comment. Thank you!

Copy link
Contributor

@LuboTerifaj LuboTerifaj left a comment

Choose a reason for hiding this comment

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

Hello @romartin

My findings:

  • In case management editor, add Imports , do not make any other changes. There is a message shown that there are no any changes to save.

  • If you add already existing Class Name in to Data Type Imports, it is not possible to add, update, delete or even see any class. When you close and open Imports back, all previously added class names are shown.

    • It shouldn't be possible to add the same class more than once.
    • The issue applies for both buisness-central and VSCode.
    • The issue is present also on master. We can report it as separate jira.
  • After opening and closing Imports (by clicking on OK) without any changes the process is in unsaved state in VSCode.

  • Headers have not been updated yet as commented in my previous comment.

Thanks!

@romartin
Copy link
Contributor Author

romartin commented Apr 7, 2020

Hey @LuboTerifaj

Great catches!!! I was able to reproce all above 3 issues. I'm trying to fix the one about duplicated data types, which is "brreaking" the list component, I would suggest to report the other ones for some future work anyway.

So will let you know something ASAP, thianks for the great testing being done here as well!

@romartin
Copy link
Contributor Author

romartin commented Apr 8, 2020

Jenkins execute full downstream build

@romartin
Copy link
Contributor Author

romartin commented Apr 8, 2020

Hey @LuboTerifaj

I've updated this PR by rebasing it and also fixing the issue you mentioned in the comment above, which was about the widget getting broken when "duplicating" some of the imports (issue #2):

  • The issue was present for any of boith default imports or wsdl imports, and it was not possible to delete the items after introducing some duplication (even custom data types or not as well)
  • The fix for the issue is not ideal, as it allows duplications, but at least the components doesn't get broken and the user is able to fix, by updating or removing the import, in case of duplication.

So I think we can report any other issues, and if you agree, go ahead with this. WDYT?

Thanks!

@romartin
Copy link
Contributor Author

romartin commented Apr 8, 2020

@LuboTerifaj artifacts generated

@romartin
Copy link
Contributor Author

romartin commented Apr 8, 2020

Jenkins retest this

Copy link
Contributor

@LuboTerifaj LuboTerifaj left a comment

Choose a reason for hiding this comment

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

Hello @romartin ,

  • The issue below was not fixed:

    • In case management editor, add Imports , do not make any other changes. There is a message shown that there are no any changes to save.
  • Headers have not been updated yet.

    • Can you please update them?
  • Reported issues that have been found during PR review:

    • JBPM-9100 Not possible to add Data Type Import containing <> characters
    • JBPM-9101 It is possible to add the same Data Type Imports

I can report the first mentioned issue since Case Management editor is only technology preview. I will do that in case it won't be fixed when the PR is ready to merge.

Thanks!

@romartin
Copy link
Contributor Author

romartin commented Apr 8, 2020

Oh @LuboTerifaj I forgot to update the headers sorry, but this will imply to rebuild everything and tomorrow is last chance of having this merged, so I would prefer trying to merge this as it is. I've updated IntelliJ to update headers for next times, but I don't think that should block it.

And yeah, I didn't fixed the CM, I just fixed the issue about duplicating imports, which has not trivial, so I had no more time.

So what's missing? Are you agree on approving/merging?

Thx!

@romartin
Copy link
Contributor Author

romartin commented Apr 8, 2020

Jenkins execute full downstream build

@romartin
Copy link
Contributor Author

romartin commented Apr 8, 2020

@LuboTerifaj rebased and fixed headers!! thanks!

@sonarcloud
Copy link

sonarcloud bot commented Apr 9, 2020

SonarCloud Quality Gate failed.

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

85.5% 85.5% Coverage
6.6% 6.6% Duplication

Copy link
Contributor

@LuboTerifaj LuboTerifaj left a comment

Choose a reason for hiding this comment

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

Hello @romartin ,

it looks good to me now.

I have reported also the issue with Case Management editor:

  • JBPM-9102 [Case Modeler] Changes in Imports cannot be saved without other changes

Thanks for the fixes!

@romartin
Copy link
Contributor Author

romartin commented Apr 9, 2020

thanks @LuboTerifaj !

@romartin romartin merged commit 31befbb into kiegroup:master Apr 9, 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
3 participants