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.

Sign up for GitHub

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

Jump to bottom

JBPM-8666: Stunner - Missing the ability to to specify the FEEL expressions #2768

Merged
merged 1 commit into from Jul 27, 2019
Merged

JBPM-8666: Stunner - Missing the ability to to specify the FEEL expressions #2768

merged 1 commit into from Jul 27, 2019

Conversation

inodeman
Copy link
Contributor

@romartin Can you review this PR, it is for the FEEL expression changes and the readme changes

@ghost
Copy link

ghost commented Jul 18, 2019

Can one of the admins verify this PR? Comment with 'ok to test' to start the build.

@romartin romartin requested review from hasys and romartin July 18, 2019 20:52
@romartin
Copy link
Contributor

ok to test

romartin
romartin reviewed Jul 18, 2019
View reviewed changes
kie-wb-common-stunner/README.md
@@ -171,7 +171,7 @@ The following assumes that the Standalone Showcase will be utilized. Some eviden
10. Ensure that the *with JavaScript debugger* checkbox is **NOT** checked, as Chrome debugger will be used instead.
11. Under *Before launch*, click the plus (+) sign.
12. Select *Run Maven Goal*.
13. Change the working directory to /[YOUR_DIR_LOCATION]/kie-wb-common/kie-wb-common-stunner/.
13. Change the working directory to /[YOUR_DIR_LOCATION]/kie-wb-common/kie-wb-common-stunner/kie-wb-common-stunner-showcase/kie-wb-common-stunner-showcase-standalone/.
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for fixing this! 👍

@romartin romartin requested a review from wmedvede July 18, 2019 20:56
romartin
romartin reviewed Jul 18, 2019
View reviewed changes
Copy link
Contributor

@romartin romartin left a comment

Choose a reason for hiding this comment

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

Hey @inodeman

Thanks for this, your first PR! :)

It looks good but a couple of comments:

  • So is there no need to change the code for marshallers? is it un(marshalling) the expression type (feel) and its value properly? just asking to be sure, because if so, much better! hehe
  • Missing tests for the changes. At least some test case should cover the new lines you added in both classes

Thanks!

@romartin
Copy link
Contributor

can you please also update the ticket's title with the righ ticket id and description plz?

@inodeman
Copy link
Contributor Author

@romartin I added the test you mentioned and also so that tests would not fail, can you review?

@inodeman inodeman changed the title Rhpam 2022 RHPAM-2022: Stunner - Missing the ability to to specify the FEEL expressions Jul 19, 2019
wmedvede
wmedvede approved these changes Jul 22, 2019
View reviewed changes
Copy link
Member

@wmedvede wmedvede left a comment •
edited

Choose a reason for hiding this comment

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

LGTM thanks.

Quick comment, if I don't remember wrong in master branch the trend is to use JBPM- prefixed tickets instead of "product" tickets like RHPAM-XXXX.

@romartin do this still apply?

@wmedvede
Copy link
Member

jenkins retest this

@hasys
Copy link
Member

hasys commented Jul 22, 2019

Jenkins execute full downstream build

1 similar comment
@romartin
Copy link
Contributor

Jenkins execute full downstream build

@romartin romartin changed the title RHPAM-2022: Stunner - Missing the ability to to specify the FEEL expressions JBPM-8666: Stunner - Missing the ability to to specify the FEEL expressions Jul 22, 2019
@romartin
Copy link
Contributor

Thanks for pointing this out @wmedvede !
@inodeman please as commented, do the rebase (and squash) and let us know once it's ready, including the builds ok for testing.
thx!

@inodeman
Copy link
Contributor Author

inodeman commented Jul 23, 2019 •
edited

Hi @romartin can you re-check this PR? thanks a bunch, it has been rebased and squashed into single commit

@wmedvede
Copy link
Member

jenkins execute full downstream build

@hasys
Copy link
Member

hasys commented Jul 24, 2019 •
edited

Check list

  • ACKs are set
  • Affected version is set property
  • Is realization aligned to jira description?
  • Both Jenkins jobs are green

Code review

  • Code coverage
  • License headers
  • Static analyser gates
  • Code Style

Acceptance Criteria

  • Set up FEEL expression for Exclusive gateway and execute both (true and false) branches at runtime
  • Tested with full downstream build
    • Firefox
    • Chrome

Found issues

Blocker, Critical and Major bugs and Blocker and Critical enhancements should be fixed before merge.

Unrelated issues & future improvements

  • JBPM-8670 - Visual Condition Expression editor doesn't block unknown languages
  • JBPM-8669 - It is not possible to enter negative value for Integer values during process start
  • JBPM-8668 - FEEL expression validation

Used resources

Processes for quick test can be found here: https://github.com/bpmn-tutorials/FEELBPMN

hasys
hasys approved these changes Jul 24, 2019
View reviewed changes
Copy link
Member

@hasys hasys left a comment

Choose a reason for hiding this comment

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

Hi @inodeman,

looks and works good, thank you! Congratulations with your first contribution! :)

@romartin romartin merged commit 97192dc into kiegroup:master Jul 27, 2019
@romartin
Copy link
Contributor

Thanks @inodeman , feel free now to close the ticket for this 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romartin romartin romartin left review comments

@hasys hasys hasys approved these changes

@wmedvede wmedvede wmedvede approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@inodeman @romartin @wmedvede @hasys