Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🎉 Source Bind Ads: add campaignlabels col #24223

Merged

Conversation

trowacat
Copy link
Contributor

@trowacat trowacat commented Mar 19, 2023

What

Adds the field CampaignLabels to the campaign_performance_report stream.
Closes: 18476

How

Adds CampaignLabels in report streams. This is only available in campaign performance report.

Pre-merge Checklist

Updating a connector

Community member or Airbyter

  • Grant edit access to maintainers (instructions)

  • Secrets in the connector's spec are annotated with airbyte_secret

  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.

  • Code reviews completed

  • Connector version has been incremented

  • Documentation updated

    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • If new credentials are required for use in CI, add them to GSM. Instructions.
  • /test connector=connectors/<name> command is passing
  • New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Unit tests

collected 23 items

unit_tests/test_client.py::test_sudsobject_todict_primitive_types PASSED
unit_tests/test_client.py::test_sudsobject_todict_nested PASSED
unit_tests/test_client.py::test_is_expired_true PASSED
unit_tests/test_client.py::test_is_expired_true_with_delta_threshold PASSED
unit_tests/test_client.py::test_is_expired_false PASSED
unit_tests/test_client.py::test_get_auth_client {"type": "LOG", "log": {"level": "INFO", "message": "Fetching access token ..."}}
PASSED
unit_tests/test_reports.py::test_get_column_value PASSED
unit_tests/test_reports.py::test_get_updated_state_new_state PASSED
unit_tests/test_reports.py::test_get_updated_state_state_unchanged PASSED
unit_tests/test_reports.py::test_get_updated_state_state_new_account PASSED
unit_tests/test_reports.py::test_get_report_record_timestamp_daily PASSED
unit_tests/test_reports.py::test_get_report_record_timestamp_without_aggregation PASSED
unit_tests/test_reports.py::test_get_report_record_timestamp_hourly PASSED
unit_tests/test_source.py::test_streams_config_based PASSED
unit_tests/test_source.py::test_source_check_connection_ok PASSED
unit_tests/test_source.py::test_source_check_connection_failed PASSED
unit_tests/test_source.py::test_campaigns_request_params PASSED
unit_tests/test_source.py::test_campaigns_stream_slices PASSED
unit_tests/test_source.py::test_adgroups_stream_slices PASSED
unit_tests/test_source.py::test_ads_request_params PASSED
unit_tests/test_source.py::test_ads_stream_slices PASSED
unit_tests/test_source.py::test_AccountPerformanceReportMonthly_request_params PASSED
unit_tests/test_source.py::test_accounts PASSED

================================================================================================================= warnings summary =================================================================================================================
...
========================================================================================================== 23 passed, 5 warnings in 4.65s ==========================================================================================================

@CLAassistant
Copy link

CLAassistant commented Mar 19, 2023

CLA assistant check
All committers have signed the CLA.

@marcosmarxm marcosmarxm added the contributor-program PRs submitted through the contributor program. label Mar 22, 2023
@marcosmarxm
Copy link
Member

marcosmarxm commented Mar 22, 2023

/test connector=connectors/source-bing-ads

🕑 connectors/source-bing-ads https://github.com/airbytehq/airbyte/actions/runs/4492115083
❌ connectors/source-bing-ads https://github.com/airbytehq/airbyte/actions/runs/4492115083
🐛

Build Failed

Test summary info:

Could not find result summary

@sajarin sajarin self-assigned this Mar 30, 2023
@sajarin
Copy link
Contributor

sajarin commented Apr 5, 2023

Hey thanks for the contribution and apologies for the delay!

I'll be the DRI responsible for reviewing this and getting it across the finish line. We currently have a backlog of about a dozen PRs, but just wanted to let you know that this PR has been added to the queue and I'll be trying my best to get to this as soon as possible.

Thanks for the contribution and for being patient :)

@sajarin
Copy link
Contributor

sajarin commented Apr 7, 2023

/test connector=connectors/source-bing-ads

🕑 connectors/source-bing-ads https://github.com/airbytehq/airbyte/actions/runs/4640147517
❌ connectors/source-bing-ads https://github.com/airbytehq/airbyte/actions/runs/4640147517
🐛

Build Failed

Test summary info:

Could not find result summary

@sajarin
Copy link
Contributor

sajarin commented Apr 10, 2023

/test connector=connectors/source-bing-ads

🕑 connectors/source-bing-ads https://github.com/airbytehq/airbyte/actions/runs/4660711928
✅ connectors/source-bing-ads https://github.com/airbytehq/airbyte/actions/runs/4660711928
Python tests coverage:

Name                          Stmts   Miss  Cover
-------------------------------------------------
source_bing_ads/__init__.py       2      0   100%
source_bing_ads/source.py       260     10    96%
source_bing_ads/cache.py         18      1    94%
source_bing_ads/reports.py      133     17    87%
source_bing_ads/client.py        93     21    77%
-------------------------------------------------
TOTAL                           506     49    90%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: SAT doesn't support complex nested states used in incremental report streams
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
================== 37 passed, 2 skipped in 486.67s (0:08:06) ===================

@sajarin sajarin requested a review from midavadim April 10, 2023 19:51
@sajarin
Copy link
Contributor

sajarin commented Apr 10, 2023

Thanks for the contribution @trowacat, trying to get this merged in!

@midavadim can you help review this PR since it is a GA connector?

@marcosmarxm marcosmarxm self-assigned this Apr 17, 2023
@@ -38,6 +38,9 @@
"CampaignStatus": {
"type": ["null", "string"]
},
"CampaignLabels": {
"type": ["null", "string"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please confirm that CampaignLabels is just a string, because name assumes that there are a lot of labels (probably list of strings? )

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trowacat can you confirm the above, please? thanks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From: https://learn.microsoft.com/en-us/advertising/reporting-service/campaignperformancereportcolumn?view=bingads-13#campaignlabels

CampaignLabels
The labels applied to the campaign.Labels are delimited by a semicolon (;) in the report download.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trowacat did you have time to take a look on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@midavadim I added some labels to our integration account data
image
The data field is correct.

@@ -38,6 +38,9 @@
"CampaignStatus": {
"type": ["null", "string"]
},
"CampaignLabels": {
"type": ["null", "string"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@midavadim I added some labels to our integration account data
image
The data field is correct.

@marcosmarxm
Copy link
Member

marcosmarxm commented May 8, 2023

/test connector=connectors/source-bing-ads

🕑 connectors/source-bing-ads https://github.com/airbytehq/airbyte/actions/runs/4920101026
✅ connectors/source-bing-ads https://github.com/airbytehq/airbyte/actions/runs/4920101026
Python tests coverage:

Name                          Stmts   Miss  Cover
-------------------------------------------------
source_bing_ads/__init__.py       2      0   100%
source_bing_ads/source.py       260     10    96%
source_bing_ads/cache.py         18      1    94%
source_bing_ads/reports.py      142     16    89%
source_bing_ads/client.py        93     21    77%
-------------------------------------------------
TOTAL                           515     48    91%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: SAT doesn't support complex nested states used in incremental report streams
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
================== 37 passed, 2 skipped in 542.03s (0:09:02) ===================

@marcosmarxm marcosmarxm requested a review from midavadim May 8, 2023 22:50
@marcosmarxm
Copy link
Member

marcosmarxm commented May 9, 2023

/publish connector=connectors/source-bing-ads

🕑 Publishing the following connectors:
connectors/source-bing-ads
https://github.com/airbytehq/airbyte/actions/runs/4928637142


Connector Version Did it publish? Were definitions generated?
connectors/source-bing-ads 0.1.22

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@marcosmarxm marcosmarxm changed the title Source bing ads add campaignlabels col 🎉 Source Bind Ads: add campaignlabels col May 9, 2023
@marcosmarxm marcosmarxm merged commit 4806690 into airbytehq:master May 9, 2023
43 of 54 checks passed
marcosmarxm added a commit to natalia-miinto/airbyte that referenced this pull request Jun 8, 2023
* campaignLabels col

* update pr number

* updated expected_records.txt with campaign labels in campaign performance streams

* fix expected records

* bump metadata version

* auto-bump connector version

---------

Co-authored-by: Sajarin <sajarindider@gmail.com>
Co-authored-by: marcosmarxm <marcosmarxm@gmail.com>
Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation bounty community connectors/source/bing-ads contributor-program PRs submitted through the contributor program. ga-connector-change gl-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source Bing Ads - Add CampaignLabels report column in campaign performance report
7 participants