Improve decision processing and deleting of pending releases

src/NzbDrone.Core.Test/Download/DownloadApprovedReportsTests/DownloadApprovedFixture.cs

@@ -6,6 +6,7 @@ using Moq;
 using NUnit.Framework;
 using NzbDrone.Core.DecisionEngine;
 using NzbDrone.Core.Download;
+using NzbDrone.Core.Download.Pending;
 using NzbDrone.Core.Parser.Model;
 using NzbDrone.Core.Profiles;
 using NzbDrone.Core.Qualities;
@@ -182,5 +183,47 @@ namespace NzbDrone.Core.Test.Download.DownloadApprovedReportsTests
 
             Subject.GetQualifiedReports(decisions).Should().BeEmpty();
         }
+
+        [Test]
+        public void should_not_grab_if_pending()
+        {
+            var episodes = new List<Episode> { GetEpisode(1) };
+            var remoteEpisode = GetRemoteEpisode(episodes, new QualityModel(Quality.HDTV720p));
+
+            var decisions = new List<DownloadDecision>();
+            decisions.Add(new DownloadDecision(remoteEpisode, new Rejection("Failure!", RejectionType.Temporary)));
+            decisions.Add(new DownloadDecision(remoteEpisode));
+
+            Subject.ProcessDecisions(decisions);
+            Mocker.GetMock<IDownloadService>().Verify(v => v.DownloadReport(It.IsAny<RemoteEpisode>()), Times.Never());
+        }
+
+        [Test]
+        public void should_not_add_to_pending_if_episode_was_grabbed()
+        {
+            var episodes = new List<Episode> { GetEpisode(1) };
+            var remoteEpisode = GetRemoteEpisode(episodes, new QualityModel(Quality.HDTV720p));
+
+            var decisions = new List<DownloadDecision>();
+            decisions.Add(new DownloadDecision(remoteEpisode));
+            decisions.Add(new DownloadDecision(remoteEpisode, new Rejection("Failure!", RejectionType.Temporary)));
+
+            Subject.ProcessDecisions(decisions);
+            Mocker.GetMock<IPendingReleaseService>().Verify(v => v.Add(It.IsAny<DownloadDecision>()), Times.Never());
+        }
+
+        [Test]
+        public void should_add_to_pending_even_if_already_added_to_pending()
+        {
+            var episodes = new List<Episode> { GetEpisode(1) };
+            var remoteEpisode = GetRemoteEpisode(episodes, new QualityModel(Quality.HDTV720p));
+
+            var decisions = new List<DownloadDecision>();
+            decisions.Add(new DownloadDecision(remoteEpisode, new Rejection("Failure!", RejectionType.Temporary)));
+            decisions.Add(new DownloadDecision(remoteEpisode, new Rejection("Failure!", RejectionType.Temporary)));
+
+            Subject.ProcessDecisions(decisions);
+            Mocker.GetMock<IPendingReleaseService>().Verify(v => v.Add(It.IsAny<DownloadDecision>()), Times.Exactly(2));
+        }
     }
 }

src/NzbDrone.Core.Test/Download/Pending/PendingReleaseServiceTests/AddFixture.cs

@@ -1,9 +1,6 @@
 using System;
 using System.Collections.Generic;
-using System.Linq;
-using System.Text;
 using FizzWare.NBuilder;
-using FluentAssertions;
 using Marr.Data;
 using Moq;
 using NUnit.Framework;
@@ -20,7 +17,7 @@ using NzbDrone.Test.Common;
 namespace NzbDrone.Core.Test.Download.Pending.PendingReleaseServiceTests
 {
     [TestFixture]
-    public class ProcessFixture : CoreTest<PendingReleaseService>
+    public class AddFixture : CoreTest<PendingReleaseService>
     {
         private DownloadDecision _temporarilyRejected;
         private Series _series;
@@ -140,6 +137,7 @@ namespace NzbDrone.Core.Test.Download.Pending.PendingReleaseServiceTests
 
             VerifyInsert();
         }
+
         [Test]
         public void should_add_if_publish_date_is_different()
         {

src/NzbDrone.Core.Test/Download/Pending/PendingReleaseServiceTests/RemoveGrabbedFixture.cs

@@ -0,0 +1,143 @@
+using System;
+using System.Collections.Generic;
+using FizzWare.NBuilder;
+using Marr.Data;
+using Moq;
+using NUnit.Framework;
+using NzbDrone.Core.DecisionEngine;
+using NzbDrone.Core.Download.Pending;
+using NzbDrone.Core.Parser;
+using NzbDrone.Core.Parser.Model;
+using NzbDrone.Core.Profiles;
+using NzbDrone.Core.Qualities;
+using NzbDrone.Core.Test.Framework;
+using NzbDrone.Core.Tv;
+using NzbDrone.Test.Common;
+
+namespace NzbDrone.Core.Test.Download.Pending.PendingReleaseServiceTests
+{
+    [TestFixture]
+    public class RemoveGrabbedFixture : CoreTest<PendingReleaseService>
+    {
+        private DownloadDecision _temporarilyRejected;
+        private Series _series;
+        private Episode _episode;
+        private Profile _profile;
+        private ReleaseInfo _release;
+        private ParsedEpisodeInfo _parsedEpisodeInfo;
+        private RemoteEpisode _remoteEpisode;
+
+        [SetUp]
+        public void Setup()
+        {
+            _series = Builder<Series>.CreateNew()
+                                     .Build();
+
+            _episode = Builder<Episode>.CreateNew()
+                                       .Build();
+
+            _profile = new Profile
+                       {
+                           Name = "Test",
+                           Cutoff = Quality.HDTV720p,
+                           GrabDelay = 1,
+                           Items = new List<ProfileQualityItem>
+                                   {
+                                       new ProfileQualityItem { Allowed = true, Quality = Quality.HDTV720p },
+                                       new ProfileQualityItem { Allowed = true, Quality = Quality.WEBDL720p },
+                                       new ProfileQualityItem { Allowed = true, Quality = Quality.Bluray720p }
+                                   },
+                       };
+
+            _series.Profile = new LazyLoaded<Profile>(_profile);
+
+            _release = Builder<ReleaseInfo>.CreateNew().Build();
+
+            _parsedEpisodeInfo = Builder<ParsedEpisodeInfo>.CreateNew().Build();
+            _parsedEpisodeInfo.Quality = new QualityModel(Quality.HDTV720p);
+
+            _remoteEpisode = new RemoteEpisode();
+            _remoteEpisode.Episodes = new List<Episode>{ _episode };
+            _remoteEpisode.Series = _series;
+            _remoteEpisode.ParsedEpisodeInfo = _parsedEpisodeInfo;
+            _remoteEpisode.Release = _release;
+            
+            _temporarilyRejected = new DownloadDecision(_remoteEpisode, new Rejection("Temp Rejected", RejectionType.Temporary));
+
+            Mocker.GetMock<IPendingReleaseRepository>()
+                  .Setup(s => s.All())
+                  .Returns(new List<PendingRelease>());
+
+            Mocker.GetMock<ISeriesService>()
+                  .Setup(s => s.GetSeries(It.IsAny<Int32>()))
+                  .Returns(_series);
+
+            Mocker.GetMock<IParsingService>()
+                  .Setup(s => s.GetEpisodes(It.IsAny<ParsedEpisodeInfo>(), _series, true, null))
+                  .Returns(new List<Episode> {_episode});
+
+            Mocker.GetMock<IPrioritizeDownloadDecision>()
+                  .Setup(s => s.PrioritizeDecisions(It.IsAny<List<DownloadDecision>>()))
+                  .Returns((List<DownloadDecision> d) => d);
+        }
+
+        private void GivenHeldRelease(QualityModel quality)
+        {
+            var parsedEpisodeInfo = _parsedEpisodeInfo.JsonClone();
+            parsedEpisodeInfo.Quality = quality;
+
+            var heldReleases = Builder<PendingRelease>.CreateListOfSize(1)
+                                                   .All()
+                                                   .With(h => h.SeriesId = _series.Id)
+                                                   .With(h => h.Release = _release.JsonClone())
+                                                   .With(h => h.ParsedEpisodeInfo = parsedEpisodeInfo)
+                                                   .Build();
+
+            Mocker.GetMock<IPendingReleaseRepository>()
+                  .Setup(s => s.All())
+                  .Returns(heldReleases);
+        }
+
+        [Test]
+        public void should_delete_if_the_grabbed_quality_is_the_same()
+        {
+            GivenHeldRelease(_parsedEpisodeInfo.Quality);
+
+            Subject.RemoveGrabbed(new List<DownloadDecision> { _temporarilyRejected });
+
+            VerifyDelete();
+        }
+
+        [Test]
+        public void should_delete_if_the_grabbed_quality_is_the_higher()
+        {
+            GivenHeldRelease(new QualityModel(Quality.SDTV));
+
+            Subject.RemoveGrabbed(new List<DownloadDecision> { _temporarilyRejected });
+
+            VerifyDelete();
+        }
+
+        [Test]
+        public void should_not_delete_if_the_grabbed_quality_is_the_lower()
+        {
+            GivenHeldRelease(new QualityModel(Quality.Bluray720p));
+
+            Subject.RemoveGrabbed(new List<DownloadDecision> { _temporarilyRejected });
+
+            VerifyNoDelete();
+        }
+
+        private void VerifyDelete()
+        {
+            Mocker.GetMock<IPendingReleaseRepository>()
+                .Verify(v => v.Delete(It.IsAny<PendingRelease>()), Times.Once());
+        }
+
+        private void VerifyNoDelete()
+        {
+            Mocker.GetMock<IPendingReleaseRepository>()
+                .Verify(v => v.Delete(It.IsAny<PendingRelease>()), Times.Never());
+        }
+    }
+}

src/NzbDrone.Core.Test/Download/Pending/PendingReleaseServiceTests/RemoveRejectedFixture.cs

@@ -0,0 +1,155 @@
+using System;
+using System.Collections.Generic;
+using FizzWare.NBuilder;
+using Marr.Data;
+using Moq;
+using NUnit.Framework;
+using NzbDrone.Core.DecisionEngine;
+using NzbDrone.Core.Download.Pending;
+using NzbDrone.Core.Parser;
+using NzbDrone.Core.Parser.Model;
+using NzbDrone.Core.Profiles;
+using NzbDrone.Core.Qualities;
+using NzbDrone.Core.Test.Framework;
+using NzbDrone.Core.Tv;
+using NzbDrone.Test.Common;
+
+namespace NzbDrone.Core.Test.Download.Pending.PendingReleaseServiceTests
+{
+    [TestFixture]
+    public class RemoveRejectedFixture : CoreTest<PendingReleaseService>
+    {
+        private DownloadDecision _temporarilyRejected;
+        private Series _series;
+        private Episode _episode;
+        private Profile _profile;
+        private ReleaseInfo _release;
+        private ParsedEpisodeInfo _parsedEpisodeInfo;
+        private RemoteEpisode _remoteEpisode;
+
+        [SetUp]
+        public void Setup()
+        {
+            _series = Builder<Series>.CreateNew()
+                                     .Build();
+
+            _episode = Builder<Episode>.CreateNew()
+                                       .Build();
+
+            _profile = new Profile
+                       {
+                           Name = "Test",
+                           Cutoff = Quality.HDTV720p,
+                           GrabDelay = 1,
+                           Items = new List<ProfileQualityItem>
+                                   {
+                                       new ProfileQualityItem { Allowed = true, Quality = Quality.HDTV720p },
+                                       new ProfileQualityItem { Allowed = true, Quality = Quality.WEBDL720p },
+                                       new ProfileQualityItem { Allowed = true, Quality = Quality.Bluray720p }
+                                   },
+                       };
+
+            _series.Profile = new LazyLoaded<Profile>(_profile);
+
+            _release = Builder<ReleaseInfo>.CreateNew().Build();
+
+            _parsedEpisodeInfo = Builder<ParsedEpisodeInfo>.CreateNew().Build();
+            _parsedEpisodeInfo.Quality = new QualityModel(Quality.HDTV720p);
+
+            _remoteEpisode = new RemoteEpisode();
+            _remoteEpisode.Episodes = new List<Episode>{ _episode };
+            _remoteEpisode.Series = _series;
+            _remoteEpisode.ParsedEpisodeInfo = _parsedEpisodeInfo;
+            _remoteEpisode.Release = _release;
+            
+            _temporarilyRejected = new DownloadDecision(_remoteEpisode, new Rejection("Temp Rejected", RejectionType.Temporary));
+
+            Mocker.GetMock<IPendingReleaseRepository>()
+                  .Setup(s => s.All())
+                  .Returns(new List<PendingRelease>());
+
+            Mocker.GetMock<ISeriesService>()
+                  .Setup(s => s.GetSeries(It.IsAny<Int32>()))
+                  .Returns(_series);
+
+            Mocker.GetMock<IParsingService>()
+                  .Setup(s => s.GetEpisodes(It.IsAny<ParsedEpisodeInfo>(), _series, true, null))
+                  .Returns(new List<Episode> {_episode});
+
+            Mocker.GetMock<IPrioritizeDownloadDecision>()
+                  .Setup(s => s.PrioritizeDecisions(It.IsAny<List<DownloadDecision>>()))
+                  .Returns((List<DownloadDecision> d) => d);
+        }
+
+        private void GivenHeldRelease(String title, String indexer, DateTime publishDate)
+        {
+            var release = _release.JsonClone();
+            release.Indexer = indexer;
+            release.PublishDate = publishDate;
+
+
+            var heldReleases = Builder<PendingRelease>.CreateListOfSize(1)
+                                                   .All()
+                                                   .With(h => h.SeriesId = _series.Id)
+                                                   .With(h => h.Title = title)
+                                                   .With(h => h.Release = release)
+                                                   .Build();
+
+            Mocker.GetMock<IPendingReleaseRepository>()
+                  .Setup(s => s.All())
+                  .Returns(heldReleases);
+        }
+
+        [Test]
+        public void should_remove_if_it_is_the_same_release_from_the_same_indexer()
+        {
+            GivenHeldRelease(_release.Title, _release.Indexer, _release.PublishDate);
+
+            Subject.RemoveRejected(new List<DownloadDecision> { _temporarilyRejected });
+
+            VerifyDelete();
+        }
+
+        [Test]
+        public void should_not_remove_if_title_is_different()
+        {
+            GivenHeldRelease(_release.Title + "-RP", _release.Indexer, _release.PublishDate);
+
+            Subject.RemoveRejected(new List<DownloadDecision> { _temporarilyRejected });
+
+            VerifyNoDelete();
+        }
+
+        [Test]
+        public void should_not_remove_if_indexer_is_different()
+        {
+            GivenHeldRelease(_release.Title, "AnotherIndexer", _release.PublishDate);
+
+            Subject.RemoveRejected(new List<DownloadDecision> { _temporarilyRejected });
+
+            VerifyNoDelete();
+        }
+
+        [Test]
+        public void should_not_remove_if_publish_date_is_different()
+        {
+            GivenHeldRelease(_release.Title, _release.Indexer, _release.PublishDate.AddHours(1));
+
+            Subject.RemoveRejected(new List<DownloadDecision> { _temporarilyRejected });
+
+            VerifyNoDelete();
+        }
+
+        private void VerifyDelete()
+        {
+            Mocker.GetMock<IPendingReleaseRepository>()
+                .Verify(v => v.Delete(It.IsAny<PendingRelease>()), Times.Once());
+        }
+
+        private void VerifyNoDelete()
+        {
+            Mocker.GetMock<IPendingReleaseRepository>()
+                .Verify(v => v.Delete(It.IsAny<PendingRelease>()), Times.Never());
+        }
+    }
+}

src/NzbDrone.Core.Test/NzbDrone.Core.Test.csproj

@@ -135,7 +135,9 @@
     <Compile Include="Download\DownloadClientTests\SabnzbdTests\SabnzbdFixture.cs" />
     <Compile Include="Download\DownloadServiceFixture.cs" />
     <Compile Include="Download\FailedDownloadServiceFixture.cs" />
-    <Compile Include="Download\Pending\PendingReleaseServiceTests\ProcessFixture.cs" />
+    <Compile Include="Download\Pending\PendingReleaseServiceTests\RemoveRejectedFixture.cs" />
+    <Compile Include="Download\Pending\PendingReleaseServiceTests\RemoveGrabbedFixture.cs" />
+    <Compile Include="Download\Pending\PendingReleaseServiceTests\AddFixture.cs" />
     <Compile Include="FluentTest.cs" />
     <Compile Include="Framework\CoreTest.cs" />
     <Compile Include="Framework\DbTest.cs" />

src/NzbDrone.Core/Download/Pending/PendingReleaseService.cs

@@ -2,6 +2,7 @@
 using System.Collections.Generic;
 using System.Linq;
 using NLog;
+using NzbDrone.Common;
 using NzbDrone.Core.DecisionEngine;
 using NzbDrone.Core.Messaging.Events;
 using NzbDrone.Core.Parser;
@@ -77,18 +78,33 @@ namespace NzbDrone.Core.Download.Pending
                 var decisionLocal = decision;
                 var episodeIds = decisionLocal.RemoteEpisode.Episodes.Select(e => e.Id);
 
-
                 var existingReports = alreadyPending.Where(r => r.RemoteEpisode.Episodes.Select(e => e.Id)
                                                                  .Intersect(episodeIds)
-                                                                 .Any());
+                                                                 .Any())
+                                                                 .ToList();
+
+                if (existingReports.Empty())
+                {
+                    continue;
+                }
+
+                var profile = decisionLocal.RemoteEpisode.Series.Profile.Value;
                 
                 foreach (var existingReport in existingReports)
+                {
+                    var compare = new QualityModelComparer(profile).Compare(decision.RemoteEpisode.ParsedEpisodeInfo.Quality,
+                                                                            existingReport.RemoteEpisode.ParsedEpisodeInfo.Quality);
+
+                    //Only remove lower/equal quality pending releases
+                    //It is safer to retry these releases on the next round than remove it and try to re-add it (if its still in the feed)
+                    if (compare >= 0)
                     {
                         _logger.Debug("Removing previously pending release, as it was grabbed.");
                         Delete(existingReport);
                     }
                 }
             }
+        }
 
         public void RemoveRejected(List<DownloadDecision> rejected)
         {

src/NzbDrone.Core/Download/ProcessDownloadDecisions.cs

@@ -35,14 +35,21 @@ namespace NzbDrone.Core.Download
         {
             var qualifiedReports = GetQualifiedReports(decisions);
             var prioritizedDecisions = _prioritizeDownloadDecision.PrioritizeDecisions(qualifiedReports);
-            var downloadedReports = new List<DownloadDecision>();
+            var grabbed = new List<DownloadDecision>();
-            var pendingReports = new List<DownloadDecision>();
+            var pending = new List<DownloadDecision>();
 
             foreach (var report in prioritizedDecisions)
             {
                 var remoteEpisode = report.RemoteEpisode;
 
-                if (DownloadingOrPending(downloadedReports, pendingReports, remoteEpisode))
+                var episodeIds = remoteEpisode.Episodes.Select(e => e.Id).ToList();
+
+                //Skip if already grabbed
+                if (grabbed.SelectMany(r => r.RemoteEpisode.Episodes)
+                               .Select(e => e.Id)
+                               .ToList()
+                               .Intersect(episodeIds)
+                               .Any())
                 {
                     continue;
                 }
@@ -50,54 +57,39 @@ namespace NzbDrone.Core.Download
                 if (report.TemporarilyRejected)
                 {
                     _pendingReleaseService.Add(report);
-                    pendingReports.Add(report);
+                    pending.Add(report);
                     continue;
                 }
 
-                try
-                {
-                    _downloadService.DownloadReport(remoteEpisode);
-                    downloadedReports.Add(report);
-                }
-                catch (Exception e)
-                {
-                    //TODO: support for store & forward
-                    _logger.WarnException("Couldn't add report to download queue. " + remoteEpisode, e);
-                }
-            }
-
-            return new ProcessedDecisions(downloadedReports, pendingReports);
-        }
-
-        internal List<DownloadDecision> GetQualifiedReports(IEnumerable<DownloadDecision> decisions)
-        {
-            //Process both approved and temporarily rejected
-            return decisions.Where(c => (c.Approved || c.TemporarilyRejected) && c.RemoteEpisode.Episodes.Any()).ToList();
-        }
-
-        private bool DownloadingOrPending(List<DownloadDecision> downloading, List<DownloadDecision> pending, RemoteEpisode remoteEpisode)
-        {
-            var episodeIds = remoteEpisode.Episodes.Select(e => e.Id).ToList();
-
-            if (downloading.SelectMany(r => r.RemoteEpisode.Episodes)
-                           .Select(e => e.Id)
-                           .ToList()
-                           .Intersect(episodeIds)
-                           .Any())
-            {
-                return true;
-            }
-
                 if (pending.SelectMany(r => r.RemoteEpisode.Episodes)
                        .Select(e => e.Id)
                        .ToList()
                        .Intersect(episodeIds)
                        .Any())
                 {
-                return true;
+                    continue;
                 }
 
-            return false;
+                try
+                {
+                    _downloadService.DownloadReport(remoteEpisode);
+                    grabbed.Add(report);
+                }
+                catch (Exception e)
+                {
+                    //TODO: support for store & forward
+                    //We'll need to differentiate between a download client error and an indexer error
+                    _logger.WarnException("Couldn't add report to download queue. " + remoteEpisode, e);
+                }
+            }
+
+            return new ProcessedDecisions(grabbed, pending);
+        }
+
+        internal List<DownloadDecision> GetQualifiedReports(IEnumerable<DownloadDecision> decisions)
+        {
+            //Process both approved and temporarily rejected
+            return decisions.Where(c => (c.Approved || c.TemporarilyRejected) && c.RemoteEpisode.Episodes.Any()).ToList();
         }
     }
 }