feat: NeoWorlder integration to production#287
Conversation
🤖 Augment PR SummarySummary: This PR promotes the NeoWorlder deliverer toward production usage and adds campaign-routing metadata to the exported lead CSVs. Changes:
Technical Notes: CSV augmentation currently round-trips through 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
preritdas
left a comment
preritdas
left a comment
There was a problem hiding this comment.
Looks good overall, specific comments throughout the implementation and test file.
| STAGING_BASE_URL = "https://public-api.staging.neoworlder.com" | ||
| # PRODUCTION_BASE_URL = "https://public-api.neoworlder.com" # Update when available | ||
| PRODUCTION_BASE_URL = "https://public-api.neoworlder.com" |
There was a problem hiding this comment.
Let's keep this out of the SDK, they should be supplied in keys.yaml from real-intent-deliveries.
| PRODUCTION_BASE_URL = "https://public-api.neoworlder.com" | ||
|
|
||
| # Valid campaign types for NeoWorlder persona routing | ||
| VALID_CAMPAIGN_TYPES = ("seller", "buyer") |
There was a problem hiding this comment.
Let's use a StrEnum for this, and reference that datatype below in the initializer. Serves both purposes of data validation and documenting available values.
| if csv_string: | ||
| reader = csv.reader(io.StringIO(csv_string)) | ||
| rows = list(reader) | ||
|
|
||
| # Append campaign columns to header and data rows | ||
| buyer_val = "BUYER" if self.campaign_type == "buyer" else "" | ||
| recovery_val = "YES" if self.is_recovery else "" | ||
| sms_val = "YES" if self.sms_optin else "" | ||
|
|
||
| rows[0].extend(["BUYER", "RECOVERY", "SMS_OPTIN"]) | ||
| for row in rows[1:]: | ||
| row.extend([buyer_val, recovery_val, sms_val]) | ||
|
|
||
| output = io.StringIO() | ||
| writer = csv.writer(output) | ||
| writer.writerows(rows) | ||
| csv_string = output.getvalue() |
There was a problem hiding this comment.
In a situation like this I think it makes more sense to use the pattern:
if not csv_string:
# handle edge case
# main logic unindented
tests/test_neoworlder.py
Outdated
| def test_deliverer_production_url(): | ||
| """Test that the production URL constant is available.""" | ||
| assert NeoworlderDeliverer.PRODUCTION_BASE_URL == "https://public-api.neoworlder.com" |
There was a problem hiding this comment.
In accordance with prior comment about removing URLs from SDK, would remove this test entirely.
tests/test_neoworlder.py
Outdated
| import pandas as pd | ||
| from io import StringIO |
There was a problem hiding this comment.
Would put these at the root of the test module, not within the function, to fail-fast on test collection vs. test execution. Imports within functions are useful for conditional imports, but io is stdlib and pandas is required by the package at large (I'm pretty sure).
There was a problem hiding this comment.
This would apply to all instances of imports within functions.
…ard-labs/real-intent into taran/neoworlder-deliverer
2 participants
No description provided.