Fix missing retain on add_group, allow specifying of groupID (#2266)

* Accept json for add_group so a groupID can be provided, also set retain property on group creation

* Update util/settings tests for groupID and retain

* Update bridgeConfig tests for add_group with json

lib/extension/bridgeConfig.js

@@ -204,8 +204,29 @@ class BridgeConfig extends BaseExtension {
     }
 
     addGroup(topic, message) {
-        const name = message;
-        const group = settings.addGroup(name);
+        let id = null;
+        let name = null;
+        try {
+            // json payload with id and friendly_name
+            const json = JSON.parse(message);
+            if (json.hasOwnProperty('id')) {
+                id = json.id;
+                name = `group_${id}`;
+            }
+            if (json.hasOwnProperty('friendly_name')) {
+                name = json.friendly_name;
+            }
+        } catch (e) {
+            // just friendly_name
+            name = message;
+        }
+
+        if (name == null) {
+            logger.error('Failed to add group, missing friendly_name!');
+            return;
+        }
+
+        const group = settings.addGroup(name, id);
         this.zigbee.createGroup(group.ID);
         logger.info(`Added group '${name}'`);
     }

lib/util/settings.js

@@ -483,7 +483,7 @@ function removeDevice(IDorName) {
     write();
 }
 
-function addGroup(name) {
+function addGroup(name, ID=null) {
     if (getGroup(name) || getDevice(name)) {
         throw new Error(`friendly_name '${name}' is already in use`);
     }
@@ -493,12 +493,21 @@ function addGroup(name) {
         settings.groups = {};
     }
 
-    let ID = '1';
-    while (settings.groups.hasOwnProperty(ID)) {
-        ID = (Number.parseInt(ID) + 1).toString();
+    if (ID == null) {
+        // look for free ID
+        ID = '1';
+        while (settings.groups.hasOwnProperty(ID)) {
+            ID = (Number.parseInt(ID) + 1).toString();
+        }
+    } else {
+        // ensure provided ID is not in use
+        ID = ID.toString();
+        if (settings.groups.hasOwnProperty(ID)) {
+            throw new Error(`group id '${ID}' is already in use`);
+        }
     }
 
-    settings.groups[ID] = {friendly_name: name};
+    settings.groups[ID] = {friendly_name: name, retain: false};
     write();
 
     return getGroup(ID);

test/bridgeConfig.test.js

@@ -205,11 +205,38 @@ describe('Bridge config', () => {
         zigbeeHerdsman.createGroup.mockClear();
         MQTT.events.message('zigbee2mqtt/bridge/config/add_group', 'new_group');
         await flushPromises();
-        expect(settings.getGroup('new_group')).toStrictEqual({"ID": 3, "friendlyName": "new_group", "friendly_name": "new_group", devices: [], optimistic: true});
+        expect(settings.getGroup('new_group')).toStrictEqual({"ID": 3, "friendlyName": "new_group", "friendly_name": "new_group", "retain": false, devices: [], optimistic: true});
         expect(zigbeeHerdsman.createGroup).toHaveBeenCalledTimes(1);
         expect(zigbeeHerdsman.createGroup).toHaveBeenCalledWith(3)
     });
 
+    it('Should allow to add groups with json', async () => {
+        zigbeeHerdsman.createGroup.mockClear();
+        MQTT.events.message('zigbee2mqtt/bridge/config/add_group', '{"friendly_name": "new_group"}');
+        await flushPromises();
+        expect(settings.getGroup('new_group')).toStrictEqual({"ID": 3, "friendlyName": "new_group", "friendly_name": "new_group", "retain": false, devices: [], optimistic: true});
+        expect(zigbeeHerdsman.createGroup).toHaveBeenCalledTimes(1);
+        expect(zigbeeHerdsman.createGroup).toHaveBeenCalledWith(3)
+    });
+
+    it('Should allow to add groups with json specifying id', async () => {
+        zigbeeHerdsman.createGroup.mockClear();
+        MQTT.events.message('zigbee2mqtt/bridge/config/add_group', '{"friendly_name": "new_group", "id": 42}');
+        await flushPromises();
+        expect(settings.getGroup('new_group')).toStrictEqual({"ID": 42, "friendlyName": "new_group", "friendly_name": "new_group", "retain": false, devices: [], optimistic: true});
+        expect(zigbeeHerdsman.createGroup).toHaveBeenCalledTimes(1);
+        expect(zigbeeHerdsman.createGroup).toHaveBeenCalledWith(42)
+    });
+
+    it('Should allow to add groups with json specifying only id', async () => {
+        zigbeeHerdsman.createGroup.mockClear();
+        MQTT.events.message('zigbee2mqtt/bridge/config/add_group', '{"id": 42}');
+        await flushPromises();
+        expect(settings.getGroup('group_42')).toStrictEqual({"ID": 42, "friendlyName": "group_42", "friendly_name": "group_42", "retain": false, devices: [], optimistic: true});
+        expect(zigbeeHerdsman.createGroup).toHaveBeenCalledTimes(1);
+        expect(zigbeeHerdsman.createGroup).toHaveBeenCalledWith(42)
+    });
+
     it('Should allow to remove groups', async () => {
         settings.addGroup('to_be_removed')
         MQTT.events.message('zigbee2mqtt/bridge/config/remove_group', 'to_be_removed');
@@ -217,6 +244,13 @@ describe('Bridge config', () => {
         expect(settings.getGroup('to_be_removed')).toStrictEqual(null);
     });
 
+    it('Shouldnt allow add groups without id or friendly_name in json', async () => {
+        zigbeeHerdsman.createGroup.mockClear();
+        MQTT.events.message('zigbee2mqtt/bridge/config/add_group', '{}');
+        await flushPromises();
+        expect(logger.error).toHaveBeenCalledWith('Failed to add group, missing friendly_name!');
+    });
+
     it('Shouldnt do anything on unsupported topic', async () => {
         await flushPromises();
         MQTT.publish.mockClear();

test/settings.test.js

@@ -338,6 +338,21 @@ describe('Settings', () => {
         const expected = {
             '1': {
                 friendly_name: 'test123',
+                retain: false,
+            },
+        };
+
+        expect(settings.get().groups).toStrictEqual(expected);
+    });
+
+    it('Should add groups with specific ID', () => {
+        write(configurationFile, {});
+
+        const added = settings.addGroup('test123', 123);
+        const expected = {
+            '123': {
+                friendly_name: 'test123',
+                retain: false,
             },
         };
 
@@ -354,6 +369,24 @@ describe('Settings', () => {
         const expected = {
             '1': {
                 friendly_name: 'test123',
+                retain: false,
+            },
+        };
+
+        expect(settings.get().groups).toStrictEqual(expected);
+    });
+
+    it('Should not add duplicate groups with specific ID', () => {
+        write(configurationFile, {});
+
+        settings.addGroup('test123', 123);
+        expect(() => {
+            settings.addGroup('test_id_123', 123);
+        }).toThrow(new Error("group id '123' is already in use"));
+        const expected = {
+            '123': {
+                friendly_name: 'test123',
+                retain: false,
             },
         };
 
@@ -375,6 +408,7 @@ describe('Settings', () => {
         const expected = {
             '1': {
                 friendly_name: 'test123',
+                retain: false,
                 devices: ['0x123'],
             },
         };