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

[STRATCONN-5603] - Format timestamp fields without miliiseconds #2816

Merged
merged 5 commits into from
Mar 25, 2025

Conversation

varadarajan-tw
Copy link
Contributor

@varadarajan-tw varadarajan-tw commented Mar 20, 2025

This pull request updates timestamp conversion function to handle timestamps without milliseconds part.

Previously timestamp like 2025-03-11T17:57:29Z was passed as is to google ads API. Google ads API expects timezone specified. Since a timestamp with Z indicates UTC, as part of this change, we simply replace Z with +00.

Adding unit test cases for timestamp formats with and without milliseconds

JIRA

Testing

Without milliseconds
image

With milliseconds

image

  • Added unit tests for new functionality
  • Tested end-to-end using the local server
  • [If destination is already live] Tested for backward compatibility of destination. Note: New required fields are a breaking change.
  • [Segmenters] Tested in the staging environment
  • [Segmenters] [If applicable for this change] Tested for regression with Hadron.

Copy link

codecov bot commented Mar 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.96%. Comparing base (61eb683) to head (dcffd66).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2816      +/-   ##
==========================================
- Coverage   78.54%   77.96%   -0.59%     
==========================================
  Files        1095     1042      -53     
  Lines       21775    19007    -2768     
  Branches     4467     3704     -763     
==========================================
- Hits        17104    14818    -2286     
+ Misses       3362     2886     -476     
+ Partials     1309     1303       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@varadarajan-tw varadarajan-tw requested a review from Copilot March 20, 2025 09:21
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the timestamp conversion logic to remove milliseconds and adjusts error handling by explicitly typing error objects as GoogleAdsError. The changes include:

  • Refining the convertTimestamp function to replace both the 'Z' suffix and milliseconds.
  • Introducing explicit type casting for errors passed to handleGoogleAdsError in several functions.
  • Adding unit tests to validate convertTimestamp functionality.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
packages/destination-actions/src/destinations/google-enhanced-conversions/functions.ts Enhancements to timestamp formatting and error handling type definitions.
packages/destination-actions/src/destinations/google-enhanced-conversions/tests/functions.test.ts Added tests to cover the updated convertTimestamp function.

@varadarajan-tw varadarajan-tw marked this pull request as ready for review March 21, 2025 05:59
@varadarajan-tw varadarajan-tw requested a review from a team as a code owner March 21, 2025 05:59
@varadarajan-tw varadarajan-tw requested a review from Copilot March 21, 2025 09:14

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the timestamp conversion function to strip out milliseconds and enforce a standard timezone offset format while adding tests to verify the new behavior.

  • Updated the convertTimestamp function to replace the trailing "Z" with a timezone offset.
  • Added new unit tests covering timestamp conversion with and without milliseconds.
  • Updated import statements in tests to include the new function.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
packages/destination-actions/src/destinations/google-enhanced-conversions/functions.ts Updated timestamp conversion logic for proper formatting without milliseconds
packages/destination-actions/src/destinations/google-enhanced-conversions/tests/functions.test.ts Added test cases to verify convertTimestamp functionality
@varadarajan-tw varadarajan-tw requested a review from Copilot March 24, 2025 08:09

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the timestamp conversion function to format timestamps without milliseconds by replacing the trailing "Z" with "+00:00", and refactors the way user address identifiers are constructed.

  • Updated convertTimestamp to support timestamps with and without milliseconds.
  • Refactored the construction of the addressInfo object to inline the processing of name and address fields.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
packages/destination-actions/src/destinations/google-enhanced-conversions/functions.ts Updates timestamp conversion regex and refactors construction of address identifiers.
packages/destination-actions/src/destinations/google-enhanced-conversions/tests/functions.test.ts Adds unit tests for the updated convertTimestamp function.
Comments suppressed due to low confidence (2)

packages/destination-actions/src/destinations/google-enhanced-conversions/functions.ts:486

  • Using the nullish coalescing operator to hash an empty string when first_name is missing may produce an unintended hash value. Consider conditionally adding the hashedFirstName property only when payload.first_name is provided.
hashedFirstName: processHashing(payload.first_name ?? '',

packages/destination-actions/src/destinations/google-enhanced-conversions/functions.ts:494

  • Using the nullish coalescing operator to hash an empty string when last_name is missing may produce an unintended hash value. Consider conditionally adding the hashedLastName property only when payload.last_name is provided.
hashedLastName: processHashing(payload.last_name ?? '',
sayan-das-in
sayan-das-in previously approved these changes Mar 24, 2025
@sayan-das-in sayan-das-in self-requested a review March 24, 2025 08:40
@varadarajan-tw varadarajan-tw merged commit dc67be8 into main Mar 25, 2025
13 of 14 checks passed
@varadarajan-tw varadarajan-tw deleted the gec-timestamp-fix branch March 25, 2025 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants