Added feature config groups

[KeshavSoni2511]

Fix for issue #752

[KeshavSoni2511]

Hi @fgalan , the tests are passing on my local setup but here the tests are failing. Kindly look into the issue or anything I can fix. Thanks

[mapedraza]

Hi @fgalan , the tests are passing on my local setup but here the tests are failing. Kindly look into the issue or anything I can fix. Thanks

Hi @Keshav-NEC , thank for your willingness to contribute!

The tests only fails on Node V18, which version are you using in your local enviroment?. Would be great if you can review it using the same version (V18). I suggest to use n Node version manager to switch between different node versions

Probably those errors are related with https://github.com/telefonicaid/iotagent-node-lib/pull/1333

[KeshavSoni2511]

Hi @mapedraza , I have tested the setup on Node V16. I will test the setup on Node V18 and will share my findings.

[KeshavSoni2511]

Hi @fgalan , @mapedraza I have tested the setup on Node V18 and npm V9.5.1 and test cases are passing and also I have used the link n Node version manager shared by @mapedraza . Please find the logs of test cases of pull request #1375 and also I have attached the screenshot of passing test cases. Thanks.

[mapedraza]

As mentioned in my previous comment, you did not addressed the changes made in https://github.com/telefonicaid/iotagent-node-lib/pull/1333 in your new tests.

Take as an example the ones I noted above and check the rest of the codebase (I suggested 3, the tests indicates 5, so 2 are remaining)

[KeshavSoni2511]

As mentioned in my previous comment, you did not addressed the changes made in #1333 in your new tests.

Take as an example the ones I noted above and check the rest of the codebase (I suggested 3, the tests indicates 5, so 2 are remaining)

Hi @mapedraza , Thanks for the help and I have done changes as suggested by you and I am sharing the test cases screenshot.

[KeshavSoni2511]

Hi @mapedraza, If you got any chance to review this PR.

[mapedraza]

There are still tests failling. You need to solve them before merging this PR

[mapedraza]

@Keshav-NEC thank you so mucho for your contribution. I think it is quite easy to change the PR according to my feedback. Please, address all the comments and the general idea before reviewing again

[KeshavSoni2511]

Hi @mapedraza, As in this comment, https://github.com/telefonicaid/iotagent-node-lib/issues/752#issue-403146821 it is suggested to keep the backward compatibility intact. I have created the separate functions for /iot/services and /iot/configGroups to return the result in services:[] and configGroups:[] objects respectively. listGroups() will output the result into services: [] object here but listConfigGroups() will output the result into configGroups:[] object here.

As per @mapedraza, I need to delete the listConfigGroups() and other functions which are similar to previous one. And handle both APIs /iot/services and /iot/configGroups in the same function by using if/else or some other logic. Could you please confirm my understanding?

[mapedraza]

@Keshav-NEC as you stated, the idea is to not break previous methods and paths. My comment comes because of the duplicated logic you added:

function listGroups(service, limit, offset, callback) {
    const result = [];
    let skipped = 0;
    const filteredGroups = getConfigurationsByService(service);
    for (const i in filteredGroups) {
        if (registeredGroups.hasOwnProperty(filteredGroups[i])) {
            if (offset && skipped < parseInt(offset, 10)) {
                skipped++;
            } else {
                result.push(registeredGroups[filteredGroups[i]]);
            }
            if (limit && result.length === parseInt(limit, 10)) {
                break;
            }
        }
    }
    callback(null, {
        count: filteredGroups.length,
        services: result
    });
}

function listConfigGroups(service, limit, offset, callback) {
    const result = [];
    let skipped = 0;
    const filteredGroups = getConfigurationsByService(service);

    for (const i in filteredGroups) {
        if (registeredGroups.hasOwnProperty(filteredGroups[i])) {
            if (offset && skipped < parseInt(offset, 10)) {
                skipped++;
            } else {
                result.push(registeredGroups[filteredGroups[i]]);
            }

            if (limit && result.length === parseInt(limit, 10)) {
                break;
            }
        }
    }

    callback(null, {
        count: filteredGroups.length,
        configGroups: result
    });
}

Both codeblocks mentioned above differs just in one line, the name of the variable passed to the callback. Probably there are better ways to implement it without duplicating code, that supposes worst maintainability (duplicating logics and paths). There are different alternatives to achieve this, like controlling it at higher level, or a wrapping the function. Take the one you feel confident with it, but try not to duplicate the code.

[AlvaroVega]

What is the difference between this PR and these others: https://github.com/telefonicaid/iotagent-node-lib/pull/1198 and https://github.com/telefonicaid/iotagent-node-lib/pull/954. I guess two of there PRs should be closed.

[mapedraza]

What is the difference between this PR and these others: #1198 and #954. I guess two of there PRs should be closed.

Older PRs closed

[KeshavSoni2511]

Hi @mapedraza , I have updated the PR as per the review comments received. Please review, Thanks.

[KeshavSoni2511]

Hi @mapedraza, If you got any chance to review this PR.

[KeshavSoni2511]

Hi @fgalan, if you got any chance to review this PR

[mapedraza]

Hi @KeshavSoni2511, could you address what I noted in my previous comment?. It would be great if you complete everything so we can merge it

Thanks in advance!

Hi @KeshavSoni2511, than you so much for your amazing job. Now it is more aligned. Just some minor improvements points:

• [ ] 1): I can't see tests implementing cross functionality. I mean, creating groups using services and then retrieving it through configGroup endpoint. It should be aded in both ways, I mean, creating as services and configGroup and retrieving them with the opposite.
• [ ] 2) In order to configure the keyword used by the new endpoint, it would be great if instead of having a magic word, it is moved to the constans file (lib/constants.js), replacing each time in the code appears 'configgroups'. It should be configured for the API route and for the term used in the json response.

[KeshavSoni2511]

Hi @KeshavSoni2511, could you address what I noted in my previous comment?. It would be great if you complete everything so we can merge it

Thanks in advance!

Hi @mapedraza, Hope you are doing well. FYI, due to sudden layoff, I am not working with NEC anymore. I think anyone else from NEC can do requested changes.

[mapedraza]

Hi @KeshavSoni2511, could you address what I noted in my previous comment?. It would be great if you complete everything so we can merge it Thanks in advance!

Hi @mapedraza, Hope you are doing well. FYI, due to sudden layoff, I am not working with NEC anymore. I think anyone else from NEC can do requested changes.

Really sad to hear that. I will merge your PR into a prelanding branch, so the work you accomplished is not going to be be thrown.

Thank for your help