Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add tedee to latest #3071

Merged
merged 1 commit into from
Dec 17, 2023
Merged

add tedee to latest #3071

merged 1 commit into from
Dec 17, 2023

Conversation

TA2k
Copy link
Contributor

@TA2k TA2k commented Dec 16, 2023

No description provided.

@github-actions github-actions bot added auto-checked This PR was automatically checked for obvious criterias must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review *📬 a new comment has been added labels Dec 16, 2023
@github-actions github-actions bot deleted a comment from TA2k Dec 16, 2023
@mcm1957 mcm1957 added RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. and removed must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review *📬 a new comment has been added labels Dec 16, 2023
@github-actions github-actions bot added the must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review label Dec 16, 2023
@github-actions github-actions bot deleted a comment from mcm1957 Dec 16, 2023
@github-actions github-actions bot deleted a comment from mcm1957 Dec 16, 2023
@mcm1957 mcm1957 removed the must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review label Dec 16, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented Dec 16, 2023

First of all - THANK YOU for the time and effort you spend to maintain this adapter.

I would like to give some feedback based on my personal oppinion. @Apollon77 might have additional suggestions or even a different oppinion to one or the other statement. Please feel free to contact him if you cannot follow my suggestions or want to discuss some special aspects.

  • Readme.md should contain a link to the manufacturer and/or device

    The README.md of any adapter related to device or m ultiple devices of a manufacturer should contain a link the manufacturers website and/or to a description of the device. This is to anable new users to easily check wether they associate the adapter with the correct device(s). (See https://github.com/ioBroker/ioBroker.repositories#requirements-for-adapter-to-get-added-to-the-latest-repository paragraph 4.)

  • Are you sure that adapter uses a cloud service?

    "connectionType": "cloud", is specified at https://github.com/TA2k/ioBroker.tedee/blob/add612d9fa0cefb7203c7c3ad26af8976d04a574/io-package.json#L77 but code looks like it is using local connection.

    Please check / adapt if local access only.

  • sensitive information, especially passwords should be stored encrypted

    You configuration seems to contain passwords (or other sensitive information) which is not stored encrypted. As long as those information is not stored inside a table you can encrypt and secure this infomation simply by adding the following configuration to io-package.json. The iob runtime will encrypt / decrypt the data as required; no addition programming effort is required. Note that users will need to reenter this info one time after adding encryption, so drop a note at release notes.

    "encryptedNative": [
      "password",
      "data2"
    ],
    "protectedNative": [
      "password",
      "data2"
    ],
    

    You have configured a parameter named 'password' to be encrypted. But you config uses a paramater named 'token' which should be protected like a password.
    Please adapt io-package.json.

  • sensitive information should not be logged into logfile

    To avoid publishing sensitive information when forwarding logs, those information should never be written into the logs but either omitted or at least partly masked so that the information cannot be misused. This relates at least to usernames, passwords, uniqui (user assigned) keys and tokens. In addition personal information like name, address, gegraphical coordinagtes should not be logged too.

  • invalid characters should be filtered from object ids

    Some characters are not allowed to be used as part of an object id. If an object id is not hardcoded or even depending on user input you should ensure that such an object id does not contain an invalid character. Iobroker provides the constant adapter.FORBIDDEN_CHARS, i.e. by using the following snippet:

    function name2id(pName) {
        return (pName || '').replace(adapter.FORBIDDEN_CHARS, '_');
    }
    

    You retrieve the 'id' used to create the device object by queryiing the (external) device and using the 'name'attribute. (https://github.com/TA2k/ioBroker.tedee/blob/add612d9fa0cefb7203c7c3ad26af8976d04a574/main.js#L107). As the content this data might change in future, please filter illegal characters before using the data to create states.

  • reevaluate state roles

    Only the values specified here (https://github.com/ioBroker/ioBroker.docs/blob/master/docs/en/dev/stateroles.md) may be used as state roles. Do not invent new names as this might disturb the functionalyity of other adapters or vis.

    In addition the roles MUST match the read/write functionality. As an example a role value.* requires that the state is a readable state. Input states (write only) should have roles like level.*. Please read the description carefully. States with role 'button' should (must) have attribute 'common.read=false' set. A button ( "Taster" in german only triggers when you press but else has no state), so it should also be not readable. This is also like HomeMatic does it. A switch has clear states true or false and so can be read.

    Please avoid using general roles (state, value) whnever a dedicated role exists.

    role 'boolean' is invalid, maybe you menat button (needs read:false) or switch, see https://github.com/TA2k/ioBroker.tedee/blob/1b328f631363df0666baf7ba7802382b29228cde/main.js#L158

  • consider using adapter.setTimeout / this.setTimeout instead of (standard) setTimeout

    The adapter package provides wrapper routines for native setTimeout, setInterval, clearTimeout and clearInterval. Using those routines ensures that timers are cancelled on on load. Additional checks on thomse limits might be performed, too. So consider replacing native setTimeout/clearTimeout by adapter.setTimeout/adapter.clearTimeout or this.setTimeout/this.clearTimeout. The same refers to setInterval/clearInterval.

  • check updateInterval / config.interval

    updateInterval is initialized and clered, but I did not see any place where it was used (no setInterval).

    Perameter config.interval exists and it limits to be in range. But I did not see any place where it is used.

  • github dependabot support seems to be missing

    It looks like you did not yet setup dependabot support; at least .github/dependaboit.yml is missing.

    Please consider setting up dependabot support (and to ease life the workflow auto-merge.yml). Dependabot will ensure that your adapter uses the most recent versions of external packages and avoids problem due to outdted base libraries. Dependabit provides PRs at you repository to update the dependencies. Depending on you setting, especially on auto-merge.yml configuration no or only minor revisions will be outmatically merged into you main branch. All major revisions msiut (ans should) be reviewed by you. (Note: That's a suggestion only and not blocking)

Thanks for reading and evaluating this suggestions.
McM1957

Please add a comment when you have reviewed and fixed the suggestionsor at least commented the suggestions and you think the adapter is ready for a re-review!

@mcm1957 mcm1957 added must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review and removed RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. labels Dec 16, 2023
@TA2k
Copy link
Contributor Author

TA2k commented Dec 16, 2023

  1. I added the information which devices a supported. But I will not add link or photos of the manufacture to prevent any risk of an Unterlassungsklage. Because this project is not official connected to the manufactures and I'm not allowed to give this impression when I use manufacture links. I also have not the right to use official device photos. A good example was the Vodafone speedtest adapter
  2. fixed
  3. fixed
  4. sensitive information are not logged
  5. fixed
  6. fixed
  7. I will use the native implementation to have the control over the behavior
  8. fixed
  9. GitHub dependabot brings no benefit in my opinion. It only spams the commit history and notifications. Major updates should not updated automatically and minor updates are applied automatically at installation because of the ^ syntax in the package.json. Normally a developer checks all dependencies before publishing a new release on npm

@github-actions github-actions bot added the *📬 a new comment has been added label Dec 16, 2023
@mcm1957 mcm1957 added RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. and removed must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review *📬 a new comment has been added labels Dec 16, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented Dec 17, 2023

RE-CHECK!

Copy link

Automated adapter checker

ioBroker.tedee

Downloads Number of Installations (latest)
NPM

👍 No errors found

  • 👀 [W400] Cannot find "tedee" in latest repository
  • 👀 [W515] Why you decided to disable i18n support?

Add comment "RE-CHECK!" to start check anew

@mcm1957
Copy link
Collaborator

mcm1957 commented Dec 17, 2023

Thanks for the ultrafast reaction.
Sorry, the logging issue was my fault; I forgot to remive this checkpoint from feedback.

I completly understand that using a companys logo might cause serious trobled. I personally do not think that a plain links to an official website of a manufacturer could be criticial - I did not read about any such claim. But I can and will respect your decision.

Same for Dependabot - I already mentioned that it is no blocking feedback.

@mcm1957 mcm1957 added lgtm Looks Good To Me and removed RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. labels Dec 17, 2023
@mcm1957 mcm1957 merged commit 1bd1a32 into ioBroker:master Dec 17, 2023
28 checks passed
@mcm1957
Copy link
Collaborator

mcm1957 commented Dec 17, 2023

This adapter has been released to latest repository and should be visible within 24h maximum.

Please create a thread at https://forum.iobroker.net/category/91/tester titled like "Test Adapter " to collect some user feedback and provide a link to this topic when requesting addition to stable repository later.

Note: If an other testing topic already exists, ist OK to continue using this topic too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-checked This PR was automatically checked for obvious criterias lgtm Looks Good To Me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants