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

include checkPresence in core of Gladys #437

Closed
wants to merge 3 commits into from
Closed

include checkPresence in core of Gladys #437

wants to merge 3 commits into from

Conversation

Jean-PhilippeD
Copy link
Contributor

Gladys Pull Request check-list

  • If your changes affects code, did your write the tests?
  • Are tests passing?
  • [x Is the linter (eslint) passing?
  • [-] If your changes modify the API (REST or Node.js), did you modify the documentation?

Description of change

I've included the check user presence in house, by adding 2 parameters in house model (check frequency and time before considering left), and can be different per house.

User won't need anymore to add an alarm, triggering a scenario which run a script (with a specific parameter set).

User just have to activate the check on a house, and change the value if needed.

By the way, I let the old feature active so nobody gonna loose working stuff.

Here is a screen on how it's implemented in the house view:
screenshot_20181030_223443

@Pierre-Gilles
Copy link
Contributor

Hey! Thanks for your work :)

One question I have, do you think it's really required to have one frequency/time before left house by house?

And even, do we want to expose that to users? It's not super user-friendly

I think it's really the kind of internal mechanism that doesn't need to be exposed and required...

What I see:

One default value for both that can be overridden by a Gladys param.

What do you think of that?

@Pierre-Gilles
Copy link
Contributor

Pierre-Gilles commented Oct 31, 2018

Maybe :

  • Default Frequency: every 2 minutes
  • Default time before considered left home: 10 minutes

It's just a suggestion

@Jean-PhilippeD
Copy link
Contributor Author

Jean-PhilippeD commented Oct 31, 2018

Why not, I admit it's not really user friendly while providing default values are avoiding useless questions to users, do you want me to adapt the code and provide a new PR ? Should I close this one ?

@Jean-PhilippeD
Copy link
Contributor Author

Hey,
I've implemented what you proposed.

@Pierre-Gilles
Copy link
Contributor

Thanks! I used your code but implemented it a little bit differently, it's now in the core.

@@ -350,9 +350,11 @@
"area-delete": "Supprimer",
"house-box-title": "Maisons",
"house-box-description": "Vous pouvez définir ici votre maison dans Gladys. Pour la latitude et la longitude, vous pouvez utiliser Google Maps pour trouver la latitude et la longitude votre maison, et la rentrer au format (par exemple) : latitude = 42.1, longitude = 43.2. Rappel: Toutes ces données restent en locale sur votre machine, et ne sont jamais envoyées nulle part.",
"house-box-check-occupation-description": "Vous pouvez activer la vérification cyclique de présence d'habitant dans votre/vos maison(s). Pour cela, il faut au paravant dispoer d'une une solution pour indiquer à Gladys votre présence régulière dans la maison. Par exemple, le module gladys-bluetooth permet cela au moyen d'équipement bluetooth. Cette vérification a lieu toutes les minutes par défaut, et Gladys consiidère qu'un habitant a quitté la maison s'il n'a pas été vu depuis 10min. Ces 2 valeurs peuvent être changé par des paramètres USER_CHECK_PRESENCE_FREQUENCY et USER_TIME_BEFORE_CONSIDERING_LEFT_HOME.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review before merging...
"il faut au paravant dispoer d'une une solution" should be "il faut auparavant disposer d'une solution".
"Gladys consiidère qu'un habitant" should be "Gladys considère qu'un habitant".
"Ces 2 valeurs peuvent être changé" should be "Ces 2 valeurs peuvent être changées".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR has not been merged in Gladys, as you can see I've closed the PR. Of course I review before merging :)

R6n0 pushed a commit to R6n0/Gladys that referenced this pull request Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants