feat(masters): add masters update --headline/--text — Этап B point-replacement - #669
Conversation
…t-replacement Implements per-slot headline/ad-text variant replacement for `direct masters update`, closing the Этап B gap from the #648 umbrella (originally #631). Reuses `_clear_text_field`/`_read_repeating_values` from the create-page implementation (#653); does NOT reuse `_add_repeating_values`, whose contract (clear/rewrite every slot) is the opposite of a point replacement. Deliberate departure from this CLI's dominant list-field convention (`campaigns update --negative-keywords` replaces the whole array) — Мастер кампаний has no API, variant sets can be large, and forcing every variant to be re-typed to fix one typo would defeat the point of a partial update. Documented in code, README, and CHANGELOG as a conscious choice, not an oversight. Also fixes `masters update` on DRAFT campaigns (issue #668, found while live-verifying this PR): DRAFT edit pages have no "Сохранить кампанию" button at all, only saveDraft/launch — `update_master` now detects and handles this, defaulting to draft-preserving saves with an opt-in `--launch`. A live-recon-confirmed redirect race (the draft-save click navigates away from /edit/ ~5s after clicking, racing the immediate post-click reload) is fixed by polling page.url before re-verifying, mirroring copy_master's existing pattern. Live-verified end to end against DRAFT campaign 713231614: replaced a headline slot, confirmed saved via reload, reverted to the original text the same way. Campaign confirmed to remain DRAFT throughout (masters list --status all). Closes #665 Closes #668
411c928 to
52a6a96
Compare
🔍 Local review (cycle 1) — round 39dd6369-7b79-424e-8906-b330df0240eeReviewed locally (
Both reviewers independently reached this slot-blanking case. Verified by executing the parser and by reading the post-save comparison: the check tests the re-read slot against the requested value, so a slot that was successfully emptied compares equal and passes. Deleting a variant is documented as out of scope, so the destructive path should be refused rather than silently confirmed. Tests on the reviewed head: 241 passed. Lint clean on the files this PR touches. |
…ant delete)
`masters update --headline "1="` was accepted and silently DELETED the ad
variant in slot 1 instead of replacing it. The whole path was destructive
and reported success:
_parse_repeating_slot_options split("=", 1) -> {0: ""}
-> _set_repeating_value clears the slot, types ""
-> the form is saved (or, with --launch on a DRAFT, published)
-> _verify_repeating_value_mismatches compares the re-read slot against
the REQUESTED value: "" == "", so it matches and the delete is
reported as a successful update.
Deleting a variant is explicitly out of scope for Этап B (#665, "Явно вне
объёма"), and on an active campaign this is a live ad mutation with no
rollback.
Guarded at both layers: click.UsageError at the CLI boundary, before any
browser session opens, and BrowserSessionError in _set_repeating_value
(before the field is read or cleared) so non-CLI callers are safe too.
Whitespace-only values are rejected the same way — same delete in disguise.
Found in review by both reviewers (/review + Codex adversarial).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d4e89bd9b
ℹ️ 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".
| - New `--headline "N=text"` / `--text "N=text"` flags on `direct masters | ||
| update <CAMPAIGN_ID>` replace one existing headline/ad-text variant slot |
There was a problem hiding this comment.
The canonical command is split inside its code span between direct masters and update <CAMPAIGN_ID>. Copying this text into a shell can execute direct masters and update as separate commands, and the repository explicitly forbids multi-line canonical commands in documentation. Keep the complete invocation on one physical line.
AGENTS.md reference: AGENTS.md:L43-L47
Useful? React with 👍 / 👎.
| if slot_number < 1: | ||
| raise click.UsageError( | ||
| f"{option_name} slot number {slot_number} must be 1 or " "greater." | ||
| ) |
There was a problem hiding this comment.
When a user passes an out-of-range value such as --headline 6=x or --text 4=x, this parser accepts it, so _with_session opens or authenticates a browser before _set_repeating_value eventually rejects the index. This contradicts the helper's stated fail-fast behavior and makes a purely invalid CLI argument incur browser prompts and surface as a browser-session error rather than a usage error; validate the documented upper bounds here as well.
Useful? React with 👍 / 👎.
…he CLI `masters update --headline "6=x"` (or `--text "4=x"`) was accepted by _parse_repeating_slot_options, so a purely invalid CLI argument opened a browser session — with a possible auth prompt — and only then failed, as a BrowserSessionError from _set_repeating_value rather than a UsageError. That contradicts the parser's documented fail-fast contract. The edit page renders a FIXED 5 headline / 3 text slots, so the bound is knowable up front. _parse_repeating_slot_options now takes slot_count and enforces it, with the value imported from the browser layer's own _HEADLINES_SLOT_COUNT/_TEXTS_SLOT_COUNT so the CLI's bound cannot drift from the page's. Below-1 and above-slot_count stay separate branches with distinct messages: "0=x" is a counting mistake, "6=x" is a slot that does not exist. Found in review by Codex (P2). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
direct masters update --headline "N=text"/--text "N=text"— point-replacement of one existing headline/ad-text variant slot at a time (N is the 1-based slot, 1-5 for headlines, 1-3 for ad text). Closes the Этап B gap tracked under the direct masters — follow-up: Этапы B/C/D update, живая проверка add/archive/update, ревью #647 #648 umbrella (issue masters update — Этап B: точечная правка заголовков и текстов объявлений #665, originally direct masters update — редактирование настроек Мастера кампаний #631).updatereplaces the WHOLE array in one call (e.g.campaigns update --negative-keywords, built on_array_of_string_option). Мастер кампаний has no API at all, variant sets can be large, and forcing every variant to be re-typed to fix one typo would defeat the point of a partial update. See_set_repeating_value's docstring, and the CHANGELOG/README sections, for the full rationale — documented as a conscious choice, not an oversight; a future refactor may want to unify this with the rest of the CLI, but that's explicitly out of scope here._clear_text_field/_read_repeating_valuesfrom the create-page implementation (masters add: шаг 2 формы (заголовки/тексты/регион) тоже мигрировал на новую разметку — падает после фикса URL-поля (#650) #653) as-is. Does not reuse_add_repeating_values— that function's contract (clear and rewrite every slot) is the opposite of a point replacement.UsageError) — this only edits variants that already exist, it does not add new ones. Deleting a variant and editing variant weights are out of scope, tracked as follow-ups.CampaignTitles{N}.textarea/CampaignTexts{N}.textarea, 5/3 slots) are identical in shape/count to the create page's — seetests/fixtures/masters_wizard_edit_stage_b.html.Also fixes:
masters updateon DRAFT campaigns (issue #668)Found while live-verifying this PR — the two DRAFT campaigns available in the account (leftover clones from #659/#663) couldn't be used to test
update, because DRAFT edit pages have no "Сохранить кампанию" button at all, onlyCampaignFormControls.saveDraft.button/.save.button(the latter labelled "Запустить кампанию" — publishes the campaign).update_masternow detects DRAFT (via presence of thesaveDraft.buttontestid) and clicks it by default, keeping DRAFT status. New--launchflag publishes instead./edit/to the campaign's overview page, and not instantly (~5s observed) — the original immediate post-click reload raced this redirect, producing a false "did not save as requested" error even though the edit had actually saved server-side. Fixed by pollingpage.urluntil it leaves/edit/before re-verifying, mirroring the patterncopy_masteralready uses for its own post-click redirect.Test plan
pytest tests/test_masters.py -q— 240/240 pass (including 8 new DRAFT-support tests, 7_set_repeating_valuetests, and 14 newupdate/CLI tests).pytest -q— full offline suite green (2819 passed; the 62 errors are the pre-existing, unrelated VCR/aiohttp environment issue noted in prior masters PRs).black/flake8clean on all touched files.direct masters update 713231614 --headline "1=...", confirmed saved via reload, reverted to the original text the same way. Confirmed viamasters list --status allthat the campaign remained DRAFT throughout (never published).Closes #665
Closes #668