Skip to content

WebGUI Cron Schedule Input#396

Merged
ExtremeFiretop merged 5 commits intoExtremeFiretop:WebFunfrom
Martinski4GitHub:WebFun
Jan 27, 2025
Merged

WebGUI Cron Schedule Input#396
ExtremeFiretop merged 5 commits intoExtremeFiretop:WebFunfrom
Martinski4GitHub:WebFun

Conversation

@Martinski4GitHub
Copy link
Collaborator

@Martinski4GitHub Martinski4GitHub commented Jan 26, 2025

Implemented the "Schedule for F/W Update Checks" user input on the WebGUI page. Currently, the design is meant to be simple and intuitive to allow users to set up the most common cron schedules with standard parameters that should satisfy the requirements of the vast majority of users (~95% perhaps?)

If a user wants to have a more complex cron schedule (e.g. with a specific list of days of month) then the recommendation would be to use the SSH CLI menus. In such cases, when the WebGUI page cannot display the cron schedule parameters, some of the page elements would be either shown as empty or grayed out to indicate that those parameters are not represented on the WebGUI.

MerlinAU_WebGUI_CronSchedule#1

MerlinAU_WebGUI_CronSchedule#2

MerlinAU_WebGUI_CronSchedule#3

Implemented the "Schedule for F/W Update Checks" user input on the WebGUI page.
Currently, the design is meant be simple and intuitive to allow users to set up the most common cron schedules with standard parameters that should satisfy the requirements of  the vast majority of users (~95% perhaps?)

If a user wants to have more complex ccron schedule (e.g. with a specific list of days of month) then the recommendation would be to use the CLI SSH menu option. In such cases, when the WebGUI page cannot display some cron schedule parameters, some of the page elements would be either shown as empty or grayed out to indicate that those parameters are not represented on the WebGUI.
@Martinski4GitHub
Copy link
Collaborator Author

Martinski4GitHub commented Jan 26, 2025

@ExtremeFiretop,

Please review and take a look at this cron schedule implementation when you get a chance. The goal was not to handle any and all possible cron schedule parameter combinations. It was designed with simplicity and ease of use in mind so that regular users can set up the most common cron schedules that would satisfy their requirements. For anything more complex, users will need to use the SSH CLI menu options.

FYI,
We had a power outage for almost 2 hours, but the power came back about 10 minutes ago, so I decided to submit the PR while I could. The strong winds are really picking up around here and I heard a nearby power transformer on a pole making a very weird noise before the blackout.

@TheS1R
Copy link

TheS1R commented Jan 26, 2025

@ExtremeFiretop,

Please review and take a look at this cron schedule implementation when you get a chance. The goal was not to handle any and all possible cron schedule parameter combinations. It was designed with simplicity and ease of use in mind so that regular users can set up the most common cron schedules that would satisfy their requirements. For anything more complex, users will need to use the SSH CLI menu options.

FYI, We had a power outage for almost 2 hours, but the power came back about 10 minutes ago, so I decided to submit the PR while I could. The strong winds are really picking up around here and I heard a nearby power transformer on a pole making a very weird noise before the blackout.

@Martinski4GitHub

You hit a homerun with this implementation! Everything works exactly as expected, including modifying the Enable Auto-Updates for MerlinAU check to fire 15 minutes prior if enabled (which still occurs on a daily basis regardless of the days selected for Schedule for F/W Update Checks).

Fixed "undefined" error when "TBD" is found.
Code improvements to address the "Warnings" from the latest Linter tool report.
Fine-tuning code.
@ExtremeFiretop
Copy link
Owner

ExtremeFiretop commented Jan 27, 2025

@Martinski4GitHub

Thank you for advising of the latest commits, will do my tests on the latest copy!

Fixed problem in WebGUI page when a comment line is introduced in the configuration file.
@Martinski4GitHub
Copy link
Collaborator Author

@ExtremeFiretop,

Sunday evening, my co-worker friend tested the latest WebGUI version and, so far, has found no issues with the "cron schedule" or the "Keep configuration file" and "Bypass postponed days" implementations. He said he's very pleased with using the WebGUI page which enhances the user-friendliness, usability, and accessibility of the MerlinAU tool, especially to those users who are not really accustomed to the SSH CLI environment.

He did find one unusual problem which he admits is unlikely to happen "in the wild" with "real-world" regular users, but we should be aware of it. During his testing, he made copies of his current configuration file since he wanted to validate different parameter values while keeping his "good working" version safely tucked away. In one regression test, he added a comment line to the top of the config file to indicate the parameters he was testing, and when he reloaded the WebGUI, many values were "missing" or the wrong values were displayed; but as soon as he removed the comment line, everything worked as it was before. This behavior is repeatable and consistent which means the current JavaScript code is not handling comment lines at all.

Having a suspicion of where the root cause was, I took a look and found the issue: the 'GetConfigSettings()' function calls the 'Tokenize()' function which converts the key-value configuration pairs into an array of tokens but does not take into account the separate, individual lines where they come from so, as a result, all the contents of the comment line were tokenized as well causing issues because the key-value pairs were "out of sync."

Anyway, I have fixed this problem in my latest commit. Even though the scenario is unlikely to happen "in the wild," we really should not be choking on a simple comment line.

@ExtremeFiretop
Copy link
Owner

  1. Confirmed the cron schedule validation works as expected
  2. Confirmed the cron schedule output into the shared config works as expected (you even added your new key to my old fix! Nice catch)
  3. Confirmed you resolved the reported issue with the configuration file being TBD
  4. Confirmed you resolved the reported issue by your friend with the line comment

Approved and merging!

@ExtremeFiretop
Copy link
Owner

ExtremeFiretop commented Jan 27, 2025

@ExtremeFiretop,

He did find one unusual problem which he admits is unlikely to happen "in the wild" with "real-world" regular users, but we should be aware of it. During his testing, he made copies of his current configuration file since he wanted to validate different parameter values while keeping his "good working" version safely tucked away. In one regression test, he added a comment line to the top of the config file to indicate the parameters he was testing, and when he reloaded the WebGUI, many values were "missing" or the wrong values were displayed; but as soon as he removed the comment line, everything worked as it was before. This behavior is repeatable and consistent which means the current JavaScript code is not handling comment lines at all.

Having a suspicion of where the root cause was, I took a look and found the issue: the 'GetConfigSettings()' function calls the 'Tokenize()' function which converts the key-value configuration pairs into an array of tokens but does not take into account the separate, individual lines where they come from so, as a result, all the contents of the comment line were tokenized as well causing issues because the key-value pairs were "out of sync."

Anyway, I have fixed this problem in my latest commit. Even though the scenario is unlikely to happen "in the wild," we really should not be choking on a simple comment line.

Tell your friend, a beer on me! Always appreciate the extra robustness, no matter how "unlikely" something is, if there's a way to account for it; why not fix it!

@ExtremeFiretop ExtremeFiretop merged commit 0562391 into ExtremeFiretop:WebFun Jan 27, 2025
1 check passed
@ExtremeFiretop
Copy link
Owner

ExtremeFiretop commented Jan 27, 2025

Evolution over time:

  1. First private version (private to me learning/developing)
    image

  2. Last private version (private to me before involving the master @Martinski4GitHub)
    image

  3. Final version of the WebUI once @Martinski4GitHub has added his magic:
    image

Man does it look good now!

@ExtremeFiretop
Copy link
Owner

ExtremeFiretop commented Jan 27, 2025

Always appreciate the extra robustness, no matter how "unlikely" something is, if there's a way to account for it; why not fix it!

@Martinski4GitHub

On the same wave length as this last comment above, I noticed a bug in the latest code, which is extremely unlikely to be caught by a normal user in the "wild", but the ideas is if uninstalling MerlinAU while keeping the configuration, and then reset AMTM, it loses the saved email configuration, and then if I reinstall MerlinAU and restore the saved configuration file, the WebUI now believes the email is configured in when it really isn't as seen below:

AMTM Email options missing:
image

MerlinAU shell script correctly identifies that email is not configured:
image

WebUI still thinks email is configured:
image
image

Now; I did code logic to account for this in the WebUI, and i can still see it exists, so I went digging.
And in PR: #384

You removed the following code:

if _CheckEMailConfigFileFromAMTM_ 0
then
    if [ "$sendEMailNotificationsFlag" = "TBD" ]
    then
        Update_Custom_Settings FW_New_Update_EMail_Notification "$FW_UpdateEMailNotificationDefault"
    fi
else
    if [ "$sendEMailNotificationsFlag" != "TBD" ]
    then
        Delete_Custom_Settings "FW_New_Update_EMail_Notification"
    fi
fi

Which was made to handle this situation, now while I understand it may not be the best implementation, I do believe a check similar to this would be of value. Did you have any ideas in mind to implement a check such as this again?

If I add this code back, now the WebUI works as designed for email as found below:
image

Again, this is extremely unlikely to be found in the wild, but if we can account and think for these types of situations, I say we fix them.

Also noticed my WebUI code used to gray out the secondary email and email format options when this check was triggered, but now it doesn't anymore?
image

Previously, my code did this:
image

In the same PR identified above, I guess I missed this code being removed which handled that:

            if (emailNotificationsEnabled && emailFormat && secondaryEmail) {
                // Check if 'FW_New_Update_EMail_Notification' is present in custom_settings
                if (custom_settings.hasOwnProperty('FW_New_Update_EMail_Notification')) {
                    // If the setting exists, enable the checkbox and controls
                    emailNotificationsEnabled.disabled = false;
                    emailNotificationsEnabled.checked = (custom_settings.FW_New_Update_EMail_Notification === 'ENABLED');
                    emailNotificationsEnabled.style.opacity = '1';
                    emailFormat.disabled = false;
                    emailFormat.style.opacity = '1';
                    secondaryEmail.disabled = false;
                    secondaryEmail.style.opacity = '1';
                } else {
                    // If the setting is missing, disable and gray out the checkbox, dropdown, and email input
                    emailNotificationsEnabled.disabled = true;
                    emailNotificationsEnabled.checked = false;
                    emailNotificationsEnabled.style.opacity = '0.5';
                    emailFormat.disabled = true;
                    emailFormat.style.opacity = '0.5';
                    secondaryEmail.disabled = true;
                    secondaryEmail.style.opacity = '0.5';
                }

@ExtremeFiretop
Copy link
Owner

While testing the above, I realized why you didn't keep the check I added, because it adds a dependency to the settings file before it can be created on new installs.

image

But we can surely just change the ordering in this regard I would think?
Like I mentioned in the last comment I realize it wasn't the best implementation, but I guess I thought something else would replace the above at some point :P

@Martinski4GitHub
Copy link
Collaborator Author

@ExtremeFiretop,
He did find one unusual problem which he admits is unlikely to happen "in the wild" with "real-world" regular users, but we should be aware of it. During his testing, he made copies of his current configuration file since he wanted to validate different parameter values while keeping his "good working" version safely tucked away. In one regression test, he added a comment line to the top of the config file to indicate the parameters he was testing, and when he reloaded the WebGUI, many values were "missing" or the wrong values were displayed; but as soon as he removed the comment line, everything worked as it was before. This behavior is repeatable and consistent which means the current JavaScript code is not handling comment lines at all.
Having a suspicion of where the root cause was, I took a look and found the issue: the 'GetConfigSettings()' function calls the 'Tokenize()' function which converts the key-value configuration pairs into an array of tokens but does not take into account the separate, individual lines where they come from so, as a result, all the contents of the comment line were tokenized as well causing issues because the key-value pairs were "out of sync."
Anyway, I have fixed this problem in my latest commit. Even though the scenario is unlikely to happen "in the wild," we really should not be choking on a simple comment line.

Tell your friend, a beer on me! Always appreciate the extra robustness, no matter how "unlikely" something is, if there's a way to account for it; why not fix it!

I'll make sure to pass along the message. We'll go out to lunch sometime this week and I'll buy him his favorite drink!! :>).

@ExtremeFiretop
Copy link
Owner

@ExtremeFiretop,
He did find one unusual problem which he admits is unlikely to happen "in the wild" with "real-world" regular users, but we should be aware of it. During his testing, he made copies of his current configuration file since he wanted to validate different parameter values while keeping his "good working" version safely tucked away. In one regression test, he added a comment line to the top of the config file to indicate the parameters he was testing, and when he reloaded the WebGUI, many values were "missing" or the wrong values were displayed; but as soon as he removed the comment line, everything worked as it was before. This behavior is repeatable and consistent which means the current JavaScript code is not handling comment lines at all.
Having a suspicion of where the root cause was, I took a look and found the issue: the 'GetConfigSettings()' function calls the 'Tokenize()' function which converts the key-value configuration pairs into an array of tokens but does not take into account the separate, individual lines where they come from so, as a result, all the contents of the comment line were tokenized as well causing issues because the key-value pairs were "out of sync."
Anyway, I have fixed this problem in my latest commit. Even though the scenario is unlikely to happen "in the wild," we really should not be choking on a simple comment line.

Tell your friend, a beer on me! Always appreciate the extra robustness, no matter how "unlikely" something is, if there's a way to account for it; why not fix it!

I'll make sure to pass along the message. We'll go out to lunch sometime this week and I'll buy him his favorite drink!! :>).

Then I'll ship you a Canada dry in return! Hahah🤣

Just heading off to bed, will check on the project tomorrow! Have a goodnight buddy!

@Martinski4GitHub
Copy link
Collaborator Author

Always appreciate the extra robustness, no matter how "unlikely" something is, if there's a way to account for it; why not fix it!

@Martinski4GitHub

On the same wave length as this last comment above, I noticed a bug in the latest code, which is extremely unlikely to be caught by a normal user in the "wild", but the ideas is if uninstalling MerlinAU while keeping the configuration, and then reset AMTM, it loses the saved email configuration, and then if I reinstall MerlinAU and restore the saved configuration file, the WebUI now believes the email is configured in when it really isn't as seen below:

AMTM Email options missing: image

MerlinAU shell script correctly identifies that email is not configured: image

WebUI still thinks email is configured: image image

Very good catch!! It is certainly a highly unusual scenario, but it should be handled much better nevertheless.

Now; I did code logic to account for this in the WebUI, and i can still see it exists, so I went digging. And in PR: #384

You removed the following code:

if _CheckEMailConfigFileFromAMTM_ 0
then
    if [ "$sendEMailNotificationsFlag" = "TBD" ]
    then
        Update_Custom_Settings FW_New_Update_EMail_Notification "$FW_UpdateEMailNotificationDefault"
    fi
else
    if [ "$sendEMailNotificationsFlag" != "TBD" ]
    then
        Delete_Custom_Settings "FW_New_Update_EMail_Notification"
    fi
fi

Which was made to handle this situation, now while I understand it may not be the best implementation, I do believe a check similar to this would be of value. Did you have any ideas in mind to implement a check such as this again?

Yeah, I do recall removing that code because deleting the entry from the configuration file didn't make sense. I thought about a different way to handle it and made a mental note about it; but after all the flurry of changes, additions, and reformatting following the initial reviews, it completely slipped my mind (that's why I usually like to make a written note somewhere in my digital journal).

Anyway, I know a better way to handle such a scenario. Give me about 35-40 minutes to implement it.

If I add this code back, now the WebUI works as designed for email as found below: image

Again, this is extremely unlikely to be found in the wild, but if we can account and think for these types of situations, I say we fix them.

Agreed!!!!

Also noticed my WebUI code used to gray out the secondary email and email format options when this check was triggered, but now it doesn't anymore? image

Previously, my code did this: image

In the same PR identified above, I guess I missed this code being removed which handled that:

            if (emailNotificationsEnabled && emailFormat && secondaryEmail) {
                // Check if 'FW_New_Update_EMail_Notification' is present in custom_settings
                if (custom_settings.hasOwnProperty('FW_New_Update_EMail_Notification')) {
                    // If the setting exists, enable the checkbox and controls
                    emailNotificationsEnabled.disabled = false;
                    emailNotificationsEnabled.checked = (custom_settings.FW_New_Update_EMail_Notification === 'ENABLED');
                    emailNotificationsEnabled.style.opacity = '1';
                    emailFormat.disabled = false;
                    emailFormat.style.opacity = '1';
                    secondaryEmail.disabled = false;
                    secondaryEmail.style.opacity = '1';
                } else {
                    // If the setting is missing, disable and gray out the checkbox, dropdown, and email input
                    emailNotificationsEnabled.disabled = true;
                    emailNotificationsEnabled.checked = false;
                    emailNotificationsEnabled.style.opacity = '0.5';
                    emailFormat.disabled = true;
                    emailFormat.style.opacity = '0.5';
                    secondaryEmail.disabled = true;
                    secondaryEmail.style.opacity = '0.5';
                }

Nope, that's incorrect. I never removed that code. Are you absolutely sure that you're loading and looking at the latest code from the GitHub repository?

Here's the section of the code (lines #1081 to #1106) from the current file in GitHub:

MerlinAU_WebGUI_ASP_Code

Make sure to load and review the latest code on GitHub!!

@Martinski4GitHub
Copy link
Collaborator Author

While testing the above, I realized why you didn't keep the check I added, because it adds a dependency to the settings file before it can be created on new installs.

image

But we can surely just change the ordering in this regard I would think? Like I mentioned in the last comment I realize it wasn't the best implementation, but I guess I thought something else would replace the above at some point

Yep, working on it now...

@Martinski4GitHub
Copy link
Collaborator Author

@ExtremeFiretop,
He did find one unusual problem which he admits is unlikely to happen "in the wild" with "real-world" regular users, but we should be aware of it. During his testing, he made copies of his current configuration file since he wanted to validate different parameter values while keeping his "good working" version safely tucked away. In one regression test, he added a comment line to the top of the config file to indicate the parameters he was testing, and when he reloaded the WebGUI, many values were "missing" or the wrong values were displayed; but as soon as he removed the comment line, everything worked as it was before. This behavior is repeatable and consistent which means the current JavaScript code is not handling comment lines at all.
Having a suspicion of where the root cause was, I took a look and found the issue: the 'GetConfigSettings()' function calls the 'Tokenize()' function which converts the key-value configuration pairs into an array of tokens but does not take into account the separate, individual lines where they come from so, as a result, all the contents of the comment line were tokenized as well causing issues because the key-value pairs were "out of sync."
Anyway, I have fixed this problem in my latest commit. Even though the scenario is unlikely to happen "in the wild," we really should not be choking on a simple comment line.

Tell your friend, a beer on me! Always appreciate the extra robustness, no matter how "unlikely" something is, if there's a way to account for it; why not fix it!

I'll make sure to pass along the message. We'll go out to lunch sometime this week and I'll buy him his favorite drink!! :>).

Then I'll ship you a Canada dry in return! Hahah🤣

Just heading off to bed, will check on the project tomorrow! Have a goodnight buddy!

Have a good night, bud. Sleep tight!! :>).
By the time you get up tomorrow morning, it will be sorted out and working even better!!!

@ExtremeFiretop
Copy link
Owner

ExtremeFiretop commented Jan 28, 2025

Nope, that's incorrect. I never removed that code. Are you absolutely sure that you're loading and looking at the latest code from the GitHub repository?

Absolutely ;) Without a doubt.

Here's the section of the code (lines #1081 to #1106) from the current file in GitHub:

MerlinAU_WebGUI_ASP_Code

Make sure to load and review the latest code on GitHub!!

For sure always do :) Only correction to the sentence I'll make is change the word "removed" to "moved" hehehe
As I mentioned, I did the review through the PR and it looked removed when I searched for this code:

image

But infact I just had to scroll down further.

image

As I touched on in my comment, that assessment was only based on the PR comparison, didn't even open the file before making the comment. I always use the latest

@ExtremeFiretop
Copy link
Owner

ExtremeFiretop commented Jan 28, 2025

if we want to be 100% correct, I just did a search and out of 381 PRs, only once did I find a spot where I for sure didn't grab the latest copy.

But in fact it makes sense why I said:

In the same PR identified above, I guess I missed this code being removed which handled that:

Because in fact, it was not removed, it was simply "moved".
But I think it's safe to say that grabbing the latest copy is not a common issue around here hehe! Something else must of broken it, if it wasn't removed

@ExtremeFiretop
Copy link
Owner

ExtremeFiretop commented Jan 28, 2025

Yeah, I do recall removing that code because deleting the entry from the configuration file didn't make sense. I thought about a different way to handle it and made a mental note about it; but after all the flurry of changes, additions, and reformatting following the initial reviews, it completely slipped my mind (that's why I usually like to make a written note somewhere in my digital journal).

I hear you, I often try to take notes while testing or making changes as well, no worries on it slipping your mind it's lots of changes like you said. Stuff is bound to slip our minds. In this case I saw you removed the first chunk and just assumed you removed the other chunk when it didn't line up in the PR comparison.

Infact it was just moved, but something still wasn't working right, I can do a deep dive and troubleshoot if it's still a problem after your latest PR. But I have a feeling that won't be the case with your skills 😉

The first comment was literally just me opening the PR; and doing a quick analysis of what I could see in the comparison. Now that I'm at my desk with the code open, it's a lot easier to do a search and find ;)

Anyway, I know a better way to handle such a scenario. Give me about 35-40 minutes to implement it.

Takes you zero time my friend, zero time!

@Martinski4GitHub
Copy link
Collaborator Author

Nope, that's incorrect. I never removed that code. Are you absolutely sure that you're loading and looking at the latest code from the GitHub repository?

Absolutely ;) Without a doubt.

I had to ask to be absolutely sure because I could not think of any other reason why you could not find the code you were looking for in the latest ASP file on GitHub.

Here's the section of the code (lines #1081 to #1106) from the current file in GitHub:
...

Make sure to load and review the latest code on GitHub!!

For sure always do :) Only correction to the sentence I'll make is change the word "removed" to "moved" hehehe As I mentioned, I did the review through the PR and it looked removed when I searched for this code:
...

But infact I just had to scroll down further.
...

As I touched on in my comment, that assessment was only based on the PR comparison, didn't even open the file before making the comment. I always use the latest.

Ah, OK. I believe I understand how you went about looking for the section of code. However, it looks like you didn't do the most crucial & final step: search for the code in the latest version of the source file. This step would have given you conclusive results; without this final, crucial step your analysis was incomplete and, therefore, flawed.

My piece of advice: when you're trying to find a specific piece of code, always search for it first in the latest version of the source file. If not found, only then you can start doing diffs & comparisons between commits/PRs. As s/w devs, we do this so often and enough times that this simple but foolproof method always gives the best results.

My 2 cents.

@ExtremeFiretop
Copy link
Owner

Nope, that's incorrect. I never removed that code. Are you absolutely sure that you're loading and looking at the latest code from the GitHub repository?

Absolutely ;) Without a doubt.

I had to ask to be absolutely sure because I could not think of any other reason why you could not find the code you were looking for in the latest ASP file on GitHub.

Here's the section of the code (lines #1081 to #1106) from the current file in GitHub:
...
Make sure to load and review the latest code on GitHub!!

For sure always do :) Only correction to the sentence I'll make is change the word "removed" to "moved" hehehe As I mentioned, I did the review through the PR and it looked removed when I searched for this code:
...
But infact I just had to scroll down further.
...
As I touched on in my comment, that assessment was only based on the PR comparison, didn't even open the file before making the comment. I always use the latest.

Ah, OK. I believe I understand how you went about looking for the section of code. However, it looks like you didn't do the most crucial & final step: search for the code in the latest version of the source file. This step would have given you conclusive results; without this final, crucial step your analysis was incomplete and, therefore, flawed.

My piece of advice: when you're trying to find a specific piece of code, always search for it first in the latest version of the source file. If not found, only then you can start doing diffs & comparisons between commits/PRs. As s/w devs, we do this so often and enough times that this simple but foolproof method always gives the best results.

My 2 cents.

Your absolutely right, it was my bad for not searching the entire file first before making my comment with an assessment.

I usually would. And it usually is full proof. I've been getting lazy recently is my only explanation 😞 Hahahaha 😂 but I for sure had the latest files!

The positive is you found the issue and fixed it in your latest PR, I'm happy to know that no matter the little mixup, we got to the same end goal of making that code more robust which was really my only concern when I brought this up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants