[codex] Add TEXT_AD extension update flags by axisrow · Pull Request #263 · axisrow/direct-cli

axisrow · 2026-05-24T04:30:47Z

Summary

add typed ads update TEXT_AD flags for VideoExtension.CreativeId and PriceExtension fields
keep the new extension flags TEXT_AD-only through existing subtype compatibility validation
update dry-run coverage, WSDL parity mapping, audit docs, and README EN/RU one-line examples

Documentation check

Checked official Yandex Direct ads.update docs: TextAd.VideoExtension.CreativeId is long/nillable and TextAd.PriceExtension fields are Price/OldPrice long-unit values plus PriceQualifier/PriceCurrency enums.

Verification

python3 -m pytest tests/test_dry_run.py -k "ads_update_text_ad and (video_extension or price_extension or extension_flags)"
python3 -m pytest tests/test_wsdl_parity_gate.py
python3 -m pytest tests/test_cli.py tests/test_dry_run.py tests/test_wsdl_parity_gate.py

Closes #245

axisrow · 2026-05-24T04:31:05Z

@codex @claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems, API contract mismatches, and missing tests. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production.

claude · 2026-05-24T04:31:14Z

Claude finished @axisrow's task in 3m 28s —— View job

Code Review — PR #263 (TEXT_AD extension update flags)

Read PR diff vs origin/feat/issue-239-optional-fields-gate
Review direct_cli/commands/ads.py extension flags
Review dry-run tests coverage
Review WSDL parity gate updates
Post findings

Verdict

No blocking bugs found. The implementation is correct, the subtype gating is enforced, the no-op guard still trips on {Id: N}-only payloads, and the dry-run/parity coverage is appropriate. Enum values for PriceQualifierEnum (NONE, FROM, UP_TO) and PriceCurrencyEnum (9 currencies) match the cached WSDL (tests/wsdl_cache/ads.xml) exactly.

A few non-blocking follow-up gaps below.

Findings

1. (FYI / follow-up) `nillable=true` fields cannot be cleared

The WSDL marks three of these paths as nillable="true":

TextAdUpdate.PriceExtension (whole sub-element)
PriceExtensionUpdateItem.OldPrice
VideoExtensionUpdateItem.CreativeId

nillable is how Yandex's API surfaces "clear this field." The CLI currently uses is not None / truthy checks on the Click value, so the user can set values but has no way to send a nil to clear the existing extension. The --callouts-* flags solved the equivalent issue via explicit SET / ADD / REMOVE operations.

Not a regression and not in this PR's scope (issue #245 asks for update flags), but worth filing as a follow-up so the audit doesn't claim full parity for TextAd.PriceExtension when clearing is unreachable.

direct_cli/commands/ads.py:73-89, direct_cli/commands/ads.py:736-739

2. (Minor) No client-side range validation on numeric flags

--price-extension-price, --price-extension-old-price, and --video-extension-creative-id are declared as type=int and accept negative values. The API will reject them, but a click.IntRange(min=0) would surface the error earlier with a clearer message and stay consistent with how Price (xsd:long) and CreativeId (xsd:long) are used elsewhere. Low priority.

direct_cli/commands/ads.py:531-551

3. (Confirmed OK) Sibling subtypes correctly kept out of scope

WSDL also exposes MobileAppAdUpdate.VideoExtension and DynamicTextAdUpdate.PriceExtension. The PR (correctly) limits these flags to TEXT_AD and leaves those rows in WSDL_OPTIONAL_FIELD_AUDIT.md as missing_followup under issue #240 — verified at audit lines 234, 244, 264-265. No coverage gap introduced. ✓

4. (Confirmed OK) No-op guard still triggers correctly

test_ads_update_text_ad_extension_flags_count_as_updatable_fields proves that an extension-only update keeps len(ad_data) == 2 and passes the empty-subtype no-op guard at ads.py:771. ✓

5. (Confirmed OK) Subtype rejection is enforced before payload build

Both rejection tests (TEXT_IMAGE_AD for --video-extension-creative-id, MOBILE_APP_AD for --price-extension-price) verify the flags fail with click.UsageError rather than being silently dropped — invariant H2 from issue #198. ✓

6. (Confirmed OK) Partial `PriceExtension` update is WSDL-legal

The WSDL PriceExtensionUpdateItem has all four children at minOccurs=0, so the partial-payload test (PriceCurrency alone) is consistent with the schema. Live API may impose stricter business validation, but that's a runtime concern, not a CLI contract issue.

tests/wsdl_cache/ads.xml (PriceExtensionUpdateItem complexType)

Tests / coverage

Dry-run coverage is thorough: full payload, partial payload, subtype rejection (two subtypes), and no-op-guard bypass. The parity gate OPTIONAL_FIELD_CLI_OPTIONS correctly maps each WSDL path to its flag(s), and OPTIONAL_FIELD_AUDIT removed the now-implemented #245 rows. README EN/RU one-liners updated symmetrically.

No missing tests for the changes in scope.

• Branch: codex/issue-245-ads-textad-extensions

chatgpt-codex-connector · 2026-05-24T04:33:57Z

Codex Review: Didn't find any major issues. 🎉

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copilot

Pull request overview

Adds typed CLI flags to direct ads update for the TextAd.VideoExtension.CreativeId and TextAd.PriceExtension.* WSDL fields (issue #245), keeping them strictly TEXT_AD-only through the existing per-subtype incompatibility validation.

Changes:

Add --video-extension-creative-id and four --price-extension-* typed flags to ads update, plus a _build_price_extension helper that normalizes enum inputs to upper case.
Mark the seven TextAd.VideoExtension / TextAd.PriceExtension WSDL paths as supported in the parity gate mapping and audit doc, and add dry-run tests covering full/partial payloads, subtype rejection, and the no-op guard.
Update README EN/RU one-liners and explanatory paragraphs to document the new flags and the long-unit price format.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
direct_cli/commands/ads.py	New Click options, `_build_price_extension` helper, allow-list + flag-name entries, and payload assembly under `TextAd`.
tests/test_wsdl_parity_gate.py	Adds `OPTIONAL_FIELD_CLI_OPTIONS` mappings for the new paths; removes obsolete `missing_followup` audit entries.
tests/test_dry_run.py	Adds payload, partial-payload, subtype-rejection, and no-op-guard tests for the new flags.
tests/WSDL_OPTIONAL_FIELD_AUDIT.md	Drops #245 follow-up rows, flips path rows to `supported`, updates summary counts (2830→2823 missing, 411→418 supported).
README.md	EN and RU sections gain a one-line example and a short explanatory paragraph for the new flags.

* [codex] Add adgroup tracking params flags (#258) * Add adgroup tracking params flags * test: cover adgroup tracking params add rejection --------- Co-authored-by: axisrow <[email protected]> * [codex] Add sitelink TurboPageId typed input (#259) * feat(sitelinks): support turbo page id * test(sitelinks): cover turbo page json input --------- Co-authored-by: axisrow <[email protected]> * [codex] Add retargeting description flags (#260) * feat(retargeting): add description flags * fix(retargeting): preserve empty description --------- Co-authored-by: axisrow <[email protected]> * [codex] Add adgroups negative keyword flags (#261) * feat(adgroups): add negative keyword flags * fix(adgroups): report invalid negative set ids as usage errors --------- Co-authored-by: axisrow <[email protected]> * [codex] Add vcards nested contact flags (#262) * feat(vcards): add nested contact fields * fix(vcards): preserve grouped flag usage errors --------- Co-authored-by: axisrow <[email protected]> * feat(ads): add text ad extension update flags (#263) Co-authored-by: axisrow <[email protected]> * feat(feeds): add url feed detail flags (#265) Co-authored-by: axisrow <[email protected]> * docs: align optional WSDL audit depth notes * docs: document milestone 18 typed flags --------- Co-authored-by: axisrow <[email protected]>

feat(ads): add text ad extension update flags

1bfeede

Copilot AI review requested due to automatic review settings May 24, 2026 04:30

Copilot started reviewing on behalf of axisrow May 24, 2026 04:31 View session

Copilot AI reviewed May 24, 2026

View reviewed changes

axisrow merged commit 53db1fb into feat/issue-239-optional-fields-gate May 24, 2026
10 checks passed

axisrow deleted the codex/issue-245-ads-textad-extensions branch May 24, 2026 04:36

This was referenced May 24, 2026

ads update TEXT_AD: add VideoExtension and PriceExtension typed flags #245

Closed

ads update: complete WSDL subtype parity — add 12 missing ad subtypes #240

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[codex] Add TEXT_AD extension update flags#263

[codex] Add TEXT_AD extension update flags#263
axisrow merged 1 commit into
feat/issue-239-optional-fields-gatefrom
codex/issue-245-ads-textad-extensions

axisrow commented May 24, 2026

Uh oh!

axisrow commented May 24, 2026

Uh oh!

claude Bot commented May 24, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot commented May 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

axisrow commented May 24, 2026

Summary

Documentation check

Verification

Uh oh!

axisrow commented May 24, 2026

Uh oh!

claude Bot commented May 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review — PR #263 (TEXT_AD extension update flags)

Verdict

Findings

1. (FYI / follow-up) nillable=true fields cannot be cleared

2. (Minor) No client-side range validation on numeric flags

3. (Confirmed OK) Sibling subtypes correctly kept out of scope

4. (Confirmed OK) No-op guard still triggers correctly

5. (Confirmed OK) Subtype rejection is enforced before payload build

6. (Confirmed OK) Partial PriceExtension update is WSDL-legal

Tests / coverage

Uh oh!

chatgpt-codex-connector Bot commented May 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

claude Bot commented May 24, 2026 •

edited

Loading

1. (FYI / follow-up) `nillable=true` fields cannot be cleared

6. (Confirmed OK) Partial `PriceExtension` update is WSDL-legal