versx / WhMgr

Discord notification system that works with RealDeviceMap and reports Pokemon, Raids, Eggs, Quests, Pokestop Lures, Team Rocket Invasions, Gym team changes, and Weather as embed messages. Discord users can also subscribe to custom Pokemon, Raid, Quest, Team Rocket Invasion, and Pokestop Lure notifications via direct message (DM) with predefined requirements.
25 stars 27 forks source link

Expand Geofence capabilities to allow non-Alarm geofences to be used for DMs and to support GeoJSON #87

Closed ArcanoxDragon closed 3 years ago

ArcanoxDragon commented 3 years ago

I am currently helping test out WhMgr on a Discord server I'm a part of as a potential replacement for another bot. While I was setting up a test instance of WhMgr, I was having issues getting any DMs sent even though I could tell that webhooks were being fired on the bot and some of them should have matched my subscriptions. I went through and debugged the WhMgr project so I could step through the notification processing code, and noticed that it was bailing because there were no Geofences loaded despite me having files in the geofences folder. It appears as though Geofences are only loaded if an Alarm references them in its geofence list. I would preferably like to use the DM feature of WhMgr right now without any actual alarms set up.

To me, it would make sense to iterate through the files in the geofences folder at the same time that the Alarms are loaded (in the constructor of WebhookController). It doesn't seem that the geofences for DM notifications are in any way related to Alarm geofences except that geofence files are only loaded if an Alarm mentions them.

I can submit a PR for this if this is an acceptable change to make.

versx commented 3 years ago

If you want to make a PR for this by all means, otherwise I can get this done. I realize now this isn't the most intended implementation if someone only wants to use the DM feature without alarms. I do see the issue.

ArcanoxDragon commented 3 years ago

I'd be happy to make one; I can submit it tomorrow. I do have one other fix for something I ran into (I'll file that separately) causing Discord rate limit issues tomorrow; I can file both of those early tomorrow sometime.

versx commented 3 years ago

Sounds good, I think with the new PR for this change we might want to change the geofences property in alarms to accept the actual geofence names instead of the geofence file names possibly? Interested in your thoughts on this.

ArcanoxDragon commented 3 years ago

That sounds like a good idea; I actually ran into that during configuration of the dummy alarm I created to load the files. I wasn't sure if the file extension was needed or not; I'd agree that it makes sense to have it load by name instead of filename.

versx commented 3 years ago

The only downside I see with changing from geofence file to geofence name is that I know others wanted me to change it to allow multiple geofences per file again instead of only one per file like it is now in order to line with WhMgr-UI.

Allowing multiple geofence per file is pretty beneficial for those that have multiple geofences per alarm instead of having to specify 30 different cities they just reference one file. Although they might have to create multiple geofence files depending how their setup is if they reference the same geofences in multiple alarms that are actually different geofence files. (if that makes sense)

Personally I like the ease of having one geofence file per alarm that has 20-30+ geofences but willing to make the change for specifying geofences individually.

This is the poll results on Discord so it's kind of a toss up tbh image

ArcanoxDragon commented 3 years ago

What about allowing both a list of .ini files with a single geofence each, or a single .json file with a GeoJSON FeatureCollection inside it? I'm encountering a new challenge now where we are using generated GeoJSON files to represent different cities, but they don't always convert well to a single .ini file because sometimes a city might be made up of multiple disconnected polygons (at least in data sources like OpenStreetMap).

versx commented 3 years ago

I think the majority of admins of WhMgr would appreciate that change tbh. I generally just filter by each city although I know recently some have expressed being able to use multiple suburbs within cities. If you're familiar with WDR I guess that's how he does it, Area with sub areas within it.