[dorasun]

Change-Id: I57fa8a776fb8a1cbd43f6c1d3a1a71c3c63bdd19

[bobhancockg]

Code Vetting & Review Recommendations

Overall, the implementation for adding incentive examples (apply_incentive.rb and fetch_incentive.rb) is well-structured. Below are recommended improvements to align with google-ads-ruby repository standards, fix documentation indexing tags, ensure type safety, and standardize monetary formatting.

---

1. examples/incentives/apply_incentive.rb

• Default Parameter Value: Add country_code = nil to the method signature so calling apply_incentive(customer_id, incentive_id) programmatically works without explicitly passing nil:

  def apply_incentive(customer_id, incentive_id, country_code = nil)

• Type Safety for selected_incentive_id: Explicitly call .to_i on incentive_id when constructing request_args for protobuf field type safety (int64):

  request_args = {
    customer_id: customer_id,
    selected_incentive_id: incentive_id.to_i
  }

• CLI Default Initializers & Validation: Initialize options[:incentive_id] = 'INSERT_INCENTIVE_ID_HERE' at the top of if __FILE__ == $0 to match the doc comments, and update argument validation accordingly:

  options[:customer_id] = 'INSERT_CUSTOMER_ID_HERE'
  options[:incentive_id] = 'INSERT_INCENTIVE_ID_HERE'

  if options[:customer_id].nil? ||
      options[:customer_id] == 'INSERT_CUSTOMER_ID_HERE' ||
      options[:incentive_id].nil? ||
      options[:incentive_id] == 'INSERT_INCENTIVE_ID_HERE'
    puts 'Missing required arguments. See usage:'
    puts 'Customer ID and Incentive ID are required.'
    exit 1
  end

---

2. examples/incentives/fetch_incentive.rb

• Documentation Region Tags: Add # [START fetch_incentive] and # [END fetch_incentive] region tags around the main method so Google Developers documentation (devsite) can automatically index the code snippet.
• Standardize Currency Formatting in format_money: Simplify calculation to cleanly format zero, fractional, and negative currency values with standard 2 decimal places:

  def format_money(money)
    return 'N/A' if money.nil?
  
    amount = money.units.to_f + (money.nanos.to_f / 1_000_000_000.0)
    sprintf('%.2f %s', amount, money.currency_code)
  end

• CLI Option Placeholder & Check: Initialize options[:email] = 'INSERT_EMAIL_HERE' and validate against INSERT_EMAIL_HERE in if __FILE__ == $0.

[bobhancockg]

Overall, the implementation for adding incentive examples is well-structured and incorporates previous feedback nicely. Here are a few remaining recommendations and optimizations before merging:

1. examples/incentives/apply_incentive.rb:

   • Clean up redundant request_args[:country_code] = country_code if country_code duplicate assignment on line 36.

2. examples/incentives/fetch_incentive.rb:

   • Update request field name from type: :ACQUISITION to incentive_type: :ACQUISITION to match current Google Ads API protobuf definitions (v24+) and avoid runtime ArgumentError.
   • Provide default nil arguments in def fetch_incentive(email, language_code = nil, country_code = nil) and construct request args using .compact.
   • Fix missing whitespace in string continuation on lines 51-52 ("type." \ "Non-CYO" -> "type.Non-CYO").
   • Defensively format monetary currency strings against empty protobuf defaults ("").

[bobhancockg]

Updated the PR with the following optimizations:

• Parameter Defaults & API Alignment:
  • : Defaulted country_code to 'US' (required by API proto schema) rather than nil to avoid INVALID_ARGUMENT when omitted.
  • : Provided default values (language_code = 'en', country_code = 'US') matching API proto defaults.
• Defensive Protobuf Traversal:
  • Used incentive_id&.to_i to prevent coercing nil to 0.
  • Added safe navigation (response.incentive_offer&.cyo_incentives).
  • Added !field.to_s.empty? checks for protobuf strings before output.
  • Improved requirement fallback message when spend requirement is absent.
  • Cleaned up format_money currency formatting.
• Documentation / Code Snippet Tags:
  • Extended [START fetch_incentive] / [END fetch_incentive] scope to encompass helper functions (print_incentive_details, format_requirement, format_money) so extracted DevSite snippets are complete and runnable.
• Diagnostics & Error Reporting:
  • Surfaced e.request_id in stderr when catching GoogleAdsError across both scripts.
  • Standardized CLI parameter validation to report the exact missing flag with OptionParser usage banner.

[bobhancockg]

1. DevSite filename alignment (fetch_incentives.rb): The DevSite Incentives guide (docs/billing/incentives.md) and sample page (samples/fetch-incentives.md) reference code_example_id='incentives/fetch_incentives' (plural, matching fetch_incentives.py in Python, FetchIncentives.java in Java, and FetchIncentives.cs in .NET), which resolves to examples/incentives/fetch_incentives.rb. Consider renaming this file to examples/incentives/fetch_incentives.rb so DevSite snippet inclusion works out of the box without custom path overrides.
2. Executable file mode (100755) & header: Since #!/usr/bin/env ruby and Usage: %s [options] were added for standalone script execution, please chmod +x (100755) this file (currently 100644). Also, # Encoding: utf-8 on line 2 can be removed since UTF-8 is the default source encoding in Ruby 2.0+.

[bobhancockg]

When no incentive is available for a user, the API returns a populated IncentiveOffer with offer_type == :NO_INCENTIVE and cyo_incentives unset (nil). Currently, that case skips this nil? check and falls into the else branch on line 58 (printing 'Incentive offer is not a CHOOSE_YOUR_OWN_INCENTIVE type...').

Checking for :NO_INCENTIVE here handles empty offers accurately (and allows removing the redundant &. on line 49):

Suggested change

	if response.incentive_offer.nil?
	if response.incentive_offer.nil? || response.incentive_offer.offer_type == :NO_INCENTIVE

[bobhancockg]

Since fetch_incentive strips whitespace on line 32 (email.to_s.strip), using .strip.empty? here ensures whitespace-only input (e.g., -E " ") is caught during CLI validation rather than sending an empty email: "" to the API:

Suggested change

	if options[:email].to_s.empty? || options[:email] == 'INSERT_EMAIL_HERE'
	if options[:email].to_s.strip.empty? || options[:email] == 'INSERT_EMAIL_HERE'

[bobhancockg]

Since #!/usr/bin/env ruby and Usage: %s [options] are used for standalone execution, please chmod +x (100755) this file (currently committed with mode 100644). Also, # Encoding: utf-8 on line 2 is redundant in Ruby 2.0+ and can be removed.

[bobhancockg]

If apply_incentive is called programmatically with a non-numeric string (such as an unreplaced placeholder 'INSERT_INCENTIVE_ID_HERE'), incentive_id&.to_i silently coerces it to 0 (selected_incentive_id: 0). Using Integer(incentive_id) when incentive_id is present fails fast with an ArgumentError on invalid input:

Suggested change

	selected_incentive_id: incentive_id&.to_i,
	selected_incentive_id: incentive_id ? Integer(incentive_id) : nil,

[bobhancockg]

Use .strip.empty? (and strip country_code in request_args on line 38) to guard against whitespace-only --country-code values:

Suggested change

	if options[:country_code].to_s.empty?
	if options[:country_code].to_s.strip.empty?