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 OnDelete Notification to TraktConnection #4869

Merged
merged 1 commit into from
Sep 4, 2020
Merged

Add OnDelete Notification to TraktConnection #4869

merged 1 commit into from
Sep 4, 2020

Conversation

geogolem
Copy link
Contributor

@geogolem geogolem commented Aug 16, 2020

Database Migration

YES - needed to add migration 182 --> on_delete_notification

Description

This will notify Trakt to remove an item from Collection when a movie file is deleted. On delete must be selected and enabled for the Trakt Connection

Todos

  • Tests

Issues Fixed or Closed by this PR

#4521

Another feature still needs to be implemented. A button to manually trigger a full sync should be added still - that will be handled in a different PR in the future.

@rg9400
Copy link
Contributor

rg9400 commented Aug 16, 2020

@Qstick OnDelete could be useful for other connections as well, but if we're going to add an initialization sync + re-sync option, it may make sense to move this over to a new Integrations tab.

@Qstick
Copy link
Member

Qstick commented Aug 16, 2020

Yea, that was the plan to add onDelete to all, integrations isn’t something for v3 we are trying to get it pushed out. So I’d leave the sync out of this for now.

@geogolem geogolem requested a review from Qstick August 18, 2020 23:52
@Qstick
Copy link
Member

Qstick commented Aug 19, 2020

Looking good, can you just add the unit test case for onDelete to src\NzbDrone.Core.Test\NotificationTests\NotificationBaseFixture.cs

@geogolem
Copy link
Contributor Author

Looking good, can you just add the unit test case for onDelete to src\NzbDrone.Core.Test\NotificationTests\NotificationBaseFixture.cs

I added the unit tests but I'm not 100% sure I did it properly. I think so, but let me know if I went wrong.

Qstick
Qstick previously approved these changes Aug 23, 2020
@Qstick
Copy link
Member

Qstick commented Aug 28, 2020

Can you squash this down to one or two logical commits. Also use New: xxxxxx for commit name so it shows in changelog

to remove movies from Trakt Collection when they are removed from Radarr

Add OnDelete Notification to TraktConnection to remove movies from Trakt Collection when they are removed from Radarr

skip the deleteHandler if the delete Event was triggered for reason: Upgrade

change migration from 180 to 182 since 180 and 181 are already in plan
to be used

address comments regarding helpText for OnDelete
and move check for OnUpgrade deletion reason into trakt connection OnDelete handler

reuse TraktCollectMoviesResource for OnDelete
trakt api should just ignore the other properties anyway

add WithDefaultValue to migration

add unit test case for onDelete to

fix unit testcase for onDelete
@Qstick Qstick merged commit e033ce1 into Radarr:aphrodite Sep 4, 2020
@geogolem geogolem deleted the TraktConnectionOnDeleteUpdate branch September 5, 2020 14:06
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants