search

ianperrin / MMM-NetworkScanner

A module for MagicMirror which determines the status of devices on the network based on their MAC or IP address
MIT License
107 stars 47 forks source link

Fix bug when both showOffline and showUnknown is enabled #35

Closed O5ten closed 6 months ago

O5ten commented 6 years ago

I have a lot of machines in the house aswell as airbnb-guests coming and going with devices so i want to keep track of who is in and not. With this module i was expecting that both showOffline and showUnknown works together to show offline mac-adresses aswell as unknown machines.

So i noticed that it wasn't, so if i turned off showing offline units with showOffline: false then mac-adresses for unknowns were showing up. So i started digging and it turned out that the part that was setting the offline-status were only mapping through the configured devices. It saved only them when creating the new state if showOffline was enabled. The same behavior can still be enabled but by using showUnknown: false of course.

Instead we now add the missing configured devices to the nextState and loop over that collection instead.This allows us to have both settings enabled at the same time as in the example below.

Example with one machine Cybercom turned off and one machine unconfigured machines

O5ten commented 6 years ago

@ianperrin no longer actively maintaining the project perhaps?

ianperrin commented 6 years ago

@O5ten - @E3V3A and myself have put a fair amount of effort into the module recently to restore and improve functionality, so it’s definately not abandoned. However, given other commitments I cannot continually work on the module so efforts are sporadic.

Thanks for your pull request. Can you confirm you have tested your changes with various combinations of config options using both MAC addresses and IP addresses?

O5ten commented 6 years ago

Yes, i have tested the various combinations of showOffline and showUnknown with devices defined with MacAddress configuration as that is the only type of device that is affected by those configuration options.

I'm fairly confident that i haven't broken anything as this is used and inspected daily in my own mirror. But then again, it's hard to be sure without proper tests. :)

ianperrin commented 6 years ago

Great, thanks.

If you can give me sometime to test the changes with ipAddresses before accepting the pull request, that would be appreciated.

If you want to write some tests...

On 10 Jun 2018, at 20:04, Mikael Östberg notifications@github.com wrote:

Yes, i have tested the various combinations of showOffline and showUnknown with devices defined with MacAddress configuration as that is the only type of device that is affected by those configuration options.

I'm fairly confident that i haven't broken anything as this is used and inspected daily in my own mirror. But then again, it's hard to be sure without proper tests. :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

O5ten commented 6 years ago

No problem, i'll look into how testing is intended to be made in MagicMirror and give it a go.

ianperrin commented 6 years ago

@O5ten - I haven't found a common testing approach for MM modules. However I have now set up Travis CI in the master branch. It's currently failing due to a number of lifting issues but the concept is there certainly to test the node helper file. Feel free to take a look.

E3V3A commented 6 years ago

@ianperrin @O5ten Hi Guys! Sorry I've been too busy lately, with no time at all for MM stuff. Not sure how long this will be until I can get a chance to have look at this again. This PR look like a good improvement though... but need testing -- of course.

O5ten commented 6 months ago

I'll just close this as 6 years seems a bit excessive for testing. :)