spl0k / supysonic

Supysonic is a Python implementation of the Subsonic server API.
https://supysonic.readthedocs.io
GNU Affero General Public License v3.0
259 stars 57 forks source link

Fix for multiple artist album without album artist tag showing up as an album per artist (issue #254) #258

Open ianesten opened 11 months ago

ianesten commented 11 months ago

Note: the database must be recreated for this fix to work.

codecov[bot] commented 11 months ago

Codecov Report

Merging #258 (13cf848) into master (0fa0d55) will decrease coverage by 18.70%. The diff coverage is 80.00%.

@@             Coverage Diff             @@
##           master     #258       +/-   ##
===========================================
- Coverage   86.29%   67.60%   -18.70%     
===========================================
  Files          46       46               
  Lines        3801     3812       +11     
===========================================
- Hits         3280     2577      -703     
- Misses        521     1235      +714     
Files Changed Coverage Δ
supysonic/scanner.py 94.24% <78.57%> (-1.47%) :arrow_down:
supysonic/db.py 69.17% <100.00%> (-23.55%) :arrow_down:

... and 13 files with indirect coverage changes

spl0k commented 11 months ago

Hello. As stated in #254, this is not a behaviour I'd like to be introduced in Supysonic. But after thinking a bit about it, I might accept this PR if the behaviour were to be changed a bit. Rather than just picking an album that would reside in the same folder and calling it a day, if this album artist is different than the current track artist I'd rather see the album artist changed to something that clearly states that this is a multi-artist album (such as "Various Artists").

Now, for remarks about the current state of the PR. First, having to recreate the database is a big no-no. Existing data shouldn't be destroyed. If the schema changes, the you must provide migrations for all providers, both upgrading the schema and the data if needed. But continue reading as this might actually not be necessary. Secondly, this new folder foreign key in Album doesn't seem useful to me. Tracks have a path that can be used to know in which folder they reside (thing you're already doing). From there you can find other tracks in this folder and then get an album. Tying an album to a folder might not be a good idea as this could lead to some inconsistencies for example if an album is spread across multiple folders (think multi-discs albums where people could store each disc in its own folder). Plus removing this new FK might certainly fix the next remark. Lastly, make sure the unit tests still pass. Right now a large number of them are failing because of the new FK. You can run them using the command

python -m unittest