Xiaomi module #506

damalgos · 2019-07-03T13:39:47Z

Pull Request check-list

To ensure your Pull Request can be accepted as fast as possible, make sure to review and check all of these items:

If your changes affects code, did your write the tests?
Are tests passing? (npm test on both front/server)
Is the linter passing? (npm run eslint on both front/server)
Did you run prettier? (npm run prettier on both front/server)
If your changes modify the API (REST or Node.js), did you modify the documentation? (Documentation is based on comments in code)

NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.

Description of change

Premier module xiaomi qui permet d'ajouter les capteurs de température et d'humidité.

Ajout du module dans la partie intégration du front
Modification des noms données aux capteurs de températures et humidités
Association du capteur à une pièce
Mise à jour des valeurs automatique (en fonction du gateway)

Lister parmis les modules:

Vue du module:

Rendu final sur la partie pièce:

Pierre-Gilles

Hello!

Merci pour ta PR :)

Je t'ai fais une série de feedbacks pour que tu puisses corriger cette PR.

Quelques remarques:

Attention aux variables en français, on code en anglais, pas en français.
Il y a quelques soucis de flow de donnée, je les ai mis en feedbacks.
Tu as coché la case "j'ai bien écris les tests", mais tu n'as pas écris de tests...

Rien de négatif, juste des trucs à corriger :)

front/src/config/i18n/en.json

front/src/routes/integration/all/xiaomi/XiaomiCapteurTemperature.jsx

server/services/xiaomi/lib/listen.js

damalgos · 2019-07-07T18:54:27Z

Maintenant on peut ajouter et supprimer des capteurs. Les capteurs sont stockés en RAM.

Au niveau du front, la vue est différente:

codecov · 2019-07-07T19:00:49Z

Codecov Report

Merging #506 into master will decrease coverage by 0.21%.
The diff coverage is 72.09%.

@@            Coverage Diff             @@
##           master     #506      +/-   ##
==========================================
- Coverage   91.14%   90.93%   -0.22%     
==========================================
  Files         334      339       +5     
  Lines        3794     3837      +43     
==========================================
+ Hits         3458     3489      +31     
- Misses        336      348      +12

Flag	Coverage Δ
#server	`90.93% <72.09%> (-0.22%)`	⬇️

Impacted Files	Coverage Δ
server/services/index.js	`100% <100%> (ø)`	⬆️
server/services/xiaomi/index.js	`100% <100%> (ø)`
...er/services/xiaomi/lib/event/xiaomi.addSensorTh.js	`30% <30%> (ø)`
...services/xiaomi/lib/commands/xiaomi.getSensorTh.js	`50% <50%> (ø)`
server/services/xiaomi/api/xiaomi.controller.js	`50% <50%> (ø)`
server/services/xiaomi/lib/index.js	`93.75% <93.75%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d48fb2f...c3dee99. Read the comment docs.

codecov · 2019-07-07T19:00:49Z

Codecov Report

❗ No coverage uploaded for pull request base (master@89a87d8). Click here to learn what that means.
The diff coverage is 96.18%.

@@            Coverage Diff            @@
##             master     #506   +/-   ##
=========================================
  Coverage          ?   91.55%           
=========================================
  Files             ?      356           
  Lines             ?     4013           
  Branches          ?        0           
=========================================
  Hits              ?     3674           
  Misses            ?      339           
  Partials          ?        0

Flag	Coverage Δ
#server	`91.55% <96.18%> (?)`

Impacted Files	Coverage Δ
server/utils/constants.js	`100% <ø> (ø)`
...rver/services/xiaomi/lib/event/xiaomi.listening.js	`100% <100%> (ø)`
...er/services/xiaomi/lib/event/xiaomi.addThSensor.js	`100% <100%> (ø)`
...erver/services/xiaomi/lib/event/xiaomi.getError.js	`100% <100%> (ø)`
...es/xiaomi/lib/event/xiaomi.addTemperatureSensor.js	`100% <100%> (ø)`
...xiaomi/lib/event/xiaomi.updateTemperatureSensor.js	`100% <100%> (ø)`
server/services/xiaomi/api/xiaomi.controller.js	`100% <100%> (ø)`
server/services/xiaomi/lib/index.js	`100% <100%> (ø)`
...ervices/xiaomi/lib/event/xiaomi.addMotionSensor.js	`100% <100%> (ø)`
...r/services/xiaomi/lib/commands/xiaomi.getSensor.js	`100% <100%> (ø)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89a87d8...39e8cec. Read the comment docs.

server/services/xiaomi/lib/event/xiaomi.addSensorMagnet.js

server/services/xiaomi/lib/index.js

server/services/xiaomi/api/xiaomi.controller.js

Pierre-Gilles

Hello!

Top pour les changements :)

J'ai noté quelques feedbacks sur cette PR.

Je t'ajoute des commentaires plus généraux ici:

L'image du service Xiaomi n'est pas au bon format:

Le format est 800x534. ça fait pas propre là sur la vue!

Je ne suis pas fan de la select box en haut à droite, ma question: pourquoi as tu voulu séparer les capteurs (autant côté client que côté serveur)? Tu t'es compliqué la vie!

Je pense que tu n'as besoin (comme pour le Z-Wave) que d'une variable en RAM, que d'une route d'API, et ensuite côté client tu peux éventuellement rajouter du filtering si nécessaire, mais selon moi là l'UX est alourdit pour pas grand chose. Surtout que des capteurs Xiaomi il n'y en a pas que 2, il y en a des tonnes, donc ça va vite devenir très lourd d'adapter le service pour les autres types de capteurs.

server/services/xiaomi/lib/index.js

server/services/xiaomi/lib/commands/xiaomi.getSensorTh.js

server/services/xiaomi/index.js

front/src/routes/integration/all/xiaomi/sensorMagnet/actions.js

damalgos · 2019-07-09T09:06:42Z

@Pierre-Gilles

Je vois pour l'UI du coup je vais revenir à une seule card qui permet de lister tous les devices.

Par contre je vois pas comment tu veux que je rajouter les device en fait ... J'ai pas trop d'idée.

damalgos · 2019-07-10T09:04:06Z

Pour ce qui est des changements, j'ai fait:

Modification de la taille de l'image Xiaomi
Une route unique permet de récupérer tous les capteurs
Une route spécifique par capteurs permet de récupérer uniquement certains capteurs (températures, mouvements, ...)
Test sur toutes les fonctions

Les choses à améliorer:

Ajout d'un nouveau capteur (tu trouves ça pas forcément top :)
Lors des tests actuels, aucun capteur xiaomi n'est présent dans la base de données, du coup il ne peut pas modifier la valeur d'un capteur et donc une petite partie du code n'est pas testé. Je sais pas trop quelle est la best practice pour ajouter un capteur dans la bdd de test et ensuite modifier sa dernière valeur.

Les problèmes:

Au niveau du SID dans le nom j'ai encore du mal à voir comment faire pour mieux le gérer ... Etant donné qu'il est unique il faut récupérer tous les capteurs présents dans la base de données, ensuite trouver quel numéro lui associer. Je trouve ça justement lourd ... Pourquoi pas laisser libre l'utilisateur ?

damalgos · 2019-07-16T08:19:17Z

Actuellement en cours de changement du code pour pouvoir récupérer plus de device et être compatible avec toutes les gateways.

Pierre-Gilles

Hello!

Super boulot général, je t'ai fais une review avec tous les feedbacks que j'ai en lisant ton code. Pour l'instant c'est juste une analyse du code, je n'ai pas exécuté le code vu que pour l'instant les tests n'ont pas l'air de passer.

Petites remarques sur l'UI, en lançant la démo de ton frontend je vois ça:

Petite comparaison avec le service Z-Wave, il faut que ce soit plus user-friendly à mon avis et moins "tech".

J'ai noté le problème de nommage des devices et je pense que c'est une réflexion à avoir au niveau de Gladys, j'y réfléchis de mon côté :)

server/services/xiaomi/api/xiaomi.controller.js

server/services/xiaomi/lib/event/xiaomi.addMagnetSensor.js

server/services/xiaomi/lib/event/xiaomi.addTemperatureSensor.js

server/services/xiaomi/lib/event/xiaomi.addThSensor.js

server/test/services/xiaomi/index.test.js

front/src/config/i18n/en.json

Pierre-Gilles · 2019-07-18T11:34:45Z

On vient de tester le service et à priori ça fonctionne (sur MacOS) !

Par contre l'écoute du broadcast ne fonctionne pas sur windows, mais on pense que le fonctionnement du réseau est différent (peut-être un truc à adapter pour windows)

damalgos · 2019-07-18T11:41:04Z

Super ! Pour ma part j'ai fais pas mal de changement !

Le code est entièrement testé, coverage 100%.

Deux menus sont présent:

Xiaomi sensors
Setup

Pour le premier:

Il permet de visualiser les capteurs déjà ajouté.

Pour le deuxième:

Le service recherche automatiquement les device disponibles sur le réseau et les ajoute dans cette partie. On peut ensuite créer le device avec le bouton create.

damalgos · 2019-07-18T12:17:54Z

Le module ne fonctionne pas sur windows, il y a un soucis avec le broadcast constaté sur d'autres sites:
nodejs/node-v0.x-archive#5868

Pierre-Gilles · 2019-09-10T06:19:09Z

Hello @damalgos ! Je suis de retour de congés, je regarde une par une les PR.

Où en es-tu sur cette PR ?

damalgos · 2019-09-17T10:03:52Z

Salut,

Au niveau de cette PR, il me reste à ajouter pour chaque capteur la valeur du SID pour pouvoir l'associer à la bonne pièce. Ensuite je pense corriger les éventuelles bugs et problème qui sont présent.

Après je pense que cela sera bon pour pousser la PR. A voir avec toi :)

Pierre-Gilles · 2019-09-20T06:58:51Z

@damalgos Top, tiens moi au courant dès que je peux faire une review du code !

Pierre-Gilles · 2019-10-04T16:14:38Z

Merged in #537

thibautmery added 3 commits July 3, 2019 10:27

add service xiaomi (front and server)

2cb6416

before the pull request

e437321

update fix before pull request

2c3e01b

Pierre-Gilles requested changes Jul 3, 2019

View reviewed changes

thibautmery added 4 commits July 5, 2019 18:36

update fix of PR for xiaomi service

c938d72

add test and new structure of xiaomi service

ce06fb0

add and delete device in the interface

49441fb

update UI for xiaomi service

e8f92c8

Merge branch 'master' into xiaomi

c3dee99

thibautmery added 2 commits July 8, 2019 07:38

update xiaomi service with new icon and add magnet sensor compatibility

bee87b3

Merge branch 'xiaomi' of github.com:damalgos/Gladys into xiaomi

42f0ba0

Pierre-Gilles reviewed Jul 9, 2019

View reviewed changes

server/services/xiaomi/lib/event/xiaomi.addSensorMagnet.js Outdated Show resolved Hide resolved

server/services/xiaomi/lib/index.js Outdated Show resolved Hide resolved

server/services/xiaomi/api/xiaomi.controller.js Outdated Show resolved Hide resolved

Pierre-Gilles requested changes Jul 9, 2019

View reviewed changes

thibautmery added 9 commits July 9, 2019 16:25

update xiaomi UI, add some test and add new device detection

e36d051

update small fix of lint

affeb58

fix prettier and eslint

109434b

update test for xiaomi service

b64fb4c

update

4537895

update test xiaomi service

d765dcf

fix prettier eslint issue

b372bce

update

45b0940

forget prettier

8a8f356

damalgos closed this Jul 10, 2019

damalgos reopened this Jul 10, 2019

update test xioami service

076a8d6

thibautmery added 2 commits July 13, 2019 15:21

update xiaomi module issue

ff53465

update socket to add more device

8cafbc7

Pierre-Gilles requested changes Jul 17, 2019

View reviewed changes

thibautmery added 7 commits July 17, 2019 17:21

update xiaomi no prettier no eslint, have to push to save it

efba743

update xiaomi module

46efa11

update test

7ec098b

update test for onMessage function

3add76e

update prettier

6eecd19

update test for battery quantity

2865ba2

update xioami service render

f1c5be8

thibautmery added 7 commits July 18, 2019 14:21

update service xiaomi fix problem

ad9b8d9

update xiaomi device issue and add new device

b3c152e

add new front file

e29ace6

update

0de0f43

update front setting xiaomi

4ec1878

update test

82af553

update translate file

44f5dfe

thibautmery and others added 3 commits September 17, 2019 13:49

add SID in view

099762f

Merge branch 'master' into xiaomi

ffdb610

update sensor issue due to device const change (tag)

e119b71

Merge branch 'master' into xiaomi

39e8cec

Pierre-Gilles closed this Oct 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Xiaomi module #506

Xiaomi module #506

damalgos commented Jul 3, 2019 •

edited

Pierre-Gilles left a comment

damalgos commented Jul 7, 2019

codecov bot commented Jul 7, 2019

codecov bot commented Jul 7, 2019 •

edited

Pierre-Gilles left a comment

damalgos commented Jul 9, 2019

damalgos commented Jul 10, 2019

damalgos commented Jul 16, 2019

Pierre-Gilles left a comment

Pierre-Gilles commented Jul 18, 2019

damalgos commented Jul 18, 2019

damalgos commented Jul 18, 2019

Pierre-Gilles commented Sep 10, 2019

damalgos commented Sep 17, 2019

Pierre-Gilles commented Sep 20, 2019

Pierre-Gilles commented Oct 4, 2019

Xiaomi module #506

Xiaomi module #506

Conversation

damalgos commented Jul 3, 2019 • edited

Pull Request check-list

Description of change

Pierre-Gilles left a comment

Choose a reason for hiding this comment

damalgos commented Jul 7, 2019

codecov bot commented Jul 7, 2019

Codecov Report

codecov bot commented Jul 7, 2019 • edited

Codecov Report

Pierre-Gilles left a comment

Choose a reason for hiding this comment

damalgos commented Jul 9, 2019

damalgos commented Jul 10, 2019

damalgos commented Jul 16, 2019

Pierre-Gilles left a comment

Choose a reason for hiding this comment

Pierre-Gilles commented Jul 18, 2019

damalgos commented Jul 18, 2019

damalgos commented Jul 18, 2019

Pierre-Gilles commented Sep 10, 2019

damalgos commented Sep 17, 2019

Pierre-Gilles commented Sep 20, 2019

Pierre-Gilles commented Oct 4, 2019

damalgos commented Jul 3, 2019 •

edited

codecov bot commented Jul 7, 2019 •

edited