Added review comment changes

Author: akkumar-qlik

CHANGELOG.md

@@ -1,5 +1,5 @@
 ## 1.3.0
-- Streams the credentials cannot access (HTTP 401/403) are excluded from the catalog during discovery (discovery still fails if no parent streams are accessible). [#17](https://github.com/singer-io/tap-campaign-monitor/pull/17)
+- Streams the credentials cannot access (HTTP 403) are excluded from the catalog during discovery; invalid credentials (HTTP 401) fail discovery immediately, as does having no accessible parent stream. [#17](https://github.com/singer-io/tap-campaign-monitor/pull/17)
 - Bump to requests==2.34.2
 
 ## 1.2.0

tap_campaign_monitor/__init__.py

@@ -16,10 +16,10 @@
 LOGGER = singer.get_logger()  # noqa
 
 
-def verify_credentials(config):
+def verify_credentials(config, load_timezone=True):
     LOGGER.info("Verifying credentials.")
     try:
-        client = CampaignMonitorClient(config)
+        client = CampaignMonitorClient(config, load_timezone=load_timezone)
         LOGGER.info("Credentials verified successfully.")
         return client
     except Exception as e:
@@ -112,7 +112,9 @@ def main():
     args = singer.utils.parse_args(
         required_config_keys=['client_id', 'refresh_token'])
 
-    client = verify_credentials(args.config)
+    client = verify_credentials(
+        args.config, load_timezone=not args.discover
+    )
 
     if args.discover:
         do_discover(args, client)

tap_campaign_monitor/client.py

@@ -34,25 +34,49 @@ class CampaignMonitorForbiddenError(Exception):
 
 class CampaignMonitorClient:
 
-    def __init__(self, config):
+    def __init__(self, config, load_timezone=True):
         self.config = config
         self._retry_after = RETRY_RATE_LIMIT
         self.access_token = self.refresh_access_token()
-        self.timezone = self.get_timezone()
-        LOGGER.info("Client timezone is {}".format(self.timezone))
+        self.timezone = self.get_timezone() if load_timezone else None
+        if load_timezone:
+            LOGGER.info("Client timezone is {}".format(self.timezone))
 
     def refresh_access_token(self):
         LOGGER.info("Refreshing access token")
         url = "https://api.createsend.com/oauth/token"
         data = {'grant_type': 'refresh_token', 'refresh_token': self.config['refresh_token']}
         response = requests.request("POST", url, data=data)
         payload = response.json()
-        if 'access_token' not in payload:
+        oauth_error = payload.get('error')
+        if response.status_code == 401 or (
+                response.status_code == 400
+                and oauth_error in {'invalid_client', 'invalid_grant',
+                                    'unauthorized_client'}):
             raise CampaignMonitorUnauthorizedError(
                 "HTTP-error-code: 401, Error: Invalid credentials: {}".format(
                     payload.get('error_description') or payload.get('error') or response.text
                 )
             )
+        if response.status_code == 429:
+            raise Server429Error(
+                "HTTP-error-code: 429, Error: Rate limit exceeded. {}"
+                .format(response.text)
+            )
+        if 500 <= response.status_code < 600:
+            raise Server5xxError(
+                "HTTP-error-code: {}, Error: {}"
+                .format(response.status_code, response.text)
+            )
+        if response.status_code != 200:
+            raise RuntimeError(
+                "HTTP-error-code: {}, Error: {}"
+                .format(response.status_code, response.text)
+            )
+        if 'access_token' not in payload:
+            raise RuntimeError(
+                "Invalid token response: access_token is missing."
+            )
         return payload['access_token']
 
     def get_timezone(self):

tests/unittests/test_client.py

@@ -29,6 +29,19 @@ def test_init_calls_refresh_and_timezone(self, mock_refresh, mock_tz):
         mock_refresh.assert_called_once()
         mock_tz.assert_called_once()
 
+    @patch('tap_campaign_monitor.client.CampaignMonitorClient.get_timezone')
+    @patch('tap_campaign_monitor.client.CampaignMonitorClient.refresh_access_token')
+    def test_init_can_skip_timezone(self, mock_refresh, mock_tz):
+        """Discovery can authenticate without requesting client details."""
+        from tap_campaign_monitor.client import CampaignMonitorClient
+        config = {'client_id': 'test_id', 'refresh_token': 'test_token'}
+
+        client = CampaignMonitorClient(config, load_timezone=False)
+
+        mock_refresh.assert_called_once()
+        mock_tz.assert_not_called()
+        self.assertIsNone(client.timezone)
+
 
 class TestRefreshAccessToken(unittest.TestCase):
     """Test the refresh_access_token method."""
@@ -48,6 +61,36 @@ def test_refresh_access_token_missing_token_raises_unauthorized(self, mock_reque
         self.assertIn('401', str(ctx.exception))
         self.assertIn('Refresh token revoked', str(ctx.exception))
 
+    @parameterized.expand([
+        (429, 'rate limited', 'Server429Error'),
+        (500, 'server error', 'Server5xxError'),
+    ])
+    @patch('tap_campaign_monitor.client.requests.request')
+    def test_refresh_access_token_preserves_transient_error(
+            self, status_code, response_text, exception_name, mock_request):
+        """Token-service transient failures are not reported as invalid credentials."""
+        from tap_campaign_monitor import client as client_module
+        from tap_campaign_monitor.client import CampaignMonitorClient
+        mock_request.return_value = MockResponse(
+            status_code, {'error': 'server_error'}, response_text
+        )
+
+        with self.assertRaises(getattr(client_module, exception_name)):
+            CampaignMonitorClient(
+                {'client_id': 'test_id', 'refresh_token': 'test_token'}
+            )
+
+    @patch('tap_campaign_monitor.client.requests.request')
+    def test_refresh_access_token_rejects_malformed_success(self, mock_request):
+        """A successful response without a token is a response error, not a 401."""
+        from tap_campaign_monitor.client import CampaignMonitorClient
+        mock_request.return_value = MockResponse(200, {'unexpected': 'value'})
+
+        with self.assertRaisesRegex(RuntimeError, 'access_token is missing'):
+            CampaignMonitorClient(
+                {'client_id': 'test_id', 'refresh_token': 'test_token'}
+            )
+
     @patch('tap_campaign_monitor.client.CampaignMonitorClient.get_timezone')
     @patch('tap_campaign_monitor.client.requests.request')
     def test_refresh_access_token_success(self, mock_request, mock_tz):

tests/unittests/test_discovery.py

@@ -29,6 +29,22 @@ def _mock_client():
     return client
 
 
+class TestVerifyCredentials(unittest.TestCase):
+    """Unit tests for credential verification setup."""
+
+    @patch('tap_campaign_monitor.CampaignMonitorClient')
+    def test_can_skip_timezone_for_discovery(self, mock_client_class):
+        """Discovery authentication does not request client timezone details."""
+        from tap_campaign_monitor import verify_credentials
+
+        client = verify_credentials(CONFIG, load_timezone=False)
+
+        mock_client_class.assert_called_once_with(
+            CONFIG, load_timezone=False
+        )
+        self.assertIs(client, mock_client_class.return_value)
+
+
 class TestCheckAccess(unittest.TestCase):
     """Unit tests for BaseStream.check_access()."""