Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(web): upload json config #8953

Merged
merged 5 commits into from
Apr 24, 2024

Conversation

truppelito
Copy link
Contributor

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.

@alextran1502 alextran1502 changed the title Upload json config feat(web): upload json config Apr 20, 2024
@alextran1502
Copy link
Contributor

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

@truppelito
Copy link
Contributor Author

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

Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me the code actually looks good, not sure why you were worried :D. I'd prefer if it'll automatically make a request to the server and update the config though.

@truppelito
Copy link
Contributor Author

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.

@danieldietzler
Copy link
Member

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

@truppelito
Copy link
Contributor Author

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.

@jrasm91
Copy link
Contributor

jrasm91 commented Apr 20, 2024

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

Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you need to use bind that usually shows bad data modelling, which is on us. I think for this PR it's fine, everything else would be out of scope.
Thank you! :)

@truppelito
Copy link
Contributor Author

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,
  },
});

@jrasm91
Copy link
Contributor

jrasm91 commented Apr 20, 2024

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

@jrasm91
Copy link
Contributor

jrasm91 commented Apr 20, 2024

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.

@truppelito
Copy link
Contributor Author

truppelito commented Apr 20, 2024

that only overrides entire config sections

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

@truppelito
Copy link
Contributor Author

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.

Comment on lines 65 to 71
fileSelector.type = 'file';
fileSelector.accept = '.json';
fileSelector.addEventListener('change', (e) => {
const file = (e.target as HTMLInputElement).files?.[0];
if (!file) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should just be a normal input element in the html stuff below. It can be hidden if you want though. But button click just needs to trigger clicking the input. It doesn't need to be dynamically created. You can use something like this:

let inputElement: HTMLInputElement;
const handleUpload = () => inputElement?.click();
..

<input bind:this={inputElement} ... />

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I based myself on this but I'm happy to make that change if you wish.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah but that is a pure TypeScript file without HTML, right? Unlike this page. Sorry, learning as I go!

@truppelito
Copy link
Contributor Author

truppelito commented Apr 24, 2024

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

@alextran1502
Copy link
Contributor

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 alextran1502 merged commit 0dbe44c into immich-app:main Apr 24, 2024
22 checks passed
@truppelito truppelito deleted the upload-json-config branch April 24, 2024 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants