Convert to Flask App

[robputt]

No description provided.

[marvinmarnold]

I left a lot of comments but this is a big improvement over what's there. I know many of my comments address pre-existing issues. We don't need to address all of the things but I think we can improve readability a bit. At least a few tests should also be added.

[marvinmarnold]

Looking real good with all those changes. I'd still like to see some tests added as part of this work. If you don't think you've added any net new logic that needs to be tested, then could you add a handful of tests for pre-existing logic to establish a pattern for how Flask tests should be done?

Could you also add some documentation? Should include how to run tests. Also if there is a recommended way to run the site locally during development.

[shawaj]

Looks good to me @robputt - tested it on a couple of test devices and works nicely.

Few things I noticed, but pretty minor and can probably be fixed later I guess:

• on the backend logs it was complaining about not having a timezone set and saying it was a development server and not to use in production
• some of the font downloads were 404ing according to logs (nunito-regular.woff and nunito-bold.woff)
• seems to download the font every time the page re-loads automatically and therefore isn't instantly applied. Could we just cache it locally instead or bundle the font or something?
• when the region has been set with a REGION_OVERRIDE env variable instead of directly via the miner it gives a "region plan" of UN123 instead of what's in the region override. Not sure if this makes sense or not. For production we don't use an override but in testing of packet forwarder and light hotspots we sometimes do (especially with non location asserted devices) see here in packet forwarder. Perhaps if there is a region override set, we should mention that in the diagnostics like EU868 (Set by override) and could do similar when set by miner perhaps.
• in the original diag, instead of showing UN123 directly it showed "awaiting location assertion"
• LTE detected shows even for non-outdoor units. I'm wondering whether we should add a field to the hardware variants of "LTE Support" or something to that effect? So that we can automatically determine whether it should even bother to check for LTE?
• on a side note, perhaps cellular detected or modem detected is better than LTE?
• could add a favicon perhaps?

Going to mark as approved anyway as I guess all this can be resolved in a later update.

[shawaj]

Some screenshots of it running on a RAK based light hotspot I tested on. Looks smart!

[robputt]

Looks good to me @robputt - tested it on a couple of test devices and works nicely.

Few things I noticed, but pretty minor and can probably be fixed later I guess:

• on the backend logs it was complaining about not having a timezone set and saying it was a development server and not to use in production
• some of the font downloads were 404ing according to logs (nunito-regular.woff and nunito-bold.woff)
• seems to download the font every time the page re-loads automatically and therefore isn't instantly applied. Could we just cache it locally instead or bundle the font or something?
• when the region has been set with a REGION_OVERRIDE env variable instead of directly via the miner it gives a "region plan" of UN123 instead of what's in the region override. Not sure if this makes sense or not. For production we don't use an override but in testing of packet forwarder and light hotspots we sometimes do (especially with non location asserted devices) see here in packet forwarder. Perhaps if there is a region override set, we should mention that in the diagnostics like EU868 (Set by override) and could do similar when set by miner perhaps.
• in the original diag, instead of showing UN123 directly it showed "awaiting location assertion"
• LTE detected shows even for non-outdoor units. I'm wondering whether we should add a field to the hardware variants of "LTE Support" or something to that effect? So that we can automatically determine whether it should even bother to check for LTE?
• on a side note, perhaps cellular detected or modem detected is better than LTE?
• could add a favicon perhaps?

Going to mark as approved anyway as I guess all this can be resolved in a later update.

@shawaj

• QUESTION - The TZ issue is a Dockerism as Docker containers do not have a time zone set usually and hence default to UTC. I personally like UTC for machines as it makes them easily comparable if they are all in 1 universal time zone, however if you prefer to set a time zone please let me know which TZ and we'll create an issue for future enhancement for TZ support. The current deployed version also defaults to UTC it just doesn't scream about it.
• QUESTION - Using development server - currently the container just runs the Werkzeug Python HTTP server as I didn't feel the need to bloat with NGINX / Apache, however as per the warning it is not a production ready server, mostly in terms of scaling although this should be a moot point as only a few requests are every going to hit these endpoints and they should be internal to the customers network. Although if you prefer to use WSGI and use a "real" web server I can edit the docker accordingly.
• NEW ISSUE - Add caching headers for static files to prevent re-download each page refresh - see Caching for Static Files #102
• NEW ISSUE - Favicon - see Add Favicon #100
• FIXED - LTE detected now says "Modem Detected" in the template as requested
• NEW ISSUE - LTE showing on both indoor and outdoor variants - see Only show LTE for LTE enabled variants. #101 (this is a regression from the previous version although I believe it should be handled separately as this has now been added to the variants file and will require some rework.
• FIXED - UN123 should show "Awaiting Location Assertion"

Most comments fixed / deferred to new issues.