Problem/Motivation

Now that we started to hide configurable things if they don't make any sense at all, we should complete the steps.

The value type can be configured in all sensors although it is e.g. numeric for all aggregator sensors.

Proposed resolution

Put a configurableValueType property to the plugins and make the SensorForm consider it.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#6 configurable_type-2560213-6.patch8.21 KBgiancarlosotelo
#6 interdiff-3-6.txt2.26 KBgiancarlosotelo
#3 configurable_type-2560213-3.patch10.39 KBgiancarlosotelo

Comments

miro_dietiker created an issue. See original summary.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Sure, check all plugins and consider a fixed value type. Only leave it configurable if it makes sense.
(And extend the tests to check if it is properly persisted / applied.)

giancarlosotelo’s picture

giancarlosotelo commented
Status: Active » Needs review
StatusFileSize
new configurable_type-2560213-3.patch10.39 KB

Added the configurableValueType property, now can be added in any plugin to leave it configurable or not.

I am setting all the addable sensors with a default value like ConfigValue sensor with bool. All Agreggators with default value number. I am missing the queue sensor, not sure if could be no configurable.

Other default sensors that I think doesnt need a configurable option: CoreRequirement, UpdateStatus, UserFailLogin, UserIntegrity. Probably I am missing some others that also need this option.

I also extended tests with the watchdog,config value and viewdisplay aggregators that assert the default value type.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

Nice step, some improvements required.

  1. +++ b/src/Plugin/monitoring/SensorPlugin/ConfigValueSensorPlugin.php
@@ -25,6 +25,11 @@ class ConfigValueSensorPlugin extends ValueComparisonSensorPluginBase {
+  protected $configurableValueType = FALSE;

+++ b/src/Tests/MonitoringUITest.php
@@ -248,16 +248,14 @@ class MonitoringUITest extends MonitoringTestBase {
-    $this->assertOptionSelected('edit-value-type', 'bool');
...
+    $this->assertEqual($sensor_config->getValueType(), 'bool');

    We can not lock this. Not all config values are numeric or boolean. This needs to stay editable. 

  2. +++ b/src/Plugin/monitoring/SensorPlugin/CoreRequirementsSensorPlugin.php
@@ -39,6 +39,11 @@ class CoreRequirementsSensorPlugin extends SensorPluginBase implements ExtendedI
+  protected $configurableValueType = FALSE;

    Hmm... Followup identified! This sensor has no value (type) and thus a value label doesn't make sense at all. 

  3. +++ b/src/Plugin/monitoring/SensorPlugin/UpdateStatusSensorPlugin.php
@@ -27,6 +27,11 @@ class UpdateStatusSensorPlugin extends SensorPluginBase {
+  protected $configurableValueType = FALSE;

    This should then default to number... Is it? 

  4. +++ b/src/Plugin/monitoring/SensorPlugin/UserFailedLoginsSensorPlugin.php
@@ -25,6 +25,11 @@ class UserFailedLoginsSensorPlugin extends DatabaseAggregatorSensorPlugin {
+  protected $configurableValueType = FALSE;

    This extends DatabaseAggregator so should be numeric and non-configurable already. 

  5. +++ b/src/SensorPlugin/SensorPluginInterface.php
@@ -38,6 +38,14 @@ interface SensorPluginInterface extends PluginInspectionInterface, PluginFormInt
+  public function getConfigurableValueType();

    All other configurable flags are directly accessed through the value, not through a getter. Please do similarly.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Created issue for the value label followup #2560575: Hide value label for value type none

giancarlosotelo’s picture

giancarlosotelo commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-3-6.txt2.26 KB
new configurable_type-2560213-6.patch8.21 KB

1. Config value stays editable.
3. Installed sensors have a default value(install file) so we don't have to define one in the plugin when is non configurable.
4. Yes, removed.
5. They are directly accessed because all sensors extends from SensorPluginBase but in this case we are on the SensorForm
'#access' => $sensor_config->getPlugin()->getConfigurableValueType()
So the only way to access that protected variable is with a getter. Correct me if I am wrong or if is not the correct approach.

Now probably some others plugins need to be non editable, I will wait some feedback.

LKS90’s picture

LKS90 commented
Status: Needs review » Reviewed & tested by the community

Can confirm all the points have been addressed. Point 5 as well, I don't see a way to get the flag without a getter method in that context.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Reviewed & tested by the community » Fixed

Yeah that looks pretty fine. The Queue Size sensor also needs to hide value type and set to numeric.

Fixed, committed. Hope tests still pass... ;-)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

The last submitted patch, 3: configurable_type-2560213-3.patch, failed testing.

Status: Closed (fixed) » Needs work

The last submitted patch, 6: configurable_type-2560213-6.patch, failed testing.

juanse254’s picture

juanse254 commented
Status: Needs work » Closed (fixed)

The last submitted patch, 3: configurable_type-2560213-3.patch, failed testing.

Status: Closed (fixed) » Needs work

The last submitted patch, 6: configurable_type-2560213-6.patch, failed testing.

giancarlosotelo’s picture

giancarlosotelo commented
Status: Needs work » Closed (fixed)