Add CRITICAL flag to config parameters

[cdjackson]

This adds a critical flag to config parameters. This flag should indicate that a parameter is considered critical, or dangerous in that it could damage the system if used incorrectly. A GUI might provide an "Are you sure" prompt when a user changes such a parameter to provide a degree of protection against inadvertent misuse.

(note this isn't fully tested - I wanted to get feedback first)

Signed-off-by: Chris Jackson chris@cd-jackson.com

[tomhoefer]

I think this enhancement makes sense ... Only a few questions / remarks:

• Are these kind of parameters also to be flagged as advanced?
• What do you think about using the term confirmation instead of critical?
• You should also extend the ConfigDescriptionParameterBuilderTest

[cdjackson]

Are these kind of parameters also to be flagged as advanced?

Probably, but I don't think the two flags are linked in any way. You might have a parameter that is 'critical', but part of 'normal work' so might not be marked as advanced. In general I would think most 'critical' parameters would likely be advanced as well, but it doesn't preclude non-advanced parameters.

What do you think about using the term confirmation instead of critical?

That's fine by me.

You should also extend the ConfigDescriptionParameterBuilderTest

Of course - I just didn't do this yet ;)

[kaikreuzer]

considered critical, or dangerous in that it could damage the system

This sounds to me as if those parameters are not used for the handler configuration, but rather for device configuration or do you see any possibility to damage the system just by having a wrong handler configuration...?
So the introduction of such a flag is probably related to the outcome of this discussion, would you agree?

[cdjackson]

Maybe "damage the system" is the wrong term as there's probably no such thing as "damage". But for example to reset the handler might want a confirmation to avoid having to type in a whole load of additional reconfiguration.

I believe that this should be a general flag - not just for device configuration.

[kaikreuzer]

I'm ok with that. Just still wondering about a better name; in general, the config description should specify the purpose, but have no direct relation to what a UI makes out of that, so "confirmation" feels a bit to UI-related, while "critical" sounds too critical ;-) How about something like "verify=true"? This would also leave the possibility to have other verification mechanisms than the user through the UI.

[tomhoefer]

verify or verification is fine for me

[cdjackson]

Also fine by me. I’ll look to turn this into a proper PR.

[maggu2810]

@cdjackson Do you still consider to finish this PR?

[cdjackson]

Yes - I will look at this over the next few days.

[cdjackson]

I've updated this - tests seem to be failing at the moment but they are running ok locally here and the problem appears to be timeouts or something unrelated to this...

[cdjackson]

I've rebased this again to fix the recent merge conflicts.

[maggu2810]

As the last discussion has been only about the name and the name has been changed, I will merge this one.

[maggu2810]

@cdjackson The HIPP and the Travis builds fail now. Do you think it could be related to this changes?

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running org.eclipse.smarthome.config.xml.test.ConfigDescriptionI18nTest
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.002 sec - in org.eclipse.smarthome.config.xml.test.ConfigDescriptionI18nTest
assert config decriptions were localized(org.eclipse.smarthome.config.xml.test.ConfigDescriptionI18nTest)  Time elapsed: 0.581 sec
Running org.eclipse.smarthome.config.xml.test.ConfigDescriptionsTest
Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.305 sec <<< FAILURE! - in org.eclipse.smarthome.config.xml.test.ConfigDescriptionsTest
assert that ConfigDescriptions of fragment host were loaded properly(org.eclipse.smarthome.config.xml.test.ConfigDescriptionsTest)  Time elapsed: 0.157 sec
assert that ConfigDescriptions were loaded properly(org.eclipse.smarthome.config.xml.test.ConfigDescriptionsTest)  Time elapsed: 0.14 sec  <<< FAILURE!
java.lang.AssertionError: 
Expected: is <true>
     but: was <false>
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.junit.Assert.assertThat(Assert.java:865)
	at org.junit.Assert.assertThat(Assert.java:832)
	at org.junit.Assert$assertThat.callStatic(Unknown Source)
	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallStatic(CallSiteArray.java:53)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callStatic(AbstractCallSite.java:157)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callStatic(AbstractCallSite.java:169)
	at org.eclipse.smarthome.config.xml.test.ConfigDescriptionsTest$_assert_that_ConfigDescriptions_were_loaded_properly_closure7.doCall(ConfigDescriptionsTest.groovy:108)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:90)
	at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:324)
	at org.codehaus.groovy.runtime.metaclass.ClosureMetaClass.invokeMethod(ClosureMetaClass.java:278)
	at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1016)
	at groovy.lang.Closure.call(Closure.java:423)
	at groovy.lang.Closure.call(Closure.java:439)
	at org.codehaus.groovy.runtime.DefaultGroovyMethods.with(DefaultGroovyMethods.java:235)
	at org.codehaus.groovy.runtime.dgm$614.invoke(Unknown Source)
	at org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite$PojoMetaMethodSiteNoUnwrapNoCoerce.invoke(PojoMetaMethodSite.java:271)
	at org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite.call(PojoMetaMethodSite.java:53)
	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:45)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:108)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:116)
	at org.eclipse.smarthome.config.xml.test.ConfigDescriptionsTest.assert that ConfigDescriptions were loaded properly(ConfigDescriptionsTest.groovy:104)

Running org.eclipse.smarthome.config.xml.test.AbstractAsyncBundleProcessorTest
Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.651 sec - in org.eclipse.smarthome.config.xml.test.AbstractAsyncBundleProcessorTest
assert throwing exceptions does not break loading(org.eclipse.smarthome.config.xml.test.AbstractAsyncBundleProcessorTest)  Time elapsed: 0.224 sec
assert only relevant bundle configurations are loaded(org.eclipse.smarthome.config.xml.test.AbstractAsyncBundleProcessorTest)  Time elapsed: 0.106 sec
assert bundle configurations are loaded(org.eclipse.smarthome.config.xml.test.AbstractAsyncBundleProcessorTest)  Time elapsed: 0.205 sec
assert waiting for irrelevant and unkown bundles does not block(org.eclipse.smarthome.config.xml.test.AbstractAsyncBundleProcessorTest)  Time elapsed: 0.003 sec
assert listeners informed when loading completed(org.eclipse.smarthome.config.xml.test.AbstractAsyncBundleProcessorTest)  Time elapsed: 1.109 sec

Results :

Failed tests: 
  ConfigDescriptionsTest.assert that ConfigDescriptions were loaded properly:104 
Expected: is <true>
     but: was <false>

Tests run: 8, Failures: 1, Errors: 0, Skipped: 0

[cdjackson]

Tests were passing here when I ran them at the time I did the change a couple of months back - I’ll pull in the latest and check.

[cdjackson]

I wouldn’t like to state that it’s unconnected, but I can’t work out where the issue is if it is. The exceptions are pointing to something in the SyntheticBundleInstaller where bundleContext is apparently null.

[sjsf]

Are you sure the above comment was meant to go here? I can't see any relation of the SyntheticBundleInstaller to the test failure above.

I just looked into the test failure. And I'm sorry to say, but to me it looks like it is absolutely related to this PR. You forgot to adapt the ConfigI18nLocalizationService. It doesn't copy over the verify property and therefore falls back to the default (false). So the test indeed is correct, it revealed this flaw.

As I'm on it anyway, I will quickly created a PR to fix this.

[cdjackson]

When I tried running the tests in the IDE, it gave an NPE due to bundleContext being null. Maybe this was unrelated and there's some inconsistency with my environment, but it was stopping me from debugging this last night - hence the comment.

[sjsf]

ah, I see. The launch config worked ootb for me, that's why I got confused. But good that you left the comment, so I took a look.

Btw, a null bundleContext means that the bundle was not yet started. As it should be the "host" bundle of the test fragment, this indeed sounds like an inconsistency.

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/recovering-from-hard-reset/74848/8