Skip to content

Changes needed for MPA support - #555

Open
dorasun wants to merge 8 commits into
mainfrom
add_mpa_examples
Open

dorasun wants to merge 8 commits into
mainfrom
add_mpa_examples

Conversation

@dorasun

@dorasun dorasun commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Change-Id: Iaefffce0a2955c3df4369affc6335fb9313151df

Change-Id: Iaefffce0a2955c3df4369affc6335fb9313151df
@dorasun
dorasun requested a review from a team as a code owner July 16, 2026 16:05
Change-Id: Ie7f15d5ef3968dd177b8eddfbf43b566f72fca3c
@AnashOommen
AnashOommen requested review from bobhancockg and removed request for AnashOommen and sarahcaseybot August 6, 2026 21:38
Comment thread examples/account_management/update_user_access.rb Outdated
Comment thread examples/account_management/invite_user_with_access_role.rb
Comment thread examples/account_management/invite_user_with_access_role.rb
Comment thread examples/account_management/invite_user_with_access_role.rb
Comment thread examples/account_management/update_user_access.rb
@dorasun
dorasun requested a review from bobhancockg August 7, 2026 17:39
dorasun added 3 commits August 7, 2026 20:30
Change-Id: Id285de92d3bb3e553198a9823b3b9afb51b67538
…improving validation

Change-Id: I61c181257475a3311c9b775de7488963ad6a4a70
Change-Id: Idebe3ddca986d7e88884d28f3e000ffc566c4d1b
Comment thread examples/account_management/invite_user_with_access_role.rb
Comment thread examples/account_management/invite_user_with_access_role.rb Outdated
Comment thread examples/account_management/invite_user_with_access_role.rb
Comment thread examples/account_management/update_user_access.rb
Comment thread examples/account_management/update_user_access.rb
Comment thread examples/account_management/update_user_access.rb
…r_access.rb

Change-Id: Ic23d5af07d153674d7336edb0d4fdb6193c7aeaa
Change-Id: I0f5ecf8159a2534b6ae36cbb331020636ab2bf6d
@dorasun
dorasun requested a review from bobhancockg September 8, 2026 19:08
@bobhancockg

Copy link
Copy Markdown
Contributor

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:

  1. 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.
  2. 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.
  3. 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 || []).
  4. 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.
  5. 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.

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'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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) || []

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

)

result = response.first.customer_user_access
result = response.first&.customer_user_access

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

"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 " \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

"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} " \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants