From 8cd9ab4a9f0affbada8e1af7f3375341f7f0f669 Mon Sep 17 00:00:00 2001 From: ta264 Date: Mon, 10 Jun 2019 19:56:44 +0100 Subject: [PATCH] Add: option to skip automatic redownload when removing from queue (#734) * Add: option to skip automatic redownload when removing from queue * Add tests for RedownloadFailedDownloadService * Fix formatting * Make re-download dialog conditional --- frontend/src/Activity/Queue/Queue.js | 4 +- frontend/src/Activity/Queue/QueueConnector.js | 4 +- frontend/src/Activity/Queue/QueueRow.js | 4 +- .../src/Activity/Queue/QueueRowConnector.js | 4 +- .../Activity/Queue/RemoveQueueItemModal.js | 37 ++++- .../Activity/Queue/RemoveQueueItemsModal.js | 37 ++++- frontend/src/Store/Actions/queueActions.js | 10 +- src/Lidarr.Api.V1/Queue/QueueActionModule.cs | 10 +- .../RedownloadFailedDownloadServiceFixture.cs | 128 ++++++++++++++++++ .../NzbDrone.Core.Test.csproj | 1 + .../Download/DownloadFailedEvent.cs | 1 + .../Download/FailedDownloadService.cs | 17 +-- .../RedownloadFailedDownloadService.cs | 8 +- 13 files changed, 230 insertions(+), 35 deletions(-) create mode 100644 src/NzbDrone.Core.Test/Download/RedownloadFailedDownloadServiceFixture.cs diff --git a/frontend/src/Activity/Queue/Queue.js b/frontend/src/Activity/Queue/Queue.js index f04ca1ceb..9169927a3 100644 --- a/frontend/src/Activity/Queue/Queue.js +++ b/frontend/src/Activity/Queue/Queue.js @@ -107,8 +107,8 @@ class Queue extends Component { this.setState({ isConfirmRemoveModalOpen: true }); } - onRemoveSelectedConfirmed = (blacklist) => { - this.props.onRemoveSelectedPress(this.getSelectedIds(), blacklist); + onRemoveSelectedConfirmed = (blacklist, skipredownload) => { + this.props.onRemoveSelectedPress(this.getSelectedIds(), blacklist, skipredownload); this.setState({ isConfirmRemoveModalOpen: false }); } diff --git a/frontend/src/Activity/Queue/QueueConnector.js b/frontend/src/Activity/Queue/QueueConnector.js index 6b40f6402..ea571e8af 100644 --- a/frontend/src/Activity/Queue/QueueConnector.js +++ b/frontend/src/Activity/Queue/QueueConnector.js @@ -137,8 +137,8 @@ class QueueConnector extends Component { this.props.grabQueueItems({ ids }); } - onRemoveSelectedPress = (ids, blacklist) => { - this.props.removeQueueItems({ ids, blacklist }); + onRemoveSelectedPress = (ids, blacklist, skipredownload) => { + this.props.removeQueueItems({ ids, blacklist, skipredownload }); } // diff --git a/frontend/src/Activity/Queue/QueueRow.js b/frontend/src/Activity/Queue/QueueRow.js index 71f9f342b..8ac4f3253 100644 --- a/frontend/src/Activity/Queue/QueueRow.js +++ b/frontend/src/Activity/Queue/QueueRow.js @@ -42,8 +42,8 @@ class QueueRow extends Component { this.setState({ isRemoveQueueItemModalOpen: true }); } - onRemoveQueueItemModalConfirmed = (blacklist) => { - this.props.onRemoveQueueItemPress(blacklist); + onRemoveQueueItemModalConfirmed = (blacklist, skipredownload) => { + this.props.onRemoveQueueItemPress(blacklist, skipredownload); this.setState({ isRemoveQueueItemModalOpen: false }); } diff --git a/frontend/src/Activity/Queue/QueueRowConnector.js b/frontend/src/Activity/Queue/QueueRowConnector.js index 9d3938b13..6bbbde361 100644 --- a/frontend/src/Activity/Queue/QueueRowConnector.js +++ b/frontend/src/Activity/Queue/QueueRowConnector.js @@ -43,8 +43,8 @@ class QueueRowConnector extends Component { this.props.grabQueueItem({ id: this.props.id }); } - onRemoveQueueItemPress = (blacklist) => { - this.props.removeQueueItem({ id: this.props.id, blacklist }); + onRemoveQueueItemPress = (blacklist, skipredownload) => { + this.props.removeQueueItem({ id: this.props.id, blacklist, skipredownload }); } // diff --git a/frontend/src/Activity/Queue/RemoveQueueItemModal.js b/frontend/src/Activity/Queue/RemoveQueueItemModal.js index 1afeaeb78..6e6358c95 100644 --- a/frontend/src/Activity/Queue/RemoveQueueItemModal.js +++ b/frontend/src/Activity/Queue/RemoveQueueItemModal.js @@ -21,7 +21,8 @@ class RemoveQueueItemModal extends Component { super(props, context); this.state = { - blacklist: false + blacklist: false, + skipredownload: false }; } @@ -32,15 +33,26 @@ class RemoveQueueItemModal extends Component { this.setState({ blacklist: value }); } + onSkipReDownloadChange = ({ value }) => { + this.setState({ skipredownload: value }); + } + onRemoveQueueItemConfirmed = () => { const blacklist = this.state.blacklist; + const skipredownload = this.state.skipredownload; - this.setState({ blacklist: false }); - this.props.onRemovePress(blacklist); + this.setState({ + blacklist: false, + skipredownload: false + }); + this.props.onRemovePress(blacklist, skipredownload); } onModalClose = () => { - this.setState({ blacklist: false }); + this.setState({ + blacklist: false, + skipredownload: false + }); this.props.onModalClose(); } @@ -54,6 +66,7 @@ class RemoveQueueItemModal extends Component { } = this.props; const blacklist = this.state.blacklist; + const skipredownload = this.state.skipredownload; return ( + { + blacklist && + + Skip Redownload + + + } + diff --git a/frontend/src/Activity/Queue/RemoveQueueItemsModal.js b/frontend/src/Activity/Queue/RemoveQueueItemsModal.js index 9d9229b29..b573c5cbd 100644 --- a/frontend/src/Activity/Queue/RemoveQueueItemsModal.js +++ b/frontend/src/Activity/Queue/RemoveQueueItemsModal.js @@ -21,7 +21,8 @@ class RemoveQueueItemsModal extends Component { super(props, context); this.state = { - blacklist: false + blacklist: false, + skipredownload: false }; } @@ -32,15 +33,26 @@ class RemoveQueueItemsModal extends Component { this.setState({ blacklist: value }); } + onSkipReDownloadChange = ({ value }) => { + this.setState({ skipredownload: value }); + } + onRemoveQueueItemConfirmed = () => { const blacklist = this.state.blacklist; + const skipredownload = this.state.skipredownload; - this.setState({ blacklist: false }); - this.props.onRemovePress(blacklist); + this.setState({ + blacklist: false, + skipredownload: false + }); + this.props.onRemovePress(blacklist, skipredownload); } onModalClose = () => { - this.setState({ blacklist: false }); + this.setState({ + blacklist: false, + skipredownload: false + }); this.props.onModalClose(); } @@ -54,6 +66,7 @@ class RemoveQueueItemsModal extends Component { } = this.props; const blacklist = this.state.blacklist; + const skipredownload = this.state.skipredownload; return ( + { + blacklist && + + Skip Redownload + + + } + diff --git a/frontend/src/Store/Actions/queueActions.js b/frontend/src/Store/Actions/queueActions.js index 221d66267..8f67e08bc 100644 --- a/frontend/src/Store/Actions/queueActions.js +++ b/frontend/src/Store/Actions/queueActions.js @@ -351,13 +351,14 @@ export const actionHandlers = handleThunks({ [REMOVE_QUEUE_ITEM]: function(getState, payload, dispatch) { const { id, - blacklist + blacklist, + skipredownload } = payload; dispatch(updateItem({ section: paged, id, isRemoving: true })); const promise = createAjaxRequest({ - url: `/queue/${id}?blacklist=${blacklist}`, + url: `/queue/${id}?blacklist=${blacklist}&skipredownload=${skipredownload}`, method: 'DELETE' }).request; @@ -373,7 +374,8 @@ export const actionHandlers = handleThunks({ [REMOVE_QUEUE_ITEMS]: function(getState, payload, dispatch) { const { ids, - blacklist + blacklist, + skipredownload } = payload; dispatch(batchActions([ @@ -389,7 +391,7 @@ export const actionHandlers = handleThunks({ ])); const promise = createAjaxRequest({ - url: `/queue/bulk?blacklist=${blacklist}`, + url: `/queue/bulk?blacklist=${blacklist}&skipredownload=${skipredownload}`, method: 'DELETE', dataType: 'json', data: JSON.stringify({ ids }) diff --git a/src/Lidarr.Api.V1/Queue/QueueActionModule.cs b/src/Lidarr.Api.V1/Queue/QueueActionModule.cs index e82c50ff5..a37165613 100644 --- a/src/Lidarr.Api.V1/Queue/QueueActionModule.cs +++ b/src/Lidarr.Api.V1/Queue/QueueActionModule.cs @@ -77,8 +77,9 @@ namespace Lidarr.Api.V1.Queue private Response Remove(int id) { var blacklist = Request.GetBooleanQueryParameter("blacklist"); + var skipReDownload = Request.GetBooleanQueryParameter("skipredownload"); - var trackedDownload = Remove(id, blacklist); + var trackedDownload = Remove(id, blacklist, skipReDownload); if (trackedDownload != null) { @@ -91,13 +92,14 @@ namespace Lidarr.Api.V1.Queue private Response Remove() { var blacklist = Request.GetBooleanQueryParameter("blacklist"); + var skipReDownload = Request.GetBooleanQueryParameter("skipredownload"); var resource = Request.Body.FromJson(); var trackedDownloadIds = new List(); foreach (var id in resource.Ids) { - var trackedDownload = Remove(id, blacklist); + var trackedDownload = Remove(id, blacklist, skipReDownload); if (trackedDownload != null) { @@ -110,7 +112,7 @@ namespace Lidarr.Api.V1.Queue return new object().AsResponse(); } - private TrackedDownload Remove(int id, bool blacklist) + private TrackedDownload Remove(int id, bool blacklist, bool skipReDownload) { var pendingRelease = _pendingReleaseService.FindPendingQueueItem(id); @@ -139,7 +141,7 @@ namespace Lidarr.Api.V1.Queue if (blacklist) { - _failedDownloadService.MarkAsFailed(trackedDownload.DownloadItem.DownloadId); + _failedDownloadService.MarkAsFailed(trackedDownload.DownloadItem.DownloadId, skipReDownload); } return trackedDownload; diff --git a/src/NzbDrone.Core.Test/Download/RedownloadFailedDownloadServiceFixture.cs b/src/NzbDrone.Core.Test/Download/RedownloadFailedDownloadServiceFixture.cs new file mode 100644 index 000000000..a907d7ed5 --- /dev/null +++ b/src/NzbDrone.Core.Test/Download/RedownloadFailedDownloadServiceFixture.cs @@ -0,0 +1,128 @@ +using NUnit.Framework; +using NzbDrone.Core.Download; +using NzbDrone.Core.Test.Framework; +using NzbDrone.Core.Messaging.Commands; +using Moq; +using System.Collections.Generic; +using NzbDrone.Core.Music; +using FizzWare.NBuilder; +using NzbDrone.Core.Configuration; +using NzbDrone.Core.IndexerSearch; + +namespace NzbDrone.Core.Test.Download +{ + [TestFixture] + public class RedownloadFailedDownloadServiceFixture : CoreTest + { + [SetUp] + public void Setup() + { + Mocker.GetMock() + .Setup(x => x.AutoRedownloadFailed) + .Returns(true); + + Mocker.GetMock() + .Setup(x => x.GetAlbumsByArtist(It.IsAny())) + .Returns(Builder.CreateListOfSize(3).Build() as List); + } + + [Test] + public void should_skip_redownload_if_event_has_skipredownload_set() + { + var failedEvent = new DownloadFailedEvent { + ArtistId = 1, + AlbumIds = new List { 1 }, + SkipReDownload = true + }; + + Subject.HandleAsync(failedEvent); + + Mocker.GetMock() + .Verify(x => x.Push(It.IsAny(), It.IsAny(), It.IsAny()), + Times.Never()); + } + + [Test] + public void should_skip_redownload_if_redownload_failed_disabled() + { + var failedEvent = new DownloadFailedEvent { + ArtistId = 1, + AlbumIds = new List { 1 } + }; + + Mocker.GetMock() + .Setup(x => x.AutoRedownloadFailed) + .Returns(false); + + Subject.HandleAsync(failedEvent); + + Mocker.GetMock() + .Verify(x => x.Push(It.IsAny(), It.IsAny(), It.IsAny()), + Times.Never()); + } + + [Test] + public void should_redownload_album_on_failure() + { + var failedEvent = new DownloadFailedEvent { + ArtistId = 1, + AlbumIds = new List { 2 } + }; + + Subject.HandleAsync(failedEvent); + + Mocker.GetMock() + .Verify(x => x.Push(It.Is(c => c.AlbumIds.Count == 1 && + c.AlbumIds[0] == 2), + It.IsAny(), It.IsAny()), + Times.Once()); + + Mocker.GetMock() + .Verify(x => x.Push(It.IsAny(), It.IsAny(), It.IsAny()), + Times.Never()); + } + + [Test] + public void should_redownload_multiple_albums_on_failure() + { + var failedEvent = new DownloadFailedEvent { + ArtistId = 1, + AlbumIds = new List { 2, 3 } + }; + + Subject.HandleAsync(failedEvent); + + Mocker.GetMock() + .Verify(x => x.Push(It.Is(c => c.AlbumIds.Count == 2 && + c.AlbumIds[0] == 2 && + c.AlbumIds[1] == 3), + It.IsAny(), It.IsAny()), + Times.Once()); + + Mocker.GetMock() + .Verify(x => x.Push(It.IsAny(), It.IsAny(), It.IsAny()), + Times.Never()); + } + + [Test] + public void should_redownload_artist_on_failure() + { + // note that artist is set to have 3 albums in setup + var failedEvent = new DownloadFailedEvent { + ArtistId = 2, + AlbumIds = new List { 1, 2, 3 } + }; + + Subject.HandleAsync(failedEvent); + + Mocker.GetMock() + .Verify(x => x.Push(It.Is(c => c.ArtistId == failedEvent.ArtistId), + It.IsAny(), It.IsAny()), + Times.Once()); + + Mocker.GetMock() + .Verify(x => x.Push(It.IsAny(), It.IsAny(), It.IsAny()), + Times.Never()); + } + } +} diff --git a/src/NzbDrone.Core.Test/NzbDrone.Core.Test.csproj b/src/NzbDrone.Core.Test/NzbDrone.Core.Test.csproj index 31f5ff001..a977a07a7 100644 --- a/src/NzbDrone.Core.Test/NzbDrone.Core.Test.csproj +++ b/src/NzbDrone.Core.Test/NzbDrone.Core.Test.csproj @@ -139,6 +139,7 @@ + diff --git a/src/NzbDrone.Core/Download/DownloadFailedEvent.cs b/src/NzbDrone.Core/Download/DownloadFailedEvent.cs index c22dd9548..8145e9665 100644 --- a/src/NzbDrone.Core/Download/DownloadFailedEvent.cs +++ b/src/NzbDrone.Core/Download/DownloadFailedEvent.cs @@ -23,5 +23,6 @@ namespace NzbDrone.Core.Download public Dictionary Data { get; set; } public TrackedDownload TrackedDownload { get; set; } public Language Language { get; set; } + public bool SkipReDownload { get; set; } } } diff --git a/src/NzbDrone.Core/Download/FailedDownloadService.cs b/src/NzbDrone.Core/Download/FailedDownloadService.cs index bc2fec122..77aad8740 100644 --- a/src/NzbDrone.Core/Download/FailedDownloadService.cs +++ b/src/NzbDrone.Core/Download/FailedDownloadService.cs @@ -9,8 +9,8 @@ namespace NzbDrone.Core.Download { public interface IFailedDownloadService { - void MarkAsFailed(int historyId); - void MarkAsFailed(string downloadId); + void MarkAsFailed(int historyId, bool skipReDownload = false); + void MarkAsFailed(string downloadId, bool skipReDownload = false); void Process(TrackedDownload trackedDownload); } @@ -26,14 +26,14 @@ namespace NzbDrone.Core.Download _eventAggregator = eventAggregator; } - public void MarkAsFailed(int historyId) + public void MarkAsFailed(int historyId, bool skipReDownload = false) { var history = _historyService.Get(historyId); var downloadId = history.DownloadId; if (downloadId.IsNullOrWhiteSpace()) { - PublishDownloadFailedEvent(new List { history }, "Manually marked as failed"); + PublishDownloadFailedEvent(new List { history }, "Manually marked as failed", skipReDownload: skipReDownload); } else { @@ -42,13 +42,13 @@ namespace NzbDrone.Core.Download } } - public void MarkAsFailed(string downloadId) + public void MarkAsFailed(string downloadId, bool skipReDownload = false) { var history = _historyService.Find(downloadId, HistoryEventType.Grabbed); if (history.Any()) { - PublishDownloadFailedEvent(history, "Manually marked as failed"); + PublishDownloadFailedEvent(history, "Manually marked as failed", skipReDownload: skipReDownload); } } @@ -81,7 +81,7 @@ namespace NzbDrone.Core.Download } } - private void PublishDownloadFailedEvent(List historyItems, string message, TrackedDownload trackedDownload = null) + private void PublishDownloadFailedEvent(List historyItems, string message, TrackedDownload trackedDownload = null, bool skipReDownload = false) { var historyItem = historyItems.First(); @@ -96,7 +96,8 @@ namespace NzbDrone.Core.Download Message = message, Data = historyItem.Data, TrackedDownload = trackedDownload, - Language = historyItem.Language + Language = historyItem.Language, + SkipReDownload = skipReDownload }; _eventAggregator.PublishEvent(downloadFailedEvent); diff --git a/src/NzbDrone.Core/Download/RedownloadFailedDownloadService.cs b/src/NzbDrone.Core/Download/RedownloadFailedDownloadService.cs index 9519c1c96..ea3ce10d6 100644 --- a/src/NzbDrone.Core/Download/RedownloadFailedDownloadService.cs +++ b/src/NzbDrone.Core/Download/RedownloadFailedDownloadService.cs @@ -1,4 +1,4 @@ -using System.Linq; +using System.Linq; using NLog; using NzbDrone.Core.Configuration; using NzbDrone.Core.IndexerSearch; @@ -28,6 +28,12 @@ namespace NzbDrone.Core.Download public void HandleAsync(DownloadFailedEvent message) { + if (message.SkipReDownload) + { + _logger.Debug("Skip redownloading requested by user"); + return; + } + if (!_configService.AutoRedownloadFailed) { _logger.Debug("Auto redownloading failed albums is disabled");