Fix and Code Improvements#72
Conversation
- Added a tooltip to the "Enable additional WebUI modifications" option to explain what's being enabled or disabled, and to let users know that they will be logged out of the WebUI to restart the web server. - Fixed "parameter not set" error message when selecting to see the WAN Uptime on the CLI menu. - Added syslog message to show the add-on version number at startup. - Minor code improvements.
|
OK, here's the PR. Please review and verify whenever you get a chance. There's no hurry, so take your time. I'm signing off for the rest of the day. I'll check back in late in the evening. Have a good day, bud. |
|
Nice catch! Approved! |
|
Did I introduce that bug btw? Or was it long standing bug? I'm assuming long standing, but do point out the area I changed if I broke something please so I can reflect a bit 😭 I didn't notice any issues with the uptime tracker while testing on my version, infact it still is counting away fine on my node with the older code I submitted |
Sorry, I wasn't crystal clear. No, you didn't introduce the bug. Apparently, the bug was there before your changes, but it doesn't always show up. This morning, while testing your PR changes, I noticed the "WAN is down" message on the WebUI, and when I went to check the CLI menu option, I got the "parameter not set" error message. Upon investigating, I saw that the temporary WAN Uptime file (/tmp/wan0_uptime.tmp) was missing. It had been removed at some point; I don't even know when it happened. In any case, there was some code that was not properly initializing the variables used to read the WAN Uptime file, so I fixed that. It's rather unusual, so I don't think it's an urgent fix. So far, nobody has reported the problem. Then again, perhaps not many people check the WAN Uptime info on a regular basis. I think we're good if we wait a few days, unless you think we should make a release really soon. Either way would be fine with me. |
Ah! I see, that makes sense. And that makes me feel a bit better, I probably still introduced that bug (at some point) since I'm the one that worked on the uptime tracker. However, as long as it wasn't in my latest PR I feel a little better, mostly because in my mind none of the changes I recently submitted should of caused that. 😂 I make it a point to understand everything I'm working on and submitting as much as possible with the existing code, so the next time I work on the same add-on/project I have some groundwork knowledge.
I'm okay with waiting until the next release when the lonelycoder gives us the go-ahead, there is no rush to send anything out the door at this time. Or alternatively until someone reports it. As long as we know it's fixed in dev we can blast off a fix as soon as someone reports something. Thanks again for your second pair of eyes Martinski, your ability to track these things down and fix them is why the team work with you is superb 🙏You taught me lots with these projects, and your vast knowledge is still a shining light in a dark world 😉 I know I can occasionally make things interesting by throwing a curve ball at you, but you always seem to hit them out of the park! |
Yep, that's a good overall policy to have.
OK, sounds good. Based on the latest thelonelycoder's message, we should be able to make a release before this week is over.
Thanks for the vote of confidence. |
|
I actually found a bug with my implementation the new switch to enable/disable the scMerlin WebUi modifications. 
My best guess is my conditional check to see if values exist in MenuTree is not solid enough, maybe a firmware upgrade / reboot causes the MenuTree to be unpopulated at the time it runs and checks, causing the icon in the index_style.css file to not be loaded.
I quickly restored it by disbaling the webui Changes and then re-enabling, and the icon was restored, but something we should investigate when we have a moment before making this release. Just letting you know |
|
Just confirming that a regular reboot now also recreated the issue. Which is extremely odd considering I was sure I tested a reboot on both my routers 
|
|
So my hunch was a bit off. Turns out it was a order of operations issue. |
Great catch!! |
It looks like the bug shows up when you have only scMerlin installed. If you happen to have another add-on installed before scMerlin (e.g. ntpMerlin), which creates a page tab in the "Addons" menu, the bug does not show up because the "other" add-on script also adds the "Addons" icon during the startup sequence before scMerlin checks for it. That's why in this scenario, everything seemed to work fine when testing with a reboot. |
I was always told, as long as you can cleanup your own mess, it's not a big deal. |
That's exactly what I observed as well!!! :) It makes complete sense to me, because in my first round of testing I still had spdMerlin installed. That explains why the first time the bug was hidden, and the second time I noticed it! 120% accurate @Martinski4GitHub great catch!! |
👍🤜🤛 Oftentimes, we learn more when things go wrong than when things go right... 😉 |
Yep, the same thing happened to me when I tested the PR changes. I had ntpMerlin and connmon already installed from a previous testing cycle, so everything worked, LOL...
|
2 participants
Added a tooltip to the "Enable additional WebUI modifications" option to let users know what's being enabled or disabled, and to inform them that they will be logged out of the WebUI to restart the web server.
Fixed "parameter not set" error message when selecting to see the WAN Uptime on the CLI menu.
Added syslog message to show the add-on version number at startup.
Minor code improvements.
Sample screenshot of the tooltip [Updated with correct spelling]:
Screenshot of the CLI error message from WAN Uptime:
Screenshot of the WebUI page showing "WAN is down" message even though WAN is actually up and active: