extend modal dialog for uploading from server

[evoludolab]

The modal dialog for uploading media from the server includes a checkbox for deleting originals while, other relevant settings remain hidden in the advanced settings.

This pull request adds support for enabling/disabling import_via_symlinks, skip_duplicates and resync_metadata while preventing invalid combinations (e.g. import_via_symlinks and delete_imported both enabled). The settings are used as defaults, except for resync_metadata which was only available through the CLI lychee:sync command but makes sense to include here as well (at least provided that skip_duplicates is enabled).

Localizations of the additional text on the modal panel missing (English only).

The corresponding changes to the back end to process the settings on the modal panel are in LycheeOrg/Lychee#894.

[ildyria]

We usually don't care about const duplication. :)

[evoludolab]

The latest commit on the front and back ends address code smells and should resolve the remaining issues (including some fixes).

While doing more debugging and testing, the following issues came up that are indirectly related:

• StrategyDuplicate: resync_metadata updates the EXIF data except for the takedate. Is this on purpose?
• FromURL: import always sets delete_imported to true, which should obviously not be possible. Am I missing something?
• Exec: reports too many errors (e.g. updating metadata results in an error because no file was uploaded). This is just cosmetic but makes the progress report from importing a bit confusing.
• Changing advanced Lychee settings do not get propagated to current lychee JS object (e.g. lychee.delete_imported only gets update after reloading the page).
• Unlike other modal dialogs, the final import report panel cannot be dismissed/closed using ESC.

For now I just wanted to keep a record of those observations.

[kamil4]

* StrategyDuplicate: `resync_metadata` updates the EXIF data _except_ for the `takedate`. Is this on purpose?

Well, until your changes resync_metadata was only available via a console command, correct? There is a separate console command to update the takedate so I think we may have thought that it's better not to overlap functionality between the two. That may not be applicable with the GUI, where we don't have a capability to update the takedate.

* FromURL: import always sets `delete_imported` to `true`, which should obviously not be possible. Am I missing something?

Doesn't it simply delete the temporary file from the import directory, which has the fetched content of the url?

* Exec: reports too many errors (e.g. updating metadata results in an error because no file was uploaded). This is just cosmetic but makes the progress report from importing a bit confusing.

Yeah, I have noticed it does that with skip_duplicates as well. What do you think we should do? Simply report success (making it indistinguishable from new file imports) or should it be reporting skipped/resynced files?

* Changing advanced Lychee settings do not get propagated to current `lychee` JS object (e.g. `lychee.delete_imported` only gets update after reloading the page).

Yeah... I guess we should just reinitialize using window.location.reload(). I see it's already being called in settings.js but maybe it needs to be added in another place there?

* Unlike other modal dialogs, the final import report panel cannot be dismissed/closed using ESC.

I see that upload.show defines that button as action. If you change it to cancel, it will probably do what you want. I worry though that it may then be possible to close it using Esc during import, which probably wouldn't be a good thing... But that's probably fixable (the handling of esc is defined in init.js; search for Mousetrap).

For now I just wanted to keep a record of those observations.

I hope I pointed you in the right directions, should you feel like doing something about them! 😃

[evoludolab]

Well, until your changes resync_metadata was only available via a console command, correct?

Yes it's only part of lychee:sync. Actually, I incidentally stumbled over resync_metadata and thought it would make sense to offer this option on the import panel as well.

The purpose of the lychee:sync and lychee:takedate commands are slightly different and so I would argue that it does make sense to include updating the takedate with resync_metadata. I cannot imagine that this would interfere with existing workflows. In addition, a separate resync_metadata option could be added to lychee:takedate to update all EXIF data.

Doesn't it simply delete the temporary file from the import directory, which has the fetched content of the url?

Sounds reasonable. I'll have a closer look.

Yeah, I have noticed it does that with skip_duplicates as well. What do you think we should do? Simply report success (making it indistinguishable from new file imports) or should it be reporting skipped/resynced files?

I think it should be the latter. Currently StrategyPhoto and StrategyDuplicate do a pretty good job of accurately logging what is going on but the calling Exec is ignorant of all this and simply reports an undifferentiated 'Could not import file'. I also think this would probably involve some restructuring that I would not feel comfortable to make any propositions for.

Yeah... I guess we should just reinitialize using window.location.reload(). I see it's already being called in settings.js but maybe it needs to be added in another place there?

I see that upload.show defines that button as action. If you change it to cancel, it will probably do what you want. I worry though that it may then be possible to close it using Esc during import, which probably wouldn't be a good thing... But that's probably fixable (the handling of esc is defined in init.js; search for Mousetrap).

Thanks for the leads - that's very helpful. In particular, it might be nice to have the option to cancel the import in a controlled manner. I am thinking of ESC (and/or a cancel button) triggering a confirmation dialog to stop the import and redefine ESC to close the import dialog on completion. I'll look into this.

[evoludolab]

• StrategyDuplicate: resync_metadata updates the EXIF data except for the takedate. Is this on purpose?

@kamil4 I think I found the culprit. In my view this was not intentional but rather a bug that prevented entries with value null from getting updated. Because I manually set takedate to null in the DB to check resync_metadata it never got updated but would have worked as expected when changing the value to anything else. I would appreciate if you get a chance to check LycheeOrg/Lychee@dbe06cd.

[kamil4]

I think it should be the latter. Currently StrategyPhoto and StrategyDuplicate do a pretty good job of accurately logging what is going on but the calling Exec is ignorant of all this and simply reports an undifferentiated 'Could not import file'. I also think this would probably involve some restructuring that I would not feel comfortable to make any propositions for.

Yes, I'll look into it...

In particular, it might be nice to have the option to cancel the import in a controlled manner. I am thinking of ESC (and/or a cancel button) triggering a confirmation dialog to stop the import and redefine ESC to close the import dialog on completion. I'll look into this.

I have no idea how the front end could notify the server to cancel an ongoing operation. It doesn't mean that it can't be done though 😃. We already do something similar in the opposite direction, in that the server provides updates on the progress of the ongoing import. We added those not just because it's nice for the user to know what's going on, but because without periodic traffic over the open socket, the web browser would close it after a timeout, so the final status of an import operation would never be received. So perhaps it is possible to do something like that in the other direction as well.

[kamil4]

OK, I'm done here!

@evoludolab, could you check if it still works correctly for you?

I had to fix the skip_duplicate logic on the server (user selection was being ignored if it was set to 1 in the settings). I also improved the reporting of duplicates so that it no longer generates a generic message.

[evoludolab]

Thanks @kamil4. I just did some quick checks and everything looks good. I like the improved reporting during import and your clean-up with skip_duplicates - in particular, retiring force_skip_duplicates and resolving the logic of skip_duplicates settings (I had noticed the inconsistency too when resolving the code conflicts that resulted from your work on image rotation - but never got to commit them...).

I would still like to activate the ESC button to close the final import dialog (and possibly allow to cancel the import). Unfortunately, however, I am currently pretty busy with other things and it might be a while before I get around to look into this further. As a consequence it's probably best to postpone and open a separate PR once I get a chance to address those items.

[kamil4]

I would still like to activate the ESC button to close the final import dialog (and possibly allow to cancel the import). Unfortunately, however, I am currently pretty busy with other things and it might be a while before I get around to look into this further. As a consequence it's probably best to postpone and open a separate PR once I get a chance to address those items.

Yeah, the current PR is complete and should be merged as-is. If you get around to activating the Esc button, just submit a new PR.

[ildyria]

I get this when I try:

main.js?1612722657:4843 
{description: "Server error or API not found.", params: {…}, response: SyntaxError: Unexpected token 
 in JSON at position 161
    at parse (<anonymous>)
    at http://ly…}
description: "Server error or API not found."
params:
albumID: 0
delete_imported: "1"
function: "Import::server"
import_via_symlink: "0"
path: "/var/www/html/Lychee/public/uploads/import/"
resync_metadata: "0"
skip_duplicates: "0"
__proto__: Object
response: SyntaxError: Unexpected token in JSON at position 161 at parse (<anonymous>) at http://lychee.test/dist/main.js?1612722657:2:79369 at l (http://lychee.test/dist/main.js?1612722657:2:79486) at XMLHttpRequest.<anonymous> (http://lychee.test/dist/main.js?1612722657:2:82254)
message: "Unexpected token ↵ in JSON at position 161"
stack: "SyntaxError: Unexpected token ↵ in JSON at position 161↵    at parse (<anonymous>)↵    at http://lychee.test/dist/main.js?1612722657:2:79369↵    at l (http://lychee.test/dist/main.js?1612722657:2:79486)↵    at XMLHttpRequest.<anonymous> (http://lychee.test/dist/main.js?1612722657:2:82254)"
__proto__: Error
__proto__: Object

[kamil4]

I'm unable to reproduce. Again, I suspect that your server and front end were out of sync.

[ildyria]

I'm unable to reproduce. Again, I suspect that your server and front end were out of sync.

Nah most likely some other bug. Whatever I'm not using this functionality. :)

[evoludolab]

Implementing the option to abort the import process on the server in a controlled manner was an interesting exercise. In the end persistence prevailed and the solution is pretty simple. The ESC button provides a shortcut to cancel the import and once the import has finished closes the modal dialog. Now the PR includes all features I had planned at this point.

[ildyria]

@evoludolab I did a minor tweak both on the front and back end of your pull request, please check if it still works. This simplified the code base slightly. :)

[evoludolab]

Looks indeed cleaner but unfortunately does not work anymore - I'm working on it. Just a heads-up, I had renamed the route at the last moment and forgot to change it in upload.js... Should be Import::serverCancel (not Import::server_cancel on l.35). Will take care of that too. Back soon.

[evoludolab]

Fixed. Session variables do not get automatically updated in a running session and need to be explicitly read again (once I learned about sessions this was the trickiest part to realize 😃). Thus, the call $store->start() in Exec.php is crucial but can be replaced by session()->start().

@ildyria Thanks for streamlining the cancel request on both ends and getting rid of those silly dummy parameters. I was sure there was a better way to communicate with the server - now I know how!

[evoludolab]

Now I am done here 😉

[kamil4]

See my comment about what looks like a merge error.

I need to try this out before a final approval but it won't be tonight (sorry, it's past midnight already...). I hope to be able to do it tomorrow.

[kamil4]

OK, I made my changes (mainly adding the Cancel button to regular local uploads, because I liked it so much for the server imports 😃).

Now, can we please finally focus on getting this merged rather than keep piling on more tweaks and new features? Because, with my grupy-old-man hat firmly on, this PR is driving me crazy! I feel like I've reviewed it five times already, and today I honestly kept asking myself What? This part is still not merged?! At this point, I consider it virtually unreviewable, especially after the last round of formatting tweaks from prettier (I hate that thing!). This should've been like three separate PRs!

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication