Skip to content
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

Add Tests, Fix metadata refresh deletes backgrounds #6752

Merged

Conversation

1337joe
Copy link
Member

@1337joe 1337joe commented Nov 1, 2021

Changes

  • Add unit tests and documentation for ItemImageProvider
  • Fix failing tests:
    • Refreshing all images when the provider didn't have new images resulted in multi-image types (background, screenshots) being deleted while single-image types kept the old value.
    • Multi-image types could delete invalid paths without reporting changed, other image types had better tracking.
  • Refactor validation to use existing BaseItem.ValidateImages method up front instead of validating images piecemeal throughout the add/update handling.

Issues
Fixes #6310

Fix failing test: Invalid background images not purged by validate
Fixes jellyfin#6310: Background images only delete when using "Replace existing images" when new image(s) is found to replace them
Comment on lines +677 to +681
// Create a class that implements IHasScreenshots for testing since no BaseItem class is also IHasScreenshots
private class MovieWithScreenshots : Movie, IHasScreenshots
{
// No contents
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is now the only class in my workspace that is a BaseItem that implements IHasScreenshots.

Assuming it's not used in some other plugin that I don't have checked out, should either the handling for IHasScreenshots be removed or the interface itself be removed as unused?

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't remove the interface, we never know if there is a plugin that implements it.

Copy link
Member

Choose a reason for hiding this comment

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

@1337joe 1337joe force-pushed the fix-metadata-refresh-deletes-backgrounds branch from a97b48b to b478b11 Compare November 1, 2021 10:54
@jellyfin-bot jellyfin-bot added the merge conflict Merge conflicts should be resolved before a merge label Nov 1, 2021
@jellyfin-bot jellyfin-bot removed the merge conflict Merge conflicts should be resolved before a merge label Nov 1, 2021
@cvium cvium merged commit 869d537 into jellyfin:master Nov 3, 2021
@1337joe 1337joe deleted the fix-metadata-refresh-deletes-backgrounds branch November 4, 2021 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metadata refresh deletes existing background images when there are non to fetch
5 participants