immich icon indicating copy to clipboard operation
immich copied to clipboard

feat(web): upload json config

Open truppelito opened this issue 1 year ago • 11 comments

Adds "Import from JSON" button as discussed here. My background is in embedded software, not web programming, so a review is very appreciated.

An open question is how to handle saving of the configuration. Currently the data imported from the JSON file is just put on the UI, but not saved. I believe this is counterintuitive and we should directly save after importing. In that case, it seems what we should do is call handleSave in admin-settings.svelte, but at that point my lack of knowledge about how Svelte works shows up and I'd appreciate some guidance.

truppelito avatar Apr 20 '24 12:04 truppelito

From what I see here, you are just reading the JSON file and have not uploaded the configuration to the server yet, correct?

alextran1502 avatar Apr 20 '24 14:04 alextran1502

Correct, currently it only automates setting the values on the web UI from the JSON file.

truppelito avatar Apr 20 '24 14:04 truppelito

Thanks!

I'd prefer if it'll automatically make a request to the server and update the config though.

I agree. I can either copy the code from handleSave (not very elegant) or call the function, but I couldn't figure that out (again, never used Svelte) in my limited time this morning. I'll try to have a look again later.

truppelito avatar Apr 20 '24 18:04 truppelito

Thanks!

I'd prefer if it'll automatically make a request to the server and update the config though.

I agree. I can either copy the code from handleSave (not very elegant) or call the function, but I couldn't figure that out (again, never used Svelte) in my limited time this morning. I'll try to have a look again later.

FWIW I don't think there's much you need to do. You should be able to just call the API function and wrap it in a try catch, there isn't much else to it since what you get is already in JSON. So I'd be fine with just having that API call here as well

danieldietzler avatar Apr 20 '24 19:04 danieldietzler

It seemed to me like handleSave was doing more than just calling the API (ex. showing a notification, etc), so I just figured out how to call it instead.

truppelito avatar Apr 20 '24 20:04 truppelito

that probably won't work. the config file might be a partial but the update method requires the entire thing

jrasm91 avatar Apr 20 '24 20:04 jrasm91

that probably won't work. the config file might be a partial but the update method requires the entire thing

I believe that's what this code in handleSave does:

const newConfig = await updateConfig({
  systemConfigDto: {
    ...savedConfig,
    ...update,
  },
});

truppelito avatar Apr 20 '24 20:04 truppelito

that's not a nested deep merge, that only overrides entire config sections

jrasm91 avatar Apr 20 '24 20:04 jrasm91

I'm fine if we just want to post the whole config for now though. At least export full config and import full config would work correctly. It is quite a bit more complicated to do anything else at the moment.

jrasm91 avatar Apr 20 '24 20:04 jrasm91

that only overrides entire config sections

Ah yes, that's a good point. Handling that requires changing that code in the handleSave function.

truppelito avatar Apr 20 '24 20:04 truppelito

When you need to use bind that usually shows bad data modelling

Right, I'm not familiar with Svelte best practices but it did seem like this functionality would be better outside of the child component. On the other hand a bind for the config variable is also how the copy to clipboard and the download JSON are implemented and I wasn't going for large changes.

truppelito avatar Apr 20 '24 20:04 truppelito

Is there anything else you'd like me to change on this PR?

truppelito avatar Apr 24 '24 19:04 truppelito

Is there anything else you'd like me to change on this PR?

No, I will do some functional testing and will get it merged

alextran1502 avatar Apr 24 '24 19:04 alextran1502