Changes needed for MPA support by dorasun · Pull Request #555 · googleads/google-ads-ruby

dorasun · 2026-07-16T16:05:25Z

Change-Id: Iaefffce0a2955c3df4369affc6335fb9313151df

Change-Id: Ie7f15d5ef3968dd177b8eddfbf43b566f72fca3c

Change-Id: Id285de92d3bb3e553198a9823b3b9afb51b67538

…improving validation Change-Id: I61c181257475a3311c9b775de7488963ad6a4a70

Change-Id: Idebe3ddca986d7e88884d28f3e000ffc566c4d1b

bobhancockg · 2026-08-12T14:45:51Z

@@ -0,0 +1,200 @@
+#!/usr/bin/env ruby


Standardize on the client library header convention (# frozen_string_literal: true) rather than shebang and encoding comment.

frozen_string_literal: true

All of the examples use "#!/usr/bin/env ruby" because they're meant to be run as standalone scripts. The shebang is standard practice

Good point on keeping #!/usr/bin/env ruby for standalone execution. Two small follow-ups here:

This file was committed with mode 100644 (non-executable), unlike invite_user_with_access_role.rb and update_user_access.rb (100755). Since opts.banner now uses Usage: %s [options], please chmod +x (100755) this file so running ./fetch_and_approve_pending_multi_party_auth_reviews.rb directly works without Permission denied.

# Encoding: utf-8 on line 2 is a legacy Ruby 1.9 comment (UTF-8 is the default source encoding in Ruby 2.0+, and .rubocop.yml targets Ruby 3.2); you can replace line 2 with # frozen_string_literal: true right below the shebang.

…r_access.rb Change-Id: Ic23d5af07d153674d7336edb0d4fdb6193c7aeaa

Change-Id: I0f5ecf8159a2534b6ae36cbb331020636ab2bf6d

…ndling, and restore config

bobhancockg · 2026-09-15T18:28:10Z

Summary of Review & Applied Optimizations

I reviewed the changes in PR #555 and pushed a commit directly to add_mpa_examples with the following fixes and optimizations:

Restored google_ads_config.rb:
- Reverted the accidental file rename of google_ads_config.rb to .template. google-ads-googleads.gemspec (line 29) explicitly lists google_ads_config.rb and build scripts expect this path.
Fixed Fatal Type Mismatch in update_user_access.rb:
- ACCESS_ROLES is an array of strings (%w[ADMIN STANDARD READ_ONLY EMAIL_ONLY]), but update_user_access was passed a Symbol from the CLI (:ADMIN). This caused ACCESS_ROLES.include?(access_role) to always evaluate to false and raise an "Illegal access role specified" error.
- Normalized and validated access_role = access_role.to_s.upcase.to_sym inside both update_user_access and invite_user_with_access_role.
Added Defensive Protobuf Traversal & Nil-Guards:
- Added safe navigation (&.) on intermediate protobuf submessages in fetch_and_approve_pending_multi_party_auth_reviews.rb (access_review&.new_customer_user_access&.access_role, new_invite&.email_address, response.result_or_error&.first).
- Safely guarded response.first&.customer_user_access in get_user_id to prevent NoMethodError if no record is returned.
- Used safe navigation on response.result&.multi_party_auth_review and response.result&.resource_name.
- Defaulted decoded partial failure errors to an empty array (failures || []).
In-Method Customer ID Sanitization:
- Added customer_id = customer_id.to_s.tr('-', '') within fetch_and_approve_pending_multi_party_auth_reviews, fetch_pending_mpa_reviews, and approve_mpa_review so programmatic callers are protected regardless of input formatting.
Standardized CLI Error Reporting:
- Updated the rescue Google::Ads::GoogleAds::Errors::GoogleAdsError => e blocks across all three examples to output e.request_id and structured error locations consistent with the client library's example suite.

bobhancockg · 2026-10-08T00:28:31Z

+      multi_party_auth_review.customer_user_access_review.new_customer_user_access,
+      multi_party_auth_review.customer_user_access_invitation_review.new_customer_user_access_invitation
+    FROM multi_party_auth_review
+    WHERE multi_party_auth_review.review_status = 'PENDING'


Consider adding ORDER BY multi_party_auth_review.creation_date_time ASC to the query. Because fetch_and_approve_pending_multi_party_auth_reviews approves pending_reviews.first, ordering explicitly by creation date ensures deterministic FIFO approval of the oldest pending review.

bobhancockg · 2026-10-08T00:28:31Z

+          puts "\tOld resource name: #{access_review&.old_customer_user_access}."
+        end
+      elsif mpa_review.target_resource == :CUSTOMER_USER_ACCESS_INVITATION
+        new_invite = mpa_review.customer_user_access_invitation_review&.new_customer_user_access_invitation


Defensive check suggestion: if an MPA review corresponds to an invitation removal/revocation or if new_customer_user_access_invitation is not populated, new_invite&.email_address and new_invite&.access_role will be nil, resulting in Invitation email address: , Role: ..

Consider wrapping this in if new_invite before logging to avoid printing empty fields:

if new_invite puts "\tInvitation email address: #{new_invite.email_address}, " \ "Role: #{new_invite.access_role}." end

bobhancockg · 2026-10-08T00:28:31Z

+  elsif result_or_error&.partial_failure_error
+    # Partial failure error
+    errors_count = 0
+    failures = client.decode_partial_failure_error(result_or_error.partial_failure_error) || []


If client.decode_partial_failure_error returns an empty array or no inner errors are unpacked, errors_count will remain 0, printing 0 partial failure error(s) occurred even though partial_failure_error was present. Consider checking if failures.empty? or adding a fallback message to avoid confusing logs.

bobhancockg · 2026-10-08T00:28:31Z

  )

-  result = response.first.customer_user_access
+  result = response.first&.customer_user_access


Optimization suggestion: in get_user_id, consider appending LIMIT 1 to query (line 66) since each email address can have at most one customer_user_access record in an account. This avoids unnecessary scanning overhead.

bobhancockg · 2026-10-08T00:28:32Z

+      "access changes. See advanced_operations/fetch_and_approve_pending_multi_party_auth_reviews.rb " \
+      "for an example on how to approve an MPA auth review using the API."
+  else
+    puts "Successfully updated customer user access with resource name " \


Defensive formatting: if response.result is unexpectedly nil, response.result&.resource_name renders as an empty string, yielding "... resource name .". Consider providing a fallback (e.g. #{response.result&.resource_name || 'N/A'}) or checking if response.result.

bobhancockg · 2026-10-08T00:28:32Z

+      "for an example on how to approve an MPA auth review using the API."
+  else
+    # Print out information of the created invitation.
+    puts "Customer user access invitation was sent for customerId = #{customer_id} " \


Defensive formatting: if response.result is unexpectedly nil, response.result&.resource_name renders as an empty string, yielding "... resource name .". Consider providing a fallback (e.g. #{response.result&.resource_name || 'N/A'}) or checking if response.result.

Change-Id: I110442d56bc5f139d774b15f5af0078980afa87b

bobhancockg · 2026-10-14T12:17:56Z

+        multi_party_auth_review: pending_review,
+        new_status: :APPROVED,
+      }
+    ],


Consider using the client library's operation factory (client.operation.resolve_multi_party_auth_review) instead of a raw hash literal, to stay consistent with client.operation usage in the other examples and get protobuf field validation at construction time:

operation = client.operation.resolve_multi_party_auth_review do |op| op.multi_party_auth_review = pending_review op.new_status = :APPROVED end response = client.service.multi_party_auth_review.resolve_multi_party_auth_review( customer_id: customer_id, operations: [operation], )

bobhancockg · 2026-10-14T12:17:56Z

+
+  access_role_str = access_role.to_s.upcase
+  if !ACCESS_ROLES.include?(access_role_str)
    raise "Illegal access role specified. Expected one of " \


Prefer raising ArgumentError instead of a generic RuntimeError when validating access_role:

Suggested change

raise "Illegal access role specified. Expected one of " \

raise ArgumentError, "Illegal access role specified. Expected one of " \

"#{ACCESS_ROLES.join(' ')}"

bobhancockg · 2026-10-14T12:17:56Z

+
+  access_role_str = access_role.to_s.upcase
+  if !ACCESS_ROLES.include?(access_role_str)
+    raise "Illegal access role specified. Expected one of " \


Prefer raising ArgumentError instead of a generic RuntimeError when validating access_role:

Suggested change

raise "Illegal access role specified. Expected one of " \

raise ArgumentError, "Illegal access role specified. Expected one of " \

"#{ACCESS_ROLES.join(' ')}"

bobhancockg · 2026-10-14T12:17:56Z

+      options.fetch(:customer_id),
      options.fetch(:email_address),
-      options.fetch(:access_role),
+      options.fetch(:access_role).upcase.to_sym,


Since update_user_access already normalizes access_role.to_s.upcase.to_sym on lines 44–49 (and modify_user_access also calls .to_sym on line 90), converting .upcase.to_sym both here and in modify_user_access is redundant.

bobhancockg · 2026-10-14T12:17:56Z

+      options.fetch(:customer_id),
      options.fetch(:email_address),
-      options.fetch(:access_role).to_sym,
+      options.fetch(:access_role).upcase.to_sym,


Since invite_user_with_access_role already normalizes access_role.to_s.upcase.to_sym on lines 39–44, converting .upcase.to_sym here is redundant.

…dling, query ordering, permissions, and argument typing Change-Id: I38eac75318bb939bb11def66ed1bc1bff51da18e

Changes needed for MPA support

1654a00

Change-Id: Iaefffce0a2955c3df4369affc6335fb9313151df

dorasun requested a review from a team as a code owner July 16, 2026 16:05

dorasun requested review from AnashOommen, BenRKarl and sarahcaseybot July 16, 2026 16:05

bobhancockg requested changes Jul 16, 2026

View reviewed changes

Respond to comments

7d2c9d3

Change-Id: Ie7f15d5ef3968dd177b8eddfbf43b566f72fca3c

AnashOommen requested review from bobhancockg and removed request for AnashOommen and sarahcaseybot August 6, 2026 21:38

bobhancockg requested changes Aug 7, 2026

View reviewed changes

dorasun requested a review from bobhancockg August 7, 2026 17:39

dorasun added 3 commits August 7, 2026 20:30

Merge branch 'main' into add_mpa_examples

7cce923

Change-Id: Id285de92d3bb3e553198a9823b3b9afb51b67538

Fixing whitespace, standardizing flags, adding back missing end tag, …

19228ff

…improving validation Change-Id: I61c181257475a3311c9b775de7488963ad6a4a70

Freezing access role arrays

3688027

Change-Id: Idebe3ddca986d7e88884d28f3e000ffc566c4d1b

bobhancockg requested changes Aug 12, 2026

View reviewed changes

dorasun added 2 commits August 12, 2026 18:45

Address PR comments on invite_user_with_access_role.rb and update_use…

70a2307

…r_access.rb Change-Id: Ic23d5af07d153674d7336edb0d4fdb6193c7aeaa

Improve consistency, null handling

cd28040

Change-Id: I0f5ecf8159a2534b6ae36cbb331020636ab2bf6d

dorasun requested a review from bobhancockg September 8, 2026 19:08

Optimize MPA examples: fix role validation, safe navigation, error ha…

0d05a28

…ndling, and restore config

bobhancockg requested changes Oct 8, 2026

View reviewed changes

Standardize script entrypoint check to $0 and clean usage banner

6aa4b0c

Change-Id: I110442d56bc5f139d774b15f5af0078980afa87b

bobhancockg reviewed Oct 14, 2026

View reviewed changes

Address review comments on MPA examples: operation factory, error han…

e2f442f

…dling, query ordering, permissions, and argument typing Change-Id: I38eac75318bb939bb11def66ed1bc1bff51da18e

	raise "Illegal access role specified. Expected one of " \
	raise ArgumentError, "Illegal access role specified. Expected one of " \
	"#{ACCESS_ROLES.join(' ')}"

Conversation

dorasun commented Jul 16, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

frozen_string_literal: true

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bobhancockg commented Sep 15, 2026

Summary of Review & Applied Optimizations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Labels

2 participants