Add option to leave files where they are while importing library#503
Add option to leave files where they are while importing library#503T4g1 wants to merge 3 commits intoListenarrs:canaryfrom
Conversation
df07abd to
19eed3f
Compare
… on files and set monitoring
2024229 to
b5cf323
Compare
therobbiedavis
left a comment
There was a problem hiding this comment.
Solid job! Thank you! Just a few changes and a request to please update the changelog!
|
|
||
| foreach (var item in orderedItems) | ||
| { | ||
| var fileCount = orderedItems.Select(f => f.MatchedAudiobookId).Count(); |
There was a problem hiding this comment.
This will count every item in the batch regardless of audiobook. Example: if you import 1 file for Book A and 1 file for Book B, both files see fileCount = 2 and are treated as multi-file imports, causing wrong naming.
| } | ||
| } | ||
|
|
||
| public async Task PerformActionOn(Listenarr.Api.Services.FileMover.FileAction action, string source, string? destination = null, HashSet<string>? usedDestinations = null) |
There was a problem hiding this comment.
why not use usedDestinations? It's used for collision detection in the filemover.
listenarr.api/Services/IFileMover.cs
Outdated
| /// <param name="action">What we want to do with the file</param> | ||
| /// <param name="source">File</param> | ||
| /// <param name="destination">Optional destination of the action</param> | ||
| /// <param name="usedDestinations">To remove probably</param> |
There was a problem hiding this comment.
If we're removing then lets refactor and remove, otherwise lets update this description.
| public string? BasePath { | ||
| get | ||
| { | ||
| return FileUtils.NormalizeStoredPath(field); |
There was a problem hiding this comment.
Lets instead normalize on the setter so reads are free
There was a problem hiding this comment.
I would agree but what about values already in DB then ? Are we sure all values are currently normalized already ?
There was a problem hiding this comment.
Yes, so let's include a one-time normalization migration with this to normalize existing BasePaths
There was a problem hiding this comment.
I'm gonna need some pointers on how to achieve that, afaik, migration cannot be used to run arbitrary C# methods on the database and the logic of NormalizeStoredPath seems really complex to translate to SQL
There was a problem hiding this comment.
Generate a migration like so:
dotnet ef migrations add NormalizeStoredPaths --project listenarr.infrastructure --startup-project listenarr.api
then add the following to the generated Up() in the migration:
migrationBuilder.Sql(@"
UPDATE Audiobooks
SET BasePath = RTRIM(TRIM(BasePath), '/\')
WHERE BasePath IS NOT NULL AND BasePath != RTRIM(TRIM(BasePath), '/\');
UPDATE ApplicationSettings
SET OutputPath = RTRIM(TRIM(OutputPath), '/\')
WHERE OutputPath != RTRIM(TRIM(OutputPath), '/\');
");
| { | ||
| get | ||
| { | ||
| return FileUtils.NormalizeStoredPath(field); |
There was a problem hiding this comment.
same here, normalize on the setter
There was a problem hiding this comment.
let's include a one-time normalization migration with this to normalize existing OutputPaths, then normalize the setter
fe/src/stores/rootFolders.ts
Outdated
|
|
||
| return { folders, loading, defaultFolder, load, create, update, remove } | ||
| function getLast() { | ||
| return folders.value.at(-1); |
There was a problem hiding this comment.
this assumes two people are not creating folders at the same time. We might want RootFolderFormModal to emit the saved entity's ID and resolve it directly.
There was a problem hiding this comment.
I know this probably isn't a valid use-case, but also I've learned long ago not to assume how a user will use your software.
|
@therobbiedavis Thanks! I did a pass on your reviews, can you check again to see if we are aligned ? |
There was a problem hiding this comment.
@T4g1 Added a couple comments on the existing issues, outside of these it lgtm!
| public string? BasePath { | ||
| get | ||
| { | ||
| return FileUtils.NormalizeStoredPath(field); |
There was a problem hiding this comment.
Yes, so let's include a one-time normalization migration with this to normalize existing BasePaths
| { | ||
| get | ||
| { | ||
| return FileUtils.NormalizeStoredPath(field); |
There was a problem hiding this comment.
let's include a one-time normalization migration with this to normalize existing OutputPaths, then normalize the setter
CHANGELOG.md
Outdated
|
|
||
| ### Changed | ||
| - **Reduced redundancy for the manual import modal:** Small improvement that make it easier to add or edit import fields. | ||
| ## [0.2.66] - ? |
There was a problem hiding this comment.
Changelog needs to be updated from canary or rebased, then your changes would be under
## [Unreleased]
at the top. We will update this with a version and date before release.
Summary
Changes
Added
Testing
Notes
Next steps