Repository navigation
draft_update_conversion_action: add category, refuse include_in_conversions_metric - #122
Open
illia-sapryga wants to merge 1 commit into
Open
illia-sapryga wants to merge 1 commit into
illia-sapryga wants to merge 1 commit into
Conversation
…rsions_metric category moves the action to another goal; the preview shows the current one. include_in_conversions_metric was documented as mutable on update. Google answers IMMUTABLE_FIELD at the real apply and validate_only does not catch it, so the draft now refuses it and names primary_for_goal and category instead.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two related changes to one tool.
The docstring says
include_in_conversions_metricis mutable on update. It isn't. On a real account Google answeredIMMUTABLE_FIELDat apply, after the validate-only dry run had passed, so the preview and the dry run both looked fine and only the real apply failed. Google derives the value fromprimary_for_goaland whether the goal of the action's category is biddable. The draft now refuses the parameter and names those two instead. I kept the parameter so a caller gets that message and not a schema error. A plan already stored with the field raises at apply and sends nothing. The create tool's docs pointed at the update tool for this, so I corrected those too.That leaves a gap: an action in a category whose goal isn't biddable has no way to start counting through AdLoop.
categorycloses it by moving the action to another goal, for example DEFAULT to PHONE_CALL_LEAD. The preview showscurrent_categoryand warns that campaigns bidding on those goals can start or stop optimizing toward it. The same category, a removed action and an unknown ID are refused at draft time.What it does not do: it doesn't change
primary_for_goal, and it doesn't check whether the target goal is biddable, so the warning says "only if". There is no re-check of the category at apply; the tool's other fields have none either. I left the rules file alone because these tools aren't listed there and the refusal message explains itself.Tested with the full suite.
test_include_in_conversions_metric_is_mutable_on_updateandtest_update_include_in_conversions_metric_fieldmaskasserted the old behaviour and are replaced by tests of the refusal. The category path runs throughconfirm_and_apply(dry_run=True)with real protos, the real validate-only wrapper and a fake service, and GAQL is stubbed. I saw theIMMUTABLE_FIELDanswer on a live account, but this code has not been run against one, and validate-only is known not to catch that particular error, so a green dry run says little about real updates to this tool.