Repository navigation
Conversation
Change-Id: I57fa8a776fb8a1cbd43f6c1d3a1a71c3c63bdd19
Change-Id: I2f4db5b40ddc0494f631b62d60b6992f786cbf57
Code Vetting & Review RecommendationsOverall, the implementation for adding incentive examples ( 1.
|
Change-Id: I19de34550a54efaa8095b503bd5e8c449930e9d4
Change-Id: I4f3b19fe5ace2a6d88f7f0c3b90906f97b0d454b
bobhancockg
left a comment
There was a problem hiding this comment.
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:
-
examples/incentives/apply_incentive.rb:- Clean up redundant
request_args[:country_code] = country_code if country_codeduplicate assignment on line 36.
- Clean up redundant
-
examples/incentives/fetch_incentive.rb:- Update request field name from
type: :ACQUISITIONtoincentive_type: :ACQUISITIONto match current Google Ads API protobuf definitions (v24+) and avoid runtimeArgumentError. - Provide default
nilarguments indef 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 (
"").
- Update request field name from
Change-Id: I6dfd7bc56bafdf1910e5e35209600f3bc52a6e84
…traversal, and CLI validation
|
Updated the PR with the following optimizations:
|
| # Passing :ACQUISITION as the symbol representation of the IncentiveType enum. | ||
| incentive_type: :ACQUISITION | ||
| }.compact | ||
|
|
There was a problem hiding this comment.
Style / Formatting: Trailing whitespace on line 36 after .compact.
|
|
||
| # If the offer type is CHOOSE_YOUR_OWN_INCENTIVE, there will be 3 incentives in the | ||
| # response. At the time this example was written, all incentive offers are CYO incentive offers. | ||
| if response.incentive_offer&.cyo_incentives |
There was a problem hiding this comment.
Enhancement (API Coverage): The IncentiveOffer message can also contain consolidated_terms_and_conditions_url (combining terms across all CYO incentive offers). Consider printing it when present:
if !response.incentive_offer.consolidated_terms_and_conditions_url.to_s.empty?
puts "Consolidated terms and conditions: '#{response.incentive_offer.consolidated_terms_and_conditions_url}'"
end| parser.parse! | ||
|
|
||
| # Check if required parameters are present. | ||
| if options[:email].nil? || options[:email] == 'INSERT_EMAIL_HERE' |
There was a problem hiding this comment.
Defensive validation: Consider checking options[:email].to_s.empty? instead of just options[:email].nil? (consistent with --country-code in apply_incentive.rb line 114):
if options[:email].to_s.empty? || options[:email] == 'INSERT_EMAIL_HERE'
puts "Missing required argument: --email (-E) is required.\n\n"
puts parser
exit 1
endThis defensively catches empty string inputs (e.g. -E "").
| if !response.coupon_code.to_s.empty? | ||
| puts "Applied incentive with coupon code '#{response.coupon_code}'." | ||
| end | ||
| end |
There was a problem hiding this comment.
UX / Output: If the API returns a response where both creation_time and coupon_code happen to be empty, the method currently completes silently with no output. Consider adding a fallback confirmation if neither is printed:
if !response.creation_time.to_s.empty?
puts "Incentive was created at '#{response.creation_time}'."
end
if !response.coupon_code.to_s.empty?
puts "Applied incentive with coupon code '#{response.coupon_code}'."
end
if response.creation_time.to_s.empty? && response.coupon_code.to_s.empty?
puts "Incentive applied successfully."
end
Change-Id: I57fa8a776fb8a1cbd43f6c1d3a1a71c3c63bdd19