erohtar / Dasherr

A minimal and lightweight dashboard for your self-hosted services (and bookmarks)
GNU General Public License v3.0
190 stars 8 forks source link

Settings reset button, prevent editing non-settings JSON files #20

Closed iiPythonx closed 10 months ago

iiPythonx commented 1 year ago

Edit list:

If this PR causes any problems, or if I added something that you don't want to merge, let me know :)

erohtar commented 1 year ago

Hey @iiPythonx - really sorry for not getting back to you sooner. Work has been really busy so personal projects have taken a backseat.. Anyway, I got a chance to try out the changes and here's my feedback:

Thank you!

iiPythonx commented 1 year ago

@erohtar Thanks for getting back to me on it! About the JSON file loading change, it doesn't affect loading in any way, so assuming you still had the proper configuration files, they should load fine. The original save JS had settings.json hardcoded as well, but I can double check.

Reset file wise, a confirmation would be great. I could implement a basic alert() based one if that works for you, otherwise I'll leave it up to you.

EDIT: Basic confirmation added in b7928c7. And according to this line, the save action always overwrote settings.json, so my original commit shouldn't change anything.

erohtar commented 1 year ago

And according to this line, the save action always overwrote settings.json, so my original commit shouldn't change anything.

Yes! I was testing your updates and when I saw it always overwrote main config, I went back to check and found it was a bug in my original code. So I fixed it locally and came here to mention it to you and found your edit haha Here's my simple fix to that line:

data.append('file' , settingsFile? '../' + settingsFile : '../settings.json');

I love that confirmation msg, and you're right, that basic alert is all that was needed. Btw the above mess points out a residual problem - if a user is editing a non-main settings file, and uses the reset config feature, the main config gets reset. That's not good. Any ideas? I was thinking maybe hiding the reset button on non-main config editor or showing a simple alert that says this feature is only for settings.json resets.

iiPythonx commented 1 year ago

I went back to check and found it was a bug in my original code.

Good to know! I thought it might of been intentional but wasn't sure.

62f2e72 readds support for multiple settings.json files, and also allows them to be reset using the same reset button (with confirmation of course).

Also, I've noticed the codebase used quite a few settingsFile ? "../" + settingsFile : "../settings.json" statements. I have removed them and instead made the settingsFile variable default to settings.json. In my head, cleaner codebase means less work to maintain and easier to read through.

The dashboard still works fine, and the "non-settings JSON files" part of this PR still works (not allowing special characters or any filename not ending in .json). Let me know what you think of the commit, and if I should change anything else!

(the force push was me not realizing i pushed a change to settings.sh.json by accident, but it's reverted now)

iiPythonx commented 10 months ago

Closed due to inactivity, the commits should still be visible lass somebody want to implement any similar changes.