Changes needed for MPA support - #555
Conversation
Change-Id: Iaefffce0a2955c3df4369affc6335fb9313151df
| # code. | ||
| # | ||
| # Running the example with -h will print the command line usage. | ||
| options[:customer_id] = 'INSERT_CUSTOMER_ID_HERE' |
There was a problem hiding this comment.
Where do you check whether the user has replaced this placeholder before invoking the main method?
Change-Id: Ie7f15d5ef3968dd177b8eddfbf43b566f72fca3c
| # ENV['HOME']/google_ads_config.rb when called without parameters | ||
| client = Google::Ads::GoogleAds::GoogleAdsClient.new | ||
|
|
||
| if !ACCESS_ROLES.include?(access_role) |
There was a problem hiding this comment.
Issue: update_user_access references ACCESS_ROLES on line 34, but ACCESS_ROLES is defined on line 98 inside the if FILE == $PROGRAM_NAME block.
Impact: If update_user_access is required or called from another Ruby module (outside CLI execution), calling the method raises NameError: uninitialized constant ACCESS_ROLES.
Fix: Move ACCESS_ROLES = %w[ADMIN STANDARD READ_ONLY EMAIL_ONLY].freeze to top-level script scope (above update_user_access).
There was a problem hiding this comment.
I've moved it to the top-level of the script
| puts "Customer user access invitation was sent for customerId = #{customer_id} " \ | ||
| "email address = '#{email_address}', " \ | ||
| "access role = '#{access_role}'." | ||
| # [END invite_user_with_access_role] |
There was a problem hiding this comment.
Issue: # [END invite_user_with_access_role] was deleted in the diff and not restored.
Impact: DevSite code snippet ingestion tools will fail to bound the snippet or fail ingestion for invite_user_with_access_role.
Fix: Restore # [END invite_user_with_access_role] at the end of the method block.
There was a problem hiding this comment.
Added back, great catch!
| result = result_or_error.result | ||
| puts "Approved multi-party auth review: #{result.multi_party_auth_review}." | ||
| if !result.customer_user_access_invitation.empty? | ||
| puts "New user invitation created: #{result.customer_user_access_invitation}" |
There was a problem hiding this comment.
Issue:
if !result.customer_user_access_invitation.empty?
elsif !result.customer_user_access.empty?
Risk: If either string field is ever nil (depending on protobuf generation/deserialization state), calling .empty? directly on nil will raise NoMethodError.
Fix: Use !result.customer_user_access_invitation.to_s.empty? or result.customer_user_access_invitation != "" for safe string checking.
There was a problem hiding this comment.
Great point, I've added .to_s where needed
| "access role = '#{access_role}'. The invitation resource name is " \ | ||
| "#{response.result.resource_name}." | ||
| end | ||
| end |
There was a problem hiding this comment.
invite_user_with_access_role.rb:46 contains 74 trailing space characters after "for an example on how to approve an MPA auth review using the API.".
There was a problem hiding this comment.
Removed extra whitespace!
| options.fetch(:access_role).to_sym, | ||
| options.fetch(:access_role).upcase.to_sym, | ||
| ) | ||
| rescue Google::Ads::GoogleAds::Errors::GoogleAdsError => e |
There was a problem hiding this comment.
contains trailing whitespace on end.
There was a problem hiding this comment.
I'm not seeing any trailing whitespace, but maybe I've already removed it
| opts.separator '' | ||
| opts.separator 'Options:' | ||
|
|
||
| opts.on('-C', '--customer-id CUSTOMER-ID', String, 'Customer ID') do |v| |
There was a problem hiding this comment.
Inconsistent OptionParser Flags:
invite_user_with_access_role.rb uses -C (customer-id), -E (email-address), -R (access-role).
update_user_access.rb uses -C (customer-id), -e (email-address), -a (access-role).
Recommendation: Standardize update_user_access.rb to use -E and -R to match invite_user_with_access_role.rb and other Google Ads Ruby client library examples.
There was a problem hiding this comment.
Good point, standardized for a more consistent usage
| require 'optparse' | ||
| require 'google/ads/google_ads' | ||
|
|
||
| def update_user_access(customer_id, email_address, access_role) |
There was a problem hiding this comment.
freeze array constants to prevent accidental modification at runtime.
+ACCESS_ROLES = %w[
- ADMIN
- STANDARD
- READ_ONLY
- EMAIL_ONLY
+].freeze
| if result_or_error&.result | ||
| result = result_or_error.result | ||
| puts "Approved multi-party auth review: #{result.multi_party_auth_review}." | ||
| if !result.customer_user_access_invitation.empty? |
There was a problem hiding this comment.
Consider Defensive String Checking
- if !result.customer_user_access_invitation.empty?
- if !result.customer_user_access_invitation.to_s.empty?
puts "New user invitation created: #{result.customer_user_access_invitation}"
- elsif !result.customer_user_access.empty?
- elsif !result.customer_user_access.to_s.empty?
There was a problem hiding this comment.
I believe this has been addressed, happy to make further changes if not
Change-Id: Id285de92d3bb3e553198a9823b3b9afb51b67538
…improving validation Change-Id: I61c181257475a3311c9b775de7488963ad6a4a70
Change-Id: Idebe3ddca986d7e88884d28f3e000ffc566c4d1b
2 participants
Change-Id: Iaefffce0a2955c3df4369affc6335fb9313151df