Skip to content

fix: relink unlinked entry to existing entry without sql error #720

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

Merged
merged 6 commits into from
Jan 28, 2025

Conversation

mashed5894
Copy link
Contributor

Fix for #657. I attempted to use mirror_entry_fields in library.py, but it didn't seem to do what I wanted. I don't know if it's a bug or a skill issue on my part, so I wrote merge_entries to do exactly what I needed.

@CyanVoxel CyanVoxel added Type: Bug Something isn't working as intended TagStudio: Library Relating to the TagStudio library system Priority: High An important issue requiring attention labels Jan 23, 2025
@CyanVoxel CyanVoxel added this to the SQL Parity milestone Jan 23, 2025
@CyanVoxel CyanVoxel linked an issue Jan 23, 2025 that may be closed by this pull request
3 tasks
@mashed5894 mashed5894 marked this pull request as ready for review January 23, 2025 00:38
@CyanVoxel CyanVoxel added the Status: Review Needed A review of this is needed label Jan 23, 2025
Copy link
Member

@CyanVoxel CyanVoxel left a comment

Choose a reason for hiding this comment

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

Your code seems to work just fine here, however upon fixing this there's been yet another issue found with the original implementation that for some reason was changed during the initial jump to SQL.

Thefix_missing_files() method inside missing_files.py file was written incorrectly and tries to remove entries from the files_to_remove list while iterating over it, resulting in not all duplicate entries being removed. The wording in that method is also very confusing to me and mixes terms such as "file" and "entry" and is ambiguous as to what the "fix" actually is. The test for that method also appears to assert the incorrect result, though it's also hard to follow the state of fixtures within pytest.

That is to say, you've done a great job here and rather than ask you to clog this PR with my additional changes, I'll just be pulling this into a new branch and applying these tangential fixes and refactors before pulling into main.

Thank you for working on this!

@CyanVoxel CyanVoxel added Status: Mergeable The code is ready to be merged and removed Status: Review Needed A review of this is needed labels Jan 28, 2025
@CyanVoxel CyanVoxel changed the base branch from main to fix-unlinked-again January 28, 2025 06:14
@CyanVoxel CyanVoxel merged commit 986dc2d into TagStudioDev:fix-unlinked-again Jan 28, 2025
4 checks passed
CyanVoxel added a commit that referenced this pull request Jan 28, 2025
* fix: relink unlinked entry to existing entry without sql error (#720)

* edited and added db functions get_entry_full_by_path & merge_entries

* implemented edge case for entry existing on relinking

* added test for merge_entries

* fix: don't remove item while iterating over list

* fix: catch `FileNotFoundError` for unlinked raw files

* refactor: rename methods and variables in missing_files.py

* refactor: rename `missing_files_count` to `missing_file_entries_count`

---------

Co-authored-by: mashed5894 <mashed5894@gmail.com>
@mashed5894 mashed5894 deleted the relink-existing branch January 29, 2025 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High An important issue requiring attention Status: Mergeable The code is ready to be merged TagStudio: Library Relating to the TagStudio library system Type: Bug Something isn't working as intended
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[Bug]: Relinking with "Fix Unlinked Entries" breaks if new entry exists (SQL)
2 participants