diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f95b41..696ff5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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).** diff --git a/direct_cli/browser/masters.py b/direct_cli/browser/masters.py index 05b0a2e..0bccdf8 100644 --- a/direct_cli/browser/masters.py +++ b/direct_cli/browser/masters.py @@ -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 = "Черновик" @@ -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 diff --git a/tests/test_masters.py b/tests/test_masters.py index 0ea8b06..d368cdf 100644 --- a/tests/test_masters.py +++ b/tests/test_masters.py @@ -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 @@ -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 @@ -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. @@ -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="Обычная")]