[feature] Added configuration variables to DeviceGroup #738

[pandafy]

Closes #738

[pandafy]

This part of the JS is affecting updating the "Configuration Variables" when the device group is changed in the admin.

openwisp-controller/openwisp_controller/config/static/config/js/widget.js

Lines 34 to 38 in 14f96ab

	Object.keys(defaultValues).forEach(function (key) {
	if (!contextValue.hasOwnProperty(key) && !systemContextValue.hasOwnProperty(key)) {
	contextValue[key] = defaultValues[key];
	}
	});

Consider the following scenario:

1. There's a group. "Access points" which set the configuration variables as {'ssid': 'OpenWISP}.
2. There's a template, "Wireless Radio", which set the configuration variable as {'ssid': 'placeholder}.
3. You select the "Wireless Radio" template on a device, and you see the device configuration variables populated. This is correct.
4. Now, change the group of the device to "Access Points".

Expected Result

You would expect the configuration variables to reflect {'ssid': 'OpenWISP}.

Actual Result

The configuration variables stay {'ssid': 'placeholder}

Why does this bug occur?

The aforementioned JS, only updates the configuration variables if the key is not defined in the System Defined Variables and not already defined in the Configuration Variables for the device.

So, when the configuration variables already has {'ssid': 'placeholder}, i.e. there's ssid key present before DeviceGroup was changed. Therefore, it does not update the configuration variable.

I guess, this was done to prevent overriding of configuration variables that were overridden by the user.

This bugs also appear when two templates have same configuration variable defined. The configuration variable field of the device does not update when we select the second template.

[nemesifier]

This part of the JS is affecting updating the "Configuration Variables" when the device group is changed in the admin.

openwisp-controller/openwisp_controller/config/static/config/js/widget.js

Lines 34 to 38 in 14f96ab

	Object.keys(defaultValues).forEach(function (key) {
	if (!contextValue.hasOwnProperty(key) && !systemContextValue.hasOwnProperty(key)) {
	contextValue[key] = defaultValues[key];
	}
	});

Consider the following scenario:

1. There's a group. "Access points" which set the configuration variables as {'ssid': 'OpenWISP}.
2. There's a template, "Wireless Radio", which set the configuration variable as {'ssid': 'placeholder}.
3. You select the "Wireless Radio" template on a device, and you see the device configuration variables populated. This is correct.
4. Now, change the group of the device to "Access Points".

Expected Result

You would expect the configuration variables to reflect {'ssid': 'OpenWISP}.

Actual Result

The configuration variables stay {'ssid': 'placeholder}

Why does this bug occur?

The aforementioned JS, only updates the configuration variables if the key is not defined in the System Defined Variables and not already defined in the Configuration Variables for the device.

So, when the configuration variables already has {'ssid': 'placeholder}, i.e. there's ssid key present before DeviceGroup was changed. Therefore, it does not update the configuration variable.

I guess, this was done to prevent overriding of configuration variables that were overridden by the user.

Makes sense.

What about adding a new section called just "Group variables"?

So the new precedence order would be:

1. System variables
2. Template default values
3. Organization variables ([feature] Convert group metadata to variables #738)
4. Group variables
5. Device variables

This bugs also appears when two templates have same configuration variable defined. The configuration variable field of the device does not update when we select the second template.

For this please open a new issue and add a reference to this discussion.

Preview config

Can you make sure the preview takes into account the group too?
E.g.: selecting a group and hitting preview should cause the group variables to be used in the preview.

[pandafy]

@nemesifier having a new "Group variable" section would not solve the problem I shared in my previous comment. I think, we would eventually have to address #781.

Having a separate section would only benefit UI aspects.

[coveralls]

coverage: 98.899%. first build when pulling 4134800 on issues/738-group-metadata-variables into 89a501d on master.

[nemesifier]

Documentation recommendations:

• add a reference to group variables in the explanation of configuration variables
• explain the precedence of the different type of variables (look at the code first) - I think this should go in the configuration variables section, we said we can have a list in the beginning of "How to use configuration variables" and link each section, then reorder the sections to make sure they follow the same order
• in the group section, please explain the differences between context and meta data (when to use one and when to use the other one), explain that both end up being available as variables, make sure to mention that both are available via REST API

[nemesifier]

maybe this can become one shorter point which refers to a new section below

[nemesifier]

I tried this on a device which already had a group assigned:

• I defined a variable in a template and set a default value for it
• I selected this template in the device just mentioned

I expected the configuration variables section of the device to show this.
However, this is not happening.

It works properly if the group is not assigned yet and I assign it.
Or if the group is already assigned and I unassign it and reassign it.

[pandafy]

@nemesifier When a device already has a group assigned, then the group's configuration variable is added to the "System Defined Variables" in the configuration section. Since the "System Defined Variables" have higher priority than "Default Values/Configuration Variables", therefore those variables are not added to the UI.

[nemesifier]

Are you sure global variables would override group variables? I think it should be the opposite, group variables should override any global variable.

[nemesifier]

Are you sure of this? We don't have context as search fields in the other models.
Looking through too many textfields could make the search slow if there's many rows in the DB.
For consistency I would remove it.

[nemesifier]

Great! 👍