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

Split composite status into status and quality, add Unknown as valid quality #4221

Merged
merged 102 commits into from
Jun 3, 2018

Conversation

p0psicles
Copy link
Contributor

@p0psicles p0psicles commented May 19, 2018

This PR contains major changes to our quality system.

  1. It splits the composite qualities into status (Downloaded, Skipped, etc.) and quality (HDTV, 720p WEB-DL, etc.). This allows us to remove a lot of superfluous code and will make a better quality system possible in the future.
  2. Existing qualities are being shifted by one spot. This means that we can use Unknown as a valid quality (the lowest available), thus eliminating a bunch of annoying hacks and allowing it to be set even as Preferred (e.g. for x265 content)

It goes without saying that these are massive changes that should be tested thoroughly over a minimum period of time. It would be great if anyone could give this a spin in the next couple of days. The upgrade process is automated and backup files for main.db, failed.db and cache.db will be created.


  • PR is based on the DEVELOP branch
  • Don't send big changes all at once. Split up big PRs into multiple smaller PRs that are easier to manage and review
  • Read the contribution guide

self.connection.action("DROP TABLE IF EXISTS new_tv_episodes;")

log.info(u'Split composite status in to ep_status and ep_quality')
sql_results = self.connection.select("SELECT status from tv_episodes GROUP BY status HAVING status > -1")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@medariox Im not sure if we also need to translate status of -1

@p0psicles
Copy link
Contributor Author

p0psicles commented May 19, 2018

Pff this will require a lot of testing.

@medariox medariox changed the title Added ep_status and ep_quality to tv_episodes table Added status and quality to tv_episodes table May 20, 2018
@sharkykh

This comment has been minimized.

sharkykh added 3 commits June 2, 2018 02:56
* Set status to app.EP_DEFAULT_DELETED_STATUS if episode file was delete and old status was ARCHIVED or DOWNLOADED or IGNORED or SKIPPED

* Add some episode attributes that should be reset if a file is deleted
@sharkykh sharkykh force-pushed the feature/add-db-status-quality-fields branch from 55e01c5 to 25346d9 Compare June 2, 2018 02:25
sharkykh
sharkykh previously approved these changes Jun 2, 2018
Copy link
Contributor

@sharkykh sharkykh left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I also committed to this branch.

I would prefer it if the compact history "duplicate" qualities could be fixed, but I don't think it should stop you from merging.
Continued in a comment.

@sharkykh
Copy link
Contributor

sharkykh commented Jun 2, 2018

I tried to fix the history "bug" myself but hit a snag.
FWIW I found that because the action database column was a composite status, Action could be defined by it (the quality could be extracted from the action), and Index was defined by the quality column.

But because of the composite statuses split, now Action has quality (because it has to), and Index doesn't have it, so when the items are compacted it's compacting all of the history items of the same indexer, show, season and episode - ignoring the quality.


Edit: as I was writing this comment, I figured out what was the issue I encountered, the 2 newest commits should restore the compact layout to what it was (without affecting detailed)

sharkykh
sharkykh previously approved these changes Jun 2, 2018
@p0psicles
Copy link
Contributor Author

Both History and episode status management where broken for me.

Copy link
Contributor

@medariox medariox left a comment

Choose a reason for hiding this comment

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

I think this is good to go 🎉

@p0psicles
Copy link
Contributor Author

I dont know. I feel like we should really do more testing. The last two bugs i noticed where really obvious. And i couldnt test it sooner. As my db is a mess.

@medariox
Copy link
Contributor

medariox commented Jun 3, 2018

That's why we should merge it to develop ASAP, to have some more people actually test it.

@p0psicles p0psicles merged commit 55b472b into develop Jun 3, 2018
@p0psicles p0psicles deleted the feature/add-db-status-quality-fields branch June 3, 2018 15:03
@p0psicles
Copy link
Contributor Author

Good point

@sharkykh sharkykh mentioned this pull request Sep 12, 2018
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concluded Migration Database and/or Config migration performed Needs review Needs testing Requires testing to make sure it's working as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants