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-3974 : Guided Decision Table is not getting updated #2624

Merged
merged 1 commit into from May 14, 2019

Conversation

Rikkola
Copy link
Member

@Rikkola Rikkola commented May 6, 2019

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

Without this the dropdown just shows the code that should fetch the enumeration, while the documentation states that the use of ' quotes should let you use the code in the way this PR enables.

The fix changes
'Fact.field' : '(new org.company.DataHelper()).getList()'
to
'Fact.field[field]' : '(new org.company.DataHelper()).getList()'

This is the most simple way of getting this to work. I don't really want to touch the areas on the client side code since the unit test coverage is weak.

@jomarko @manstis I think you two are again the best to review. Thank you. Or of course happy to ping anyone else who might be able to review this.

@Rikkola Rikkola force-pushed the DROOLS-3974-dynamic-enums branch from 1f85fda to edb8648 Compare May 7, 2019 11:51
@Rikkola Rikkola marked this pull request as ready for review May 7, 2019 17:33
@manstis manstis requested review from manstis and jomarko May 9, 2019 11:32
Copy link
Member

@manstis manstis left a comment

Choose a reason for hiding this comment

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

Seems a reasonable workaround.

@manstis
Copy link
Member

manstis commented May 9, 2019

It looks like the workaround is yet to be approved by the Customer. Is this to be merged regardless?

Also, you may want to update the documentation to include this advanced case?

@manstis
Copy link
Member

manstis commented May 9, 2019

@Rikkola I'd like to understand better how this works!

I can confirm it does work; but looking through the source IDK how!

@Rikkola
Copy link
Member Author

Rikkola commented May 9, 2019

@manstis the documentation actually describes this case, but this did not work. I'll comment in code what happens.

@Rikkola
Copy link
Member Author

Rikkola commented May 9, 2019

Actually my mistake this exact situation is not in the docs, but we do state:

It is also possible to load dependent enumeration definitions dynamically from a helper class by enclosing the call to the helper class within quotation marks

The example then shows:

'Country.region[countryCode]' : '(new org.yourco.DataHelper()).getListOfRegions("@{countryCode}")'

final String field = key.substring(key.indexOf("#") + 1);
key = key + "[" + field + "]";
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Now this fix abuses the mechanism by depending on the same field the enum is for. The field has to exist, but we do not use it.
In the client side there is a check for if there are dependent enums. If there are get the enums from the backend. This is why it updates on each call.
The other option is to also change the client side, possibly in several locations since the behaviour is used on the guided editor. Since the test coverage is weak there this approach felt safer.

@jomarko
Copy link

jomarko commented May 10, 2019

@Rikkola some cleanup at first Rikkola#4

@Rikkola
Copy link
Member Author

Rikkola commented May 10, 2019

Thanks @jomarko I have had this issue with few PRs and when I tried to import the correct assert method Idea did not find the method. So I suspect I need to click something somewhere in my Idea settings, or just update it.

@jomarko
Copy link

jomarko commented May 10, 2019

@Rikkola
Copy link
Member Author

Rikkola commented May 10, 2019 via email

@Rikkola
Copy link
Member Author

Rikkola commented May 10, 2019

Need to double check. I did my checks with Guided Rule Editors, perhaps DTables has some issues.

@Rikkola
Copy link
Member Author

Rikkola commented May 13, 2019

@jomarko Ok. So tried with the provided test project and it was missing ' quotes, this marks the enum for dynamic load.

@@ -149,6 +151,16 @@ public void testInvalidDependentEnum_AdvancedEnum2() {
assertFalse(loader.hasErrors());
}

@Test
public void testInvalidDependentEnum_AdvancedEnumForDynamic() {
Copy link

Choose a reason for hiding this comment

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

I hope this is valid, so propose: testValidDependentEnum_AdvancedEnumForDynamic

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, it actually is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@Rikkola Rikkola force-pushed the DROOLS-3974-dynamic-enums branch from 1f491ca to 01b1638 Compare May 13, 2019 13:09
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.

@Rikkola one point to discuss.

The fix seems working for basic dynamic enums, like:
'Message.dropdown' : 'new com.myspace.testproject.Message().getList()'

however for dynamic dependent enums, like:
'Message.dropdown[control]' : 'new com.myspace.testproject.Message().getList(@{control})'

user need to reopen the guided rule to get proper values, is that ok?

@Rikkola
Copy link
Member Author

Rikkola commented May 13, 2019

@jomarko It should use the same reload mechanism with both, is the getList(@{control}) just the same as getList()? So that it returns a random list?

@jomarko
Copy link

jomarko commented May 13, 2019

@Rikkola I use slightly updated project, see https://issues.jboss.org/browse/DROOLS-3974

@jomarko
Copy link

jomarko commented May 14, 2019

I think my question is answered here https://issues.jboss.org/browse/DROOLS-2867, approving

@Rikkola
Copy link
Member Author

Rikkola commented May 14, 2019

@jomarko sounds like that is it.

@kiegroup/gatekeepers Please merge this.

@manstis manstis merged commit 4ddeb45 into kiegroup:master May 14, 2019
adrielparedes pushed a commit to adrielparedes/kie-wb-common that referenced this pull request May 17, 2019
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