-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix(import): don't throw away album flexible fields #5388
fix(import): don't throw away album flexible fields #5388
Conversation
c6dfe1e
to
dc61ef0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very clean PR, thank you! Added a suggestion, and requesting changes since the PR needs rebasing :)
aba5877
to
08bcbd4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small comment regarding the change in the workflows
08bcbd4
to
c131d5d
Compare
As noted by 5bf4e3d, MusicBrainz external IDs (`*_album_id`) were only saved for items and not albums. This commit addresses that by copying `AlbumInfo` fields to the `Album`, i.e. what's saved in the DB. This is similar to how `TrackInfo` fields are copied to `Item` instances except the copying is done at a different time since we only get an `Album` much later in the import flow.
Co-authored-by: Šarūnas Nejus <snejus@protonmail.com>
c131d5d
to
bf32d9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the adjustments, you're good to go!
Thanks for the review! |
@ThinkChaos thank you very much for this PR! I love the refactoring you did to the *_id fetching code! So much cleaner and shorter now! Also many thanks to @snejus for taking over with a review, much appreciated! ❤️ |
Description
I believe this fixes #4785, but I might be misunderstanding the description and it might already be fixed.
Tagging @JOJ0 since they created both the bug linked above, and 5bf4e3d which mentions it.
To Do
Documentation. (If you've added a new command-line flag, for example, find the appropriate page underdocs/
to describe it.)docs/changelog.rst
to the bottom of one of the lists near the top of the document.)