Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,37 @@ Both are handled by polling for a *stable* reading, mirroring

### Fixed

**`masters` status parsing — read the header status element, not the whole
page body (#848).**

`masters archive`/`suspend` refused to act on campaign 107705868 with
`Could not determine current status … (unrecognised status text) — refusing to
click blind`, while the same error's own diagnostic reported the page's
`CampaignHeader.ActionButton.stop` ("Остановить кампанию") as present and
enabled — i.e. the campaign was plainly ACTIVE. The refusal itself is correct
behaviour (never click blind), but it left the campaign unreachable by every
lifecycle command.

`_read_status_text` matched four fixed Russian phrases as substrings of
`inner_text("body")` — the entire page. Live recon 2026-08-14 established that
`CampaignHeader.Status` carries the status verbatim in *every* state, not just
DRAFT as this module's older comments assumed: confirmed on real campaigns as
"Кампания активна" (713234142), "Кампания остановлена" (107705868) and
"Кампания в\xa0архиве" (100571135). The parser now reads that element first and
falls back to body text only when it is absent or unrecognised.

Matching the header node also removes the substring-over-everything failure
mode: the dashboard renders free-form moderation prose that mentions the
campaign (confirmed live on 713234142: "…Кампания запущена из\xa0прошедших
модерацию элементов объявления"), so which status won depended on which
unrelated banners happened to be rendered. Marker comparison is additionally
normalised over case, U+00A0 and collapsed whitespace, so a marker can no
longer silently never match over an invisible character — a pitfall that had
already cost a full debugging pass twice (#704, #730).

Unrecognised text still returns `None`, so the refuse-to-click-blind guard is
unchanged.

**`masters update --add-metrika-counter` — read the suggestion popup at all,
and accept a bare counter id (#846).**

Expand Down
108 changes: 99 additions & 9 deletions direct_cli/browser/masters.py
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,9 @@
# as the edit page's DRAFT path (_DRAFT_SAVE_DRAFT_BUTTON_TESTID/
# _DRAFT_LAUNCH_BUTTON_TESTID above), plus its own header ones below.
_CAMPAIGN_HEADER_TITLE_NAME_SELECTOR = '[data-testid="CampaignHeader.TitleName"]'
# NOT DRAFT-only, despite what the neighbouring #660 comment implies: live
# recon 2026-08-14 (issue #848) confirmed this element carries the status
# verbatim in every state, which is why _read_status_text now reads it first.
_CAMPAIGN_HEADER_STATUS_SELECTOR = '[data-testid="CampaignHeader.Status"]'
_BUDGET_INPUT_SELECTOR = '[data-testid="BudgetWithSuggest.PriceTextInput"]'
_DRAFT_STATUS_TEXT = "Черновик"
Expand Down Expand Up @@ -2631,19 +2634,106 @@ def _read_status_text(page: "Page") -> Optional[str]:
was silently excluded by a status predicate that could never match. Same
non-breaking-space pitfall as "на\xa0модерации" — the space between "в"
and "архиве" is U+00A0, not ASCII.
"""

**Reads ``CampaignHeader.Status`` first, whole-body text only as a
fallback** (issue #848). Live recon 2026-08-14 on the reporting account
confirmed that testid is NOT the DRAFT-only marker this module's older
comments assumed: it carries the status verbatim in every state —
"Кампания активна" (713234142), "Кампания остановлена" (107705868),
"Кампания в\xa0архиве" (100571135) — alongside the same string in body
text. Matching the header node instead of the whole page removes the
substring-over-everything failure mode #848 hit: the dashboard also
renders free-form moderation prose mentioning the campaign (confirmed
live on 713234142: "…Кампания запущена из\xa0прошедших модерацию
элементов объявления"), so whether a status is recognised depended on
which unrelated banners happened to be rendered and in what order the
fixed if-chain tested them. The normalisation below (case, U+00A0, and
collapsed whitespace) also means a marker can no longer silently never
match over an invisible character, which has now cost two debugging
passes (#704, #730).
"""
# ``count()`` distinguishes a header element that is STRUCTURALLY ABSENT
# (this page shape has no such node — safe to fall back to body text)
# from one that is PRESENT but whose text read raised/timed out (still
# mid-hydration — a transient state, not "no such element"). Round-1's
# fix conflated both into a single ``None`` from
# ``_read_campaign_header_status_text``, which meant a slow-to-render
# header could still trip the unreliable body-text fallback during
# exactly the transition window callers are polling through (#848
# round-2 review) — count() is synchronous/non-waiting, so this adds no
# extra delay.
try:
header_present = page.locator(_CAMPAIGN_HEADER_STATUS_SELECTOR).count() > 0
except PlaywrightError:
header_present = False
if header_present:
header_text = _read_campaign_header_status_text(page)
if header_text is not None:
return _match_status_text(header_text)
# Present but unreadable (timeout) or present-but-unrecognised: the
# header is authoritative once it exists on the page at all, so
# returning None here (rather than trying body text) preserves the
# refuse-to-click-blind guard instead of risking an unrelated
# banner's marker substring. Callers already poll on their own
# short interval and will retry the header read next tick.
return None
try:
body_text = page.inner_text("body")
except PlaywrightError:
return None
if "Кампания остановлена" in body_text:
return "SUSPENDED"
if "Кампания активна" in body_text or "Кампания включена" in body_text:
return "ACTIVE"
if "Кампания на\xa0модерации" in body_text:
return "MODERATION"
if "Кампания в\xa0архиве" in body_text:
return "ARCHIVED"
return _match_status_text(body_text)


def _read_campaign_header_status_text(page: "Page") -> Optional[str]:
"""Return ``CampaignHeader.Status``'s trimmed text, or ``None``.

Uses an explicit short ``timeout=`` (matching this file's convention for
other probe reads, e.g. the ``timeout=1_000`` calls elsewhere in this
module) rather than Playwright's ~30s default. Every caller of
``_read_status_text`` polls on a much shorter deadline
(``_STATUS_CHANGE_TIMEOUT_MS`` = 8s, retried up to
``_STATUS_CLICK_MAX_ATTEMPTS`` times); an absent/not-yet-rendered header
element paying the default wait would single-handedly blow that budget
on every poll iteration (#848 review).
"""
try:
text = page.locator(_CAMPAIGN_HEADER_STATUS_SELECTOR).first.inner_text(
timeout=1_000
)
except PlaywrightError:
return None
text = (text or "").strip()
return text or None


def _normalise_status_text(text: str) -> str:
"""Casefold, turn U+00A0 into a plain space, and collapse whitespace.

The markers below differ from what Yandex renders only by invisible
characters often enough that matching them literally is a standing
liability — see ``_read_status_text``'s note on #704/#730.
"""
return re.sub(r"\s+", " ", text.replace("\xa0", " ")).strip().casefold()


# Status marker -> the CLI's Status vocabulary. Ordered most specific first:
# _match_status_text returns the first marker contained in the text, and
# "Кампания остановлена" must not be shadowed by a looser marker.
_STATUS_TEXT_MARKERS: Tuple[Tuple[str, str], ...] = (
("Кампания остановлена", "SUSPENDED"),
("Кампания активна", "ACTIVE"),
("Кампания включена", "ACTIVE"),
("Кампания на модерации", "MODERATION"),
("Кампания в архиве", "ARCHIVED"),
)


def _match_status_text(text: str) -> Optional[str]:
"""Return the CLI status whose marker appears in ``text``, else ``None``."""
normalised = _normalise_status_text(text)
for marker, status in _STATUS_TEXT_MARKERS:
if _normalise_status_text(marker) in normalised:
return status
return None


Expand Down
168 changes: 166 additions & 2 deletions tests/test_masters.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,13 @@ def __init__(
# Every `timeout=` this handle's click() was called with, in order
# (issue #779 review) — see click().
self.click_timeouts = []
# Every `timeout=` this handle's inner_text() was called with, in
# order (issue #848 review) — an absent header probe that omits an
# explicit timeout pays Playwright's ~30s default before falling
# through, defeating callers' much shorter poll budgets; this is the
# only observable an offline fake has for that cost, mirroring
# click_timeouts above.
self.inner_text_timeouts = []
# {selector: _FakeLocatorHandle} for a SCOPED child lookup — models
# Playwright's Locator.locator(), e.g. `label.locator("xpath=.//input
# [...]")` (issue #656: _set_region reads a checkbox scoped off the
Expand Down Expand Up @@ -184,6 +191,7 @@ def get_by_role(self, role, name=None, exact=False):
return _FakeGetByTextLocator(matched)

def inner_text(self, timeout=None):
self.inner_text_timeouts.append(timeout)
if self._raises:
# Real Playwright raises its own Error (a TimeoutError subclass) when
# an element is missing — masters.py's `except PlaywrightError` must
Expand Down Expand Up @@ -2823,6 +2831,158 @@ def inner_text(self):
self.assertGreaterEqual(warn.call_count, 3) # name, status, landing, stats


class TestReadStatusText(unittest.TestCase):
"""Issue #848: `masters archive`/`suspend` refused to act on campaign
107705868 with "unrecognised status text", even though the page's own
"Остановить кампанию" button was rendered and enabled.

Live recon 2026-08-14 established that `CampaignHeader.Status` carries
the status verbatim in EVERY non-DRAFT state (confirmed on real
campaigns: "Кампания активна"/713234142, "Кампания
остановлена"/107705868, "Кампания в\xa0архиве"/100571135) — the module's
older comments wrongly treated that testid as a DRAFT-only marker, so
the parser matched substrings against the whole page body instead. These
tests pin the header-first read and the whitespace/case normalisation
that make an invisible character unable to defeat a marker again.
"""

def _page(self, *, header=None, body="", header_raises=False):
locators = {}
if header_raises:
# Models an element that IS present (Locator.count() == 1) but
# whose .inner_text() raises — e.g. a timeout mid-hydration —
# as distinct from a structurally absent element
# (Locator.count() == 0, no handles at all).
locators[browser_masters._CAMPAIGN_HEADER_STATUS_SELECTOR] = _FakeLocator(
[_FakeLocatorHandle(raises=True)]
)
elif header is not None:
locators[browser_masters._CAMPAIGN_HEADER_STATUS_SELECTOR] = _FakeLocator(
[_FakeLocatorHandle(text=header)]
)
return FakePage(locators=locators, body_text=body)

def test_reads_header_status_element(self):
page = self._page(header="Кампания активна", body="")
self.assertEqual(browser_masters._read_status_text(page), "ACTIVE")

def test_header_wins_over_unrelated_body_prose(self):
# The live ACTIVE dashboard also renders free-form moderation prose
# mentioning the campaign ("... Кампания запущена из\xa0прошедших
# модерацию элементов объявления", confirmed on 713234142). Body-wide
# substring matching is what made recognition depend on which
# banners happened to render; the header node is authoritative.
page = self._page(
header="Кампания остановлена",
body=(
"Модерация завершена: отклонён 1 элемент объявления. "
"Кампания запущена из\xa0прошедших модерацию элементов"
),
)
self.assertEqual(browser_masters._read_status_text(page), "SUSPENDED")

def test_falls_back_to_body_when_header_absent(self):
# Older/other page shapes that have no CampaignHeader.Status must
# keep working exactly as before.
page = self._page(header=None, body="Кампания в\xa0архиве")
self.assertEqual(browser_masters._read_status_text(page), "ARCHIVED")

def test_unrecognised_header_returns_none_even_with_matching_body(self):
# Superseded by the round-2 fix (#848 review): a header element that
# WAS read but matches no known marker ("Черновик" is a DRAFT
# marker, not one of _STATUS_TEXT_MARKERS) must not fall through to
# body-text matching even when the body happens to contain a real
# marker — see test_unrecognised_header_does_not_fall_back_to_noisy_body
# for why: the header is authoritative once read, so this returns
# None rather than trusting an unrelated body match.
page = self._page(header="Черновик", body="Кампания на\xa0модерации")
self.assertIsNone(browser_masters._read_status_text(page))

def test_ascii_space_variant_matches(self):
# #704 and #730 each cost a debugging pass because a marker differed
# from the rendered text by a U+00A0. Normalisation makes both the
# ASCII and the non-breaking form match either way.
for text in ("Кампания на модерации", "Кампания на\xa0модерации"):
with self.subTest(text=text):
self.assertEqual(
browser_masters._read_status_text(self._page(header=text)),
"MODERATION",
)

def test_case_and_extra_whitespace_tolerated(self):
page = self._page(header=" КАМПАНИЯ\n\n АКТИВНА ")
self.assertEqual(browser_masters._read_status_text(page), "ACTIVE")

def test_unrecognised_text_still_returns_none(self):
# The defensive refuse-to-click-blind behaviour #848 praised must
# survive: a genuinely unknown status is None, never a guess.
page = self._page(header="Нечто совершенно новое", body="Черновик")
self.assertIsNone(browser_masters._read_status_text(page))

def test_unrecognised_header_does_not_fall_back_to_noisy_body(self):
# Round-2 review (#848): when the header element IS present but its
# text matches no known marker, falling through to whole-body
# matching reopens the exact substring-over-everything failure mode
# this fix was meant to close — the header is authoritative, so an
# unrecognised header text must return None, not whatever an
# unrelated banner in the body happens to contain. The body below
# deliberately DOES contain a real marker substring (in unrelated
# moderation prose about a sibling ad, not the campaign's own
# status) so a test that still fell through to body matching would
# observe MODERATION here instead of None.
page = self._page(
header="Совершенно новый статус",
body="Одно из объявлений отклонено, кампания на модерации ожидает решения.",
)
self.assertIsNone(browser_masters._read_status_text(page))

def test_present_but_timed_out_header_does_not_fall_back_to_noisy_body(self):
# Round-2 review (#848): a header element that IS present
# (Locator.count() == 1) but whose read raises/times out mid-
# hydration must be treated the same as "present but unrecognised"
# (see test_unrecognised_header_does_not_fall_back_to_noisy_body),
# NOT the same as "structurally absent" — round-1's fix conflated
# the two by only checking `header_text is None`, which a timeout
# also produces. A present-but-slow element must not fall back to
# body-text matching even when the body happens to contain a real
# marker for an unrelated banner, since that reopens the exact
# false-positive-status bug this PR set out to close, just via a
# transient hydration race instead of a permanently-absent header.
page = self._page(
header_raises=True,
body="Одно из объявлений отклонено, кампания на модерации ожидает решения.",
)
self.assertIsNone(browser_masters._read_status_text(page))

def test_structurally_absent_header_still_falls_back_to_body(self):
# Companion to the above: a page shape that genuinely has NO
# CampaignHeader.Status element at all (Locator.count() == 0, no
# handles registered) must still fall back to body-text matching —
# this is the pre-#848 legacy path and must keep working.
page = self._page(header=None, body="Кампания в\xa0архиве")
self.assertEqual(browser_masters._read_status_text(page), "ARCHIVED")

def test_header_probe_uses_a_bounded_timeout(self):
# Round-2 review (#848): an absent/not-yet-rendered header element
# must not pay Playwright's ~30s default action timeout — every
# caller of _read_status_text polls on a much shorter deadline
# (_STATUS_CHANGE_TIMEOUT_MS = 8s), so an unbounded read here can
# single-handedly blow that budget. Mirrors the existing
# click_timeouts convention (issue #779 review) for the same reason:
# the fake raises/returns instantly, so the passed `timeout=` is the
# only observable an offline test has for this cost.
handle = _FakeLocatorHandle(text="Кампания активна")
locators = {
browser_masters._CAMPAIGN_HEADER_STATUS_SELECTOR: _FakeLocator([handle])
}
page = FakePage(locators=locators, body_text="")
browser_masters._read_status_text(page)
self.assertEqual(len(handle.inner_text_timeouts), 1)
(timeout,) = handle.inner_text_timeouts
self.assertIsNotNone(timeout)
self.assertLessEqual(timeout, 1_000)


class TestFetchMasterDraft(unittest.TestCase):
"""DRAFT overview-page parsing (issue #660): name/status/weekly budget only.

Expand Down Expand Up @@ -2904,11 +3064,15 @@ def test_draft_partial_result_on_missing_name_and_budget(self):
def test_non_draft_page_unaffected(self):
# A page whose CampaignHeader.Status reads something other than
# "Черновик" must fall through to the normal dashboard extractors,
# not be misdetected as a draft.
# not be misdetected as a draft. Uses the real live-confirmed header
# text (#848) rather than a shortened stand-in — since the round-2
# fix, an unrecognised-but-present header no longer falls back to
# body-text matching, so the fixture must carry a marker the header
# read itself can match.
page = FakePage(
locators={
browser_masters._CAMPAIGN_HEADER_STATUS_SELECTOR: _FakeLocator(
[_FakeLocatorHandle(text="Активна")]
[_FakeLocatorHandle(text="Кампания активна")]
),
"h1, [role=heading]": _FakeLocator(
[_FakeLocatorHandle(text="Обычная")]
Expand Down
Loading