Feature/printer settings dialog improvements

[NotExpectedYet]

Printer Settings Class refactor.

Cleaned up a lot of code so far, still some things left to do.

Using this as the base to start the work on #532 #531

• Replace functions that load the settings modal.
• Refactor the code to load everything under it's own static function
• Add in try/catches and respond the error to the client if a problem arises
• Create a new octofarm client file and improve the functions with error responses in a single place
• Add in new api endpoint to trigger and update for a printers settings from octoprint
• Make settings panel trigger the update from OctoFarm for octoprint's latest settings.
• Replace global try/catch with multiple so 1 panel failing to load won't break the rest
• Put back in offline checks into panels
• Put back in offline message
• Add in check for port not available in drop down
• Add back in listener for save function
• Refactor the save function
• Fix save button event listener not applying
• Add kill button event listener
• Add test for endpoint
• Add changelog
• Add in port refresh
• Combine response into 1 alert.
• Add page refresh after save
• Clean up the extra calls on settings saving. Client refresh will make these redundant.

[davidzwa]

I see a lot of improvements in the printer settings page, really nice work dude! 🚀

The PR does not fix #532 as it is apparently a PrinterClean cache update issue as discussed offline, so my proposal is to remove that from the linked issues. The reason for this is because I have a more effective solution for it in mind for Printer Clean (for which it would be great if #406 is merged first, that would help me not having to pile a PR on top of another PR like file and job clean).

If you fix the event listener, remove the issue from linked issues I think this PR is good to go!

Dont forget:

• changelog
• Jest tests
• code annotations so I can understand client changes (lot of it is a mystery to me hahaha)

[NotExpectedYet]

I'll undo the mystery machine 🤣

[NotExpectedYet]

@davidzwa I've moved the refresh button to the side bar above save now. Was there some bugs you mentioned?

I fixed an issue that's happens when the port preference is null, it would incorrectly show the "Port not available" sign when there is no port preference. Found it because when you turn virtual printers on, the port preference is wiped on OctoPrint causing the refresh on OctoFarm to add a null in the port list. I don't see this been an issue in real world application, OctoFarm could not update the port preference if it detects a null from OctoPrint & OctoFarm has a saved value if it does become one. I've not noticed OctoPrint wiping it in any other situation, or just magically forgetting like was mentioned in the discord. The preference get's saved in config.yaml and I tested an OctoPrint restart and it persisted.

[davidzwa]

Are we ready for these errors to be thrown? DOM Javascript breaks and quits on the first errors it sees, so Im quite hesitant to see errors throw client side as we're also missing a global error handler here.

[NotExpectedYet]

I try catch everywhere I call that. And plan to do so for the rest I migrate over. I'd love a global error handler but it's not something I'd say was worth our time pending V2. Or, could be done in another PR as I do want to migrate the client over to this file but the codes a reet spaghetti junction aha.

[NotExpectedYet]

I gather I can move the try and catch inside this file and handle it there? We would have a global OctoFarm connection error handler then?

[NotExpectedYet]

Wouldn't need to repeat it around the rest of the files either. Let me know will update it.

[NotExpectedYet]

I always think of throw new Error, if caught to continue with the rest of the code. If this response is from the server then it's considered an error to me. Should this not be the case?

[davidzwa]

All Im saying is that I spotted new code with an explicit throw new Error(...) which if thrown could break the UI.
If you are certain that we try-catch everything, sure no prob. I dont oversee that. My advise is to be really careful, as we had issues with errors breaking the page in the past... and no testing to report to us 'watch out this will break here and here'

Oh note that code 503 and 400 are rare right now (400 is rarely thrown because we dont have input validation everywhere, its why I proposed using npm-input-validator as shown in octoprint-companion PR and my response on the error response below).
Codes 0, 500, 404 are more common and I dont see all of those covered.

Oh and why an else on 204 if it doesnt do anything but return?
Here Im responding to what you mentioned earlier on a 200 vs 204 case: If the body is empty and we didnt want it to be, that's a bug and it should be throw locally in the place where the bug is caused... so the runner or UI.
Should be a rare and this is best fixed by API tests tbh (I was able to get 100% fixed on the other apps I wrote just by simple dry-run API tests)

[NotExpectedYet]

There's an else on 204 because I use 204 as a confirmation response from the server where no return data is needed. The return the function to let the rest skip which is at the bottom converting anything else to a JSON object. There would be nothing to convert if it didn't return and stop the .JSON.

[davidzwa]

I dont think this statusMessage scales. Reason is that error response often is more than just a message. It could become a code, a context object, a message - more than just a string. So, I foresee a moment where we'd have to revert this in future and that's why it doesnt work for me.

throw "message"
throw new Error("error message")
throw new ValidationError(validationErrorInput)
throw new OctoPrintError(octoprintParseError)
... others

All of these dont fit in statusMessage string.

[davidzwa]

Oh btw, that does not stop this PR. It is just that I would not like to see this used globally due to reasons given above. That's all im sayin'

[NotExpectedYet]

I'm not using the status message for those eventualities there though? It's literally just saying "dodo, you need to provide a printer Id for this to work". Errors are caught below and didn't fire the status message? Can't remember. Anyway I think a global error handler should be tackled in another PR and update to use it. Would be good V2 prep work no?

[davidzwa]

If your intention is to return input validation result, I propose to use npm-input-validator and return that result in the body + 400 code. Much shorter code-wise, much clearer for another developer and much easier for the client to recognize and work with. This abstraction comes with so much benefit!
Here's how the input validation rules can look:

const checkInputRules = {
  persistenceUuid: "required",
  deviceUuid: "required",
  host: "string",
  port: "required|numeric",
  docker: "required|boolean",
  allowCrossOrigin: "required|boolean"
};

and this is the response if I leave out properties on purpose:

{
    "deviceUuid": {
        "message": "The device uuid field is mandatory.",
        "rule": "required"
    },
    "port": {
        "message": "The port field is mandatory.",
        "rule": "required"
    },
    "docker": {
        "message": "The docker field is mandatory.",
        "rule": "required"
    }
}

I had to do nothing for it (except for calling the validation and returning it if failed)! And because the rules are constant strings, we dont have to guess what is the reason for failure on the client side. Its explicit and reliable.

If input validation is what you're looking to abstract, I hope you like the method I propose above. This would make porting API code over to V2 an absolute breeze! 💨 💨💨

[davidzwa]

So to summarize

• This PR is epic
• Youre doing such nice work!
• I get to learn so much right now, this is so handy for thinking about stuff for V2
• Merge solved
• Fixed the tests
• I applied the list/getById change
• I applied prettier to some client files (only partially, more to be done)
• the error throwing in client octofarm.js code => risk analysis (maybe test with some test errors server-side, see how it looks?) I think this should be postponed for more breathing room as client side is a risk factor.
• After settings have been updated we see 3 messages (maybe a bit over the top)
  
• After saving the settings, in case we had the red portPreference problem and tried to correct for it, the red color does not disappear although the situation is fixed.
• Saving/refreshing is quite slow. Are there concessions possible to speed this up? Could it be that state.js is more work than necessary? (We can follow-up with a PR ofc)
• think about statusMessage, see my feedback above and think if this scales for the idea you have in mind in case we need to add error context (f.e. API validation errors or OctoPrint remote API errors). Please communicate what your plans are, my opinion is flexible but my code-smell nose is not 👍🏼

[NotExpectedYet]

So to summarize

• This PR is epic
• Youre doing such nice work!
• I get to learn so much right now, this is so handy for thinking about stuff for V2
• Merge solved
• Fixed the tests
• I applied the list/getById change
• I applied prettier to some client files (only partially, more to be done)
• the error throwing in client octofarm.js code => risk analysis (maybe test with some test errors server-side, see how it looks?) I think this should be postponed for more breathing room as client side is a risk factor.

It was tested but commented above, regarding moving the try/catch then we can deal with it in the file. I'm not depended on using throw new error, would just like to know alternatives and how better to deal with it.

• After settings have been updated we see 3 messages (maybe a bit over the top)
  

Those are because of the 3 calls the saving settings actually makes. It contacts OctoFarm, it contacts OctoPrint's API twice. I'll see about putting it all into a single notification and doing warning colour if one fails instead.

• After saving the settings, in case we had the red portPreference problem and tried to correct for it, the red color does not disappear although the situation is fixed.

Hmm that went on mine, will double check. Any reproduction? as I was using virtual printers for testing.

• Saving/refreshing is quite slow. Are there concessions possible to speed this up? Could it be that state.js is more work than necessary? (We can follow-up with a PR ofc)

Not from my experience here. Saving was rapid and so was refresh. Is this a windows vs linux thing again?

• think about statusMessage, see my feedback above and think if this scales for the idea you have in mind in case we need to add error context (f.e. API validation errors or OctoPrint remote API errors). Please communicate what your plans are, my opinion is flexible but my code-smell nose is not 👍🏼

Replied above.

[davidzwa]

• the error throwing in client octofarm.js code => risk analysis (maybe test with some test errors server-side, see how it looks?) I think this should be postponed for more breathing room as client side is a risk factor.

It was tested but commented above, regarding moving the try/catch then we can deal with it in the file. I'm not depended on using throw new error, would just like to know alternatives and how better to deal with it.

Responded above

Those are because of the 3 calls the saving settings actually makes. It contacts OctoFarm, it contacts OctoPrint's API twice. I'll see about putting it all into a single notification and doing warning colour if one fails instead.

That's nice

Hmm that went on mine, will double check. Any reproduction? as I was using virtual printers for testing.

I used my Pi4 + prusa mini to reproduce as this is a portPref/udev problem and virtual alone does not work for that - or I dont know how to. This seems to be best tested physically.

1. I turned on my printer (ttyACM0, not VIRTUAL)
2. connected to it in OP + saved
   2b) checked config: ttyACM0 saved
3. powered off the printer
4. I refreshed OctoPrint to check the 'usb serial not found blabla'
5. I checked OF, and went to the printer dialog (it was red: expected)
6. I rectified it in the dialog and pressed save.
7. Saw that the red color didnt dissappear. If Im correct I also pressed the refresh to make sure.
   7b) I checked OP + config: problem was gone there and serial was set back to VIRTUAL.
8. I reopened the dialog and the red color was gone, so most probably just UI logic to change the color missing or not working as expected.

So I think this will occur for your situation as well. Please try again.

• Saving/refreshing is quite slow. Are there concessions possible to speed this up? Could it be that state.js is more work than necessary? (We can follow-up with a PR ofc)
  Not from my experience here. Saving was rapid and so was refresh. Is this a windows vs linux thing again?

My prusa/Pi4 is running on the network, so I think this is just delay because of calling multiple OP API endpoints in a row synchronously. My question is can we optimize this? If I save printer settings, I dont want the UI to wait for regenerating cache
or calling to reload profiles, since we have SSE for that (and hopefully WS later). Look the reason Im hammering on this is because it seems that this is suboptimal and the core business of OF to be as snappy as possible.

This has nothing to do with Windows. That was not a problem. The reason for the 5 sec time was because SystemRunner just does a lot of work for

• think about statusMessage, see my feedback above and think if this scales for the idea you have in mind in case we need to add error context (f.e. API validation errors or OctoPrint remote API errors). Please communicate what your plans are, my opinion is flexible but my code-smell nose is not 👍🏼

Replied above.

I hope you like the npm-input-validator approach, now that I showed some examples for it. This can be followed up in another PR if you wish. This work is the biggest gap to fill to hop onto the V2 train btw, especially because I see how the cleaners and state work now. Well worth our time to discuss

[NotExpectedYet]

Ah, I didn't plan for that eventuality. I went to octoprint and resolved the issue then rescanned. I didn't sort in OctoFarm because I presumed they'd have to go there. The refresh cleared it for me though. I gather all that needs is a refresh of the page on save as that isn't done currently.

[NotExpectedYet]

@davidzwa Made the changes, and I've removed most of those calls from state. One of the checks isn't firing for reScaning the OctoPrint instance when you updated the URL that time but think that function needs cleaning up in another PR. The whole difference function checks need looking at tbh. Anyway, now saves. I decided not to put the try/catch inside the function for octofarm_client as I think the response should be handled higher up. I've also captured a screen recording of exactly how fast the older method of refresh/saving was for me so I don't think it's the multiple calls to OP that's causing the lagged response on your end unless I'm misunderstanding you, to me that would be reproducible here no?

Additionally I opened a ticket for the global error handler for OF calls, I can update the octofarm_client in that up coming PR for that. For now it handles the current situation fine to me.

[davidzwa]

Mark this in your memory that if there are problems with settings we can add any of it back. I dont think we will have to, but just note.

[davidzwa]

YAAAAS