Merge pull request #40 from jcass77/develop

Resolving ad playback issues

8 files changed, 94 insertions, 45 deletions

diff --git a/pandora/client.py b/pandora/client.py
index fafd872..403216e 100644
--- a/pandora/client.py
+++ b/pandora/client.py
@@ -219,9 +219,10 @@ class APIClient(BaseAPIClient):
         from .models.pandora import GenreStationList
 
         genres = self("station.getGenreStations")
-        genres["checksum"] = self.get_genre_stations_checksum()
 
-        return GenreStationList.from_json(self, genres)
+        genre_stations = GenreStationList.from_json(self, genres)
+        genre_stations.checksum = self.get_genre_stations_checksum()
+        return genre_stations
 
     def get_genre_stations_checksum(self):
         return self("station.getGenreStationsChecksum")["checksum"]
@@ -262,13 +263,14 @@ class APIClient(BaseAPIClient):
         from .models.pandora import AdItem
 
         if not station_id:
-            raise ValueError("The 'station_id' param must be defined, "
-                             "got: '{}'".format(station_id))
-
-        ad_metadata = self.get_ad_metadata(ad_token)
-        ad_metadata["stationId"] = station_id
-
-        return AdItem.from_json(self, ad_metadata)
+            raise errors.ParameterMissing("The 'station_id' param must be "
+                                          "defined, got: '{}'"
+                                          .format(station_id))
+
+        ad_item = AdItem.from_json(self, self.get_ad_metadata(ad_token))
+        ad_item.station_id = station_id
+        ad_item.ad_token = ad_token
+        return ad_item
 
     def get_ad_metadata(self, ad_token):
         return self("ad.getAdMetadata",
diff --git a/pandora/models/pandora.py b/pandora/models/pandora.py
index 40b368a..7c134e1 100644
--- a/pandora/models/pandora.py
+++ b/pandora/models/pandora.py
@@ -1,6 +1,7 @@
 from .. import BaseAPIClient
 from . import with_metaclass, ModelMetaClass
 from . import Field, PandoraModel, PandoraListModel, PandoraDictListModel
+from ..errors import ParameterMissing
 
 
 class Station(PandoraModel):
@@ -130,6 +131,8 @@ class PlaylistModel(PandoraModel):
         return cls.get_audio_field(data, "bitrate", preferred_quality)
 
     def get_is_playable(self):
+        if not self.audio_url:
+            return False
         return self._api_client.transport.test_url(self.audio_url)
 
     def prepare_playback(self):
@@ -217,17 +220,33 @@ class AdItem(PlaylistModel):
     audio_url = Field("audioUrl")
     image_url = Field("imageUrl")
     click_through_url = Field("clickThroughUrl")
-    station_id = Field("stationId")
+    station_id = None
+    ad_token = None
 
     @property
     def is_ad(self):
         return True
 
-    def register_ad(self, station_id):
-        self._api_client.register_ad(station_id, self.tracking_tokens)
+    def register_ad(self, station_id=None):
+        if not station_id:
+            station_id = self.station_id
+        if self.tracking_tokens:
+            self._api_client.register_ad(station_id, self.tracking_tokens)
+        else:
+            raise ParameterMissing('No ad tracking tokens provided for '
+                                   'registration.')
 
     def prepare_playback(self):
-        self.register_ad(self.station_id)
+        try:
+            self.register_ad(self.station_id)
+        except ParameterMissing as e:
+            if not self.tracking_tokens:
+                # Ignore registration attempts if no ad tracking tokens are
+                # available
+                pass
+            else:
+                # Unexpected error, re-raise.
+                raise e
         return super(AdItem, self).prepare_playback()
 
 
diff --git a/pandora/transport.py b/pandora/transport.py
index 1a2ee69..8fab7af 100644
--- a/pandora/transport.py
+++ b/pandora/transport.py
@@ -46,7 +46,11 @@ def retries(max_tries, exceptions=(Exception,)):
                     retries_left -= 1
                     return func(*args, **kwargs)
 
-                except exceptions:
+                except exceptions as e:
+                    # Don't retry for PandoraExceptions - unlikely that result
+                    # will change for same set of input parameters.
+                    if isinstance(e, PandoraException):
+                        continue
                     if retries_left > 0:
                         time.sleep(delay_exponential(
                             0.5, 2, max_tries - retries_left))
diff --git a/pydora/utils.py b/pydora/utils.py
index 756810a..275c386 100644
--- a/pydora/utils.py
+++ b/pydora/utils.py
@@ -125,15 +125,6 @@ def iterate_forever(func, *args, **kwargs):
             yield playlistItem
         except StopIteration:
             output = func(*args, **kwargs)
-        except errors.ParameterMissing as e:
-            if isinstance(playlistItem, AdItem):
-                if (not playlistItem.tracking_tokens or
-                        len(playlistItem.tracking_tokens) == 0):
-                    # Ad item does not contain any tracking tokens, yield
-                    # without registering
-                    yield playlistItem
-            # Something else went wrong, re-raise
-            raise e
 
 
 class SilentPopen(subprocess.Popen):
diff --git a/tests/test_pandora/test_client.py b/tests/test_pandora/test_client.py
index 24d9734..cf486e0 100644
--- a/tests/test_pandora/test_client.py
+++ b/tests/test_pandora/test_client.py
@@ -1,7 +1,7 @@
 from unittest import TestCase
 
 from pandora.client import APIClient, BaseAPIClient
-from pandora.errors import InvalidAuthToken
+from pandora.errors import InvalidAuthToken, ParameterMissing
 from pandora.py2compat import Mock, call, patch
 from tests.test_pandora.test_models import TestAdItem
 
@@ -64,12 +64,12 @@ class TestCallingAPIClient(TestCase):
             client = APIClient(transport, None, None, None, None)
             client._authenticate = Mock()
 
-            client.get_playlist('mock_token')
+            client.get_playlist('token_mock')
 
             playlist_mock.assert_has_calls([call("station.getPlaylist",
                                              audioAdPodCapable=True,
                                              includeTrackLength=True,
-                                             stationToken='mock_token',
+                                             stationToken='token_mock',
                                              xplatformAdCapable=True)])
 
 
@@ -103,11 +103,12 @@ class TestGettingAds(TestCase):
             client = APIClient(transport, None, None, None, None)
             client._authenticate = Mock()
 
-            ad_item = client.get_ad_item('mock_id', 'mock_token')
-            assert ad_item.station_id == 'mock_id'
+            ad_item = client.get_ad_item('id_mock', 'token_mock')
+            assert ad_item.station_id == 'id_mock'
+            assert ad_item.ad_token == 'token_mock'
 
             ad_metadata_mock.assert_has_calls([call("ad.getAdMetadata",
-                                                      adToken='mock_token',
+                                                      adToken='token_mock',
                                                       returnAdTrackingTokens=True,
                                                       supportAudioAds=True)])
 
@@ -117,4 +118,4 @@ class TestGettingAds(TestCase):
         client = APIClient(transport, None, None, None, None)
         client.get_ad_metadata = Mock()
 
-        self.assertRaises(ValueError, client.get_ad_item, '', 'mock_token')
+        self.assertRaises(ParameterMissing, client.get_ad_item, '', 'token_mock')
diff --git a/tests/test_pandora/test_models.py b/tests/test_pandora/test_models.py
index ecd8cf1..ec6e9d5 100644
--- a/tests/test_pandora/test_models.py
+++ b/tests/test_pandora/test_models.py
@@ -3,6 +3,7 @@ from datetime import datetime
 from pandora.py2compat import Mock, patch
 from pandora import APIClient
 from pandora.models.pandora import AdItem, PlaylistModel
+from pandora.errors import ParameterMissing
 
 import pandora.models as m
 
@@ -205,35 +206,46 @@ class TestAdItem(TestCase):
     JSON_DATA = {
         'audioUrlMap': {
             'mediumQuality': {
-                'audioUrl': 'mock_med_url', 'bitrate': '64', 'protocol': 'http', 'encoding': 'aacplus'
+                'audioUrl': 'med_url_mock', 'bitrate': '64', 'protocol': 'http', 'encoding': 'aacplus'
             },
             'highQuality': {
-                'audioUrl': 'mock_high_url', 'bitrate': '64', 'protocol': 'http', 'encoding': 'aacplus'
+                'audioUrl': 'high_url_mock', 'bitrate': '64', 'protocol': 'http', 'encoding': 'aacplus'
             },
             'lowQuality': {
-                'audioUrl': 'mock_low_url', 'bitrate': '32', 'protocol': 'http', 'encoding': 'aacplus'}},
-            'clickThroughUrl': 'mock_click_url',
-            'imageUrl': 'mock_img_url',
+                'audioUrl': 'low_url_mock', 'bitrate': '32', 'protocol': 'http', 'encoding': 'aacplus'}},
+            'clickThroughUrl': 'click_url_mock',
+            'imageUrl': 'img_url_mock',
             'companyName': '',
             'title': '',
             'trackGain': '0.0',
-            'adTrackingTokens': ['mock_token_1', 'mock_token_2']
+            'adTrackingTokens': ['token_1_mock', 'token_2_mock']
     }
 
     def setUp(self):
         api_client_mock = Mock(spec=APIClient)
         api_client_mock.default_audio_quality = APIClient.HIGH_AUDIO_QUALITY
         self.result = AdItem.from_json(api_client_mock, self.JSON_DATA)
+        self.result.station_id = 'station_id_mock'
+        self.result.ad_token = 'token_mock'
 
     def test_is_ad_is_true(self):
         assert self.result.is_ad is True
 
     def test_register_ad(self):
         self.result._api_client.register_ad = Mock()
-        self.result.register_ad('id_dummy')
+        self.result.register_ad('id_mock')
 
         assert self.result._api_client.register_ad.called
 
+    def test_register_ad_raises_exception_if_no_tracking_tokens_available(self):
+        with self.assertRaises(ParameterMissing):
+            self.result.tracking_tokens = []
+            self.result._api_client.register_ad = Mock(spec=AdItem)
+
+            self.result.register_ad('id_mock')
+
+            assert self.result._api_client.register_ad.called
+
     def test_prepare_playback(self):
         with patch.object(PlaylistModel, 'prepare_playback') as super_mock:
 
@@ -241,3 +253,23 @@ class TestAdItem(TestCase):
             self.result.prepare_playback()
             assert self.result.register_ad.called
             assert super_mock.called
+
+    def test_prepare_playback_raises_paramater_missing(self):
+        with patch.object(PlaylistModel, 'prepare_playback') as super_mock:
+
+            self.result.register_ad = Mock(side_effect=ParameterMissing('No ad tracking tokens provided for '
+                                                                        'registration.')
+                                           )
+            self.assertRaises(ParameterMissing, self.result.prepare_playback)
+            assert self.result.register_ad.called
+            assert not super_mock.called
+
+    def test_prepare_playback_handles_paramater_missing_if_no_tokens(self):
+        with patch.object(PlaylistModel, 'prepare_playback') as super_mock:
+
+            self.result.tracking_tokens = []
+            self.result.register_ad = Mock(side_effect=ParameterMissing('No ad tracking tokens provided for '
+                                                                        'registration.'))
+            self.result.prepare_playback()
+            assert self.result.register_ad.called
+            assert super_mock.called
diff --git a/tests/test_pandora/test_transport.py b/tests/test_pandora/test_transport.py
index 59d4cea..6596a0e 100644
--- a/tests/test_pandora/test_transport.py
+++ b/tests/test_pandora/test_transport.py
@@ -18,7 +18,7 @@ class TestTransport(TestCase):
 
             time.sleep = Mock()
             client.transport._make_http_request = Mock(
-                    side_effect=SysCallError("mock_error"))
+                    side_effect=SysCallError("error_mock"))
             client.transport._start_request = Mock()
 
             client("method")
diff --git a/tests/test_pydora/test_utils.py b/tests/test_pydora/test_utils.py
index 64cdf74..ae6fa0b 100644
--- a/tests/test_pydora/test_utils.py
+++ b/tests/test_pydora/test_utils.py
@@ -18,22 +18,22 @@ class TestIterateForever(TestCase):
         with patch.object(APIClient, 'get_playlist') as get_playlist_mock:
             with patch.object(APIClient, 'register_ad', side_effect=ParameterMissing("ParameterMissing")):
 
-                station = Station.from_json(self.client, {'stationToken': 'dummy_token'})
-                dummy_ad = AdItem.from_json(self.client, {'station_id': 'dummy_id'})
-                get_playlist_mock.return_value=iter([dummy_ad])
+                station = Station.from_json(self.client, {'stationToken': 'token_mock'})
+                ad_mock = AdItem.from_json(self.client, {'station_id': 'id_mock'})
+                get_playlist_mock.return_value=iter([ad_mock])
 
                 station_iter = iterate_forever(station.get_playlist)
 
                 next_track = station_iter.next()
-                self.assertEqual(dummy_ad, next_track)
+                self.assertEqual(ad_mock, next_track)
 
     def test_reraise_missing_params_exception(self):
         with patch.object(APIClient, 'get_playlist', side_effect=ParameterMissing("ParameterMissing")) as get_playlist_mock:
                 with self.assertRaises(ParameterMissing):
 
-                    station = Station.from_json(self.client, {'stationToken': 'dummy_token'})
-                    dummy_track = PlaylistItem.from_json(self.client, {'token': 'dummy_token'})
-                    get_playlist_mock.return_value=iter([dummy_track])
+                    station = Station.from_json(self.client, {'stationToken': 'token_mock'})
+                    track_mock = PlaylistItem.from_json(self.client, {'token': 'token_mock'})
+                    get_playlist_mock.return_value=iter([track_mock])
 
                     station_iter = iterate_forever(station.get_playlist)
                     station_iter.next()