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 cron and ping members to get last seen time #106

Merged
merged 2 commits into from
Aug 25, 2022
Merged

Conversation

NoobTW
Copy link
Contributor

@NoobTW NoobTW commented Aug 24, 2022

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: #40

What is the new behavior?

  • Add pingAll function in utils/ping.js to ping every members in a network, and update in database.
  • Add node-cron package and ping all networks every 5 minutes.

Does this introduce a breaking change?

  • Yes
  • No

Other information

There's no changes at the frontend side yet.

@NoobTW NoobTW requested a review from dec0dOS as a code owner August 24, 2022 23:56
@lgtm-com
Copy link

lgtm-com bot commented Aug 25, 2022

This pull request introduces 2 alerts when merging d3fdac6 into 54ec767 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable
  • 1 for Variable not declared before use

@dec0dOS
Copy link
Owner

dec0dOS commented Aug 25, 2022

Hello, @NoobTW!

Thanks a lot for your work. That is a great start to fully implement the last seen time feature. Initially, I thought that this feature would require much more effort.

There are some things to mention. In your PR you're deleting the yarn.lock file. Is this was a mistake or you're having some troubles in installing the dependencies?
Will you help with the frontend side or should I merge it in the current state in the dev branch?

@NoobTW
Copy link
Contributor Author

NoobTW commented Aug 25, 2022

In your PR you're deleting the yarn.lock file.

Yes, I believe this is a mistake from me because I use npm instead of yarn. Feel free to add it back, or maybe i can commit it later.

Will you help with the frontend side or should I merge it in the current state in the dev branch?

Before merging this PR, i want to check if ping members every 5 minutes is OK? if it's ok then I think you can merge it for now.

As for the frontend side, I would like to help with the frontend side but i don't know how to do it for now. (e.g., Can we get the lastSeen property from the current backend API? I haven't check for that yet.)

Last but not least, thank you for your great work for building this self-host controller and the great UI.

@dec0dOS
Copy link
Owner

dec0dOS commented Aug 25, 2022

Before merging this PR, i want to check if ping members every 5 minutes is OK? if it's ok then I think you can merge it for now.

I think that this value should be configurable. Anyway, 1 minute is a better default value for troubleshooting. Also, there should be a way to disable the 'last seen time' option.

@dec0dOS dec0dOS changed the base branch from main to dev August 25, 2022 16:08
@dec0dOS dec0dOS merged commit 6bf0f29 into dec0dOS:dev Aug 25, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Aug 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants