fix: harden publishing against DOM variants - #1229
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1229 +/- ##
==========================================
- Coverage 91.65% 91.62% -0.04%
==========================================
Files 47 47
Lines 6713 6816 +103
Branches 1133 1157 +24
==========================================
+ Hits 6153 6245 +92
- Misses 379 383 +4
- Partials 181 188 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds owned-overview title handling, supports redesigned shipping dialogs, and recovers published ad IDs when confirmation pages omit them. Publishing tracks known IDs across attempts and returns resolved IDs. ChangesOwned-ad downloads
Shipping dialog navigation
Published-ad ID recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant PublishingWorkflow
participant SubmitAndConfirmAd
participant PublishedAds
participant Persistence
PublishingWorkflow->>PublishedAds: fetch strict published-ad snapshot
PublishedAds-->>PublishingWorkflow: known published IDs
PublishingWorkflow->>SubmitAndConfirmAd: submit with root URL and known IDs
SubmitAndConfirmAd->>PublishedAds: recover new exact-title ad after ID-less success
PublishedAds-->>SubmitAndConfirmAd: one resolved ad ID
SubmitAndConfirmAd-->>PublishingWorkflow: return resolved ad ID
PublishingWorkflow->>Persistence: persist resolved ad
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/kleinanzeigen_bot/publishing_workflow.py (1)
104-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the published-ad ID recovery contract at all three new surfaces. This feature introduces one shared rule: recovery only runs when a complete pre-submit snapshot exists, and it fails closed otherwise. That rule is expressed in code at three places but documented at none of them. A reader cannot tell from any single signature that
Nonemeans "recovery disabled".
src/kleinanzeigen_bot/publishing_workflow.py#L104-L122: addknown_published_ad_idsto the Args section and add a Returns section for the newintreturn value.src/kleinanzeigen_bot/publishing_workflow.py#L199-L228: add a Returns section that names the three tuple elements and states thatstrict_published_ads_listisNonewhen the complete snapshot is unavailable.src/kleinanzeigen_bot/publishing_submission.py#L53-L56: extend the docstring to state that the function returnsNonefor a missing baseline and for an ambiguous match, and to state the retry budget.As per coding guidelines: "Use docstrings for complex functions and classes that need explanation."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/kleinanzeigen_bot/publishing_workflow.py` around lines 104 - 122, The docstrings omit the published-ad ID recovery contract. In src/kleinanzeigen_bot/publishing_workflow.py lines 104-122, document known_published_ad_ids in Args and add a Returns section for the int result; in src/kleinanzeigen_bot/publishing_workflow.py lines 199-228, document all three returned tuple elements and state that strict_published_ads_list is None when no complete pre-submit snapshot exists; in src/kleinanzeigen_bot/publishing_submission.py lines 53-56, document that recovery returns None for a missing baseline or ambiguous match and specify the retry budget.Source: Coding guidelines
src/kleinanzeigen_bot/publishing_submission.py (1)
243-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReach the ID-less recovery path explicitly, not through an
IndexError.When
_check_confirmation_statedetects the ID-less success page, it returnsTrue. Control then continues to line 258 and parsesadIdfrom a URL that has noadId. That raisesIndexError, which the handler at line 260 catches, and only then does recovery run.The detected-success path therefore depends on an exception that the surrounding comment attributes to a different cause ("auto-redirected before we could poll it"). It also logs a misleading debug message. If a later change narrows the except tuple and drops
IndexError, ID-less recovery breaks silently.Branch on the flag instead.
♻️ Proposed restructure
await web.web_await(_check_confirmation_state, timeout = confirmation_timeout) - # extract the ad id from the URL's query parameter (use JS for fresh URL, not stale page url) - current_url = str(await web.web_execute("window.location.href")) - current_url_query_params = urllib_parse.parse_qs(urllib_parse.urlparse(current_url).query) - ad_id = int(current_url_query_params.get("adId", [])[0]) + if idless_success_detected: + # The redesigned success page carries no adId; recover it from the + # published-ads list instead of parsing the URL. + ad_id = await _try_recover_ad_id_from_published_ads( + web, + root_url = root_url, + title = ad_cfg.title, + known_published_ad_ids = known_published_ad_ids, + ) + if ad_id is None: + raise PublishSubmissionUncertainError( + "publish succeeded but no ad ID could be recovered" + ) + LOG.warning( + "Confirmation page exposed no ad ID; recovered ad ID %s from the published ads list", + ad_id, + ) + else: + # extract the ad id from the URL's query parameter (use JS for fresh URL, not stale page url) + current_url = str(await web.web_execute("window.location.href")) + current_url_query_params = urllib_parse.parse_qs(urllib_parse.urlparse(current_url).query) + ad_id = int(current_url_query_params.get("adId", [])[0])Keep the existing
idless_success_detectedbranch in theexceptblock for the case where detection happened but a later step still failed, or drop it if the explicit branch covers every path.Note that this changes the mocking shape used by
test_submit_recovers_id_after_explicit_idless_successintests/unit/test_publishing_submission.py; update that test accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/kleinanzeigen_bot/publishing_submission.py` around lines 243 - 258, Update the submission flow after web.web_await(_check_confirmation_state) to branch explicitly on idless_success_detected before parsing adId, and invoke the existing ID-less recovery path directly without relying on IndexError. Preserve the normal URL-based ad ID extraction for non-ID-less confirmations, and retain or remove the except-block flag handling only as needed for genuinely later failures. Update test_submit_recovers_id_after_explicit_idless_success to match the revised mocking and control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/kleinanzeigen_bot/publishing_form.py`:
- Around line 739-740: The result of _open_shipping_size_selection is discarded
before set_shipping_options; either consume the returned route to handle the
direct_size versus via_other_methods flow, or simplify
_open_shipping_size_selection to return None and remove its unused Literal
return annotation.
- Around line 743-748: Expand the docstring for _open_shipping_size_selection
with a concise example section that lists the four supported routes: direct
size, alternate methods, bounded back navigation, and timeout. Keep the example
brief and aligned with the function’s existing Literal outcomes and navigation
behavior.
- Around line 755-760: Wrap the post-click _SHIPPING_SIZE_RADIO_XPATH lookup in
web_find within the same error-handling pattern used elsewhere in the enclosing
shipping-dialog function, including the established _("Failed to configure
shipping options in dialog!") message. Keep the click and successful return path
unchanged, while converting lookup failures into that consistent configuration
error.
- Around line 38-48: Update set_shipping_options to use
_OPEN_SHIPPING_DIALOG_XPATH for its local dialog selector instead of defining a
separate XPath, ensuring all shipping-dialog lookups share the same open-dialog
filtering.
In `@src/kleinanzeigen_bot/publishing_submission.py`:
- Line 26: Annotate the module-level _PUBLISHED_AD_RECOVERY_DELAYS_MS constant
with Final using the tuple’s appropriate type, and add or reuse the Final import
from typing in publishing_submission.py.
In `@tests/unit/test_extract.py`:
- Around line 1034-1075: Add a test covering _resolve_download_title with a
cached title and owned_overview=True, asserting the cached title is returned
unchanged and _extract_title_from_ad_page is not called. Keep the existing
page-title fallback coverage intact and follow the surrounding pytest/AsyncMock
patterns.
- Around line 1710-1715: Update the assertion for mock_extract_with_dir in the
relevant test to stop requiring the explicit owned_overview=False keyword
argument, while continuing to verify the meaningful download_base, download
identifier, and active_override values.
In `@tests/unit/test_publishing_form.py`:
- Around line 1565-1599: Add a failure-path test alongside
test_navigates_back_to_direct_size_selection that makes web_probe consistently
return None across all three back-navigation iterations, then assert
_set_configured_shipping_options or the underlying _open_shipping_size_selection
raises TimeoutError with the expected unsupported-state message. Preserve the
existing successful navigation test unchanged.
In `@tests/unit/test_publishing_submission.py`:
- Around line 332-353: Add a test for _try_recover_ad_id_from_published_ads
where the first fetch_published_ads call raises PublishedAdsFetchIncompleteError
and the next call returns one matching ad; assert recovery returns that ad ID
and fetch_published_ads is awaited twice. Import
PublishedAdsFetchIncompleteError from kleinanzeigen_bot.published_ads if needed,
while preserving the existing retry and sleep setup.
- Around line 368-373: Update the web_execute mock in the submission test to use
a callable side effect that selects its return value based on the script
content, following the existing pattern in test_publishing_form.py. Replace the
call-order-dependent list while preserving the expected responses for the
referrer, confirmation URL, ID-less detection, and final URL scripts.
---
Outside diff comments:
In `@src/kleinanzeigen_bot/publishing_submission.py`:
- Around line 243-258: Update the submission flow after
web.web_await(_check_confirmation_state) to branch explicitly on
idless_success_detected before parsing adId, and invoke the existing ID-less
recovery path directly without relying on IndexError. Preserve the normal
URL-based ad ID extraction for non-ID-less confirmations, and retain or remove
the except-block flag handling only as needed for genuinely later failures.
Update test_submit_recovers_id_after_explicit_idless_success to match the
revised mocking and control flow.
In `@src/kleinanzeigen_bot/publishing_workflow.py`:
- Around line 104-122: The docstrings omit the published-ad ID recovery
contract. In src/kleinanzeigen_bot/publishing_workflow.py lines 104-122,
document known_published_ad_ids in Args and add a Returns section for the int
result; in src/kleinanzeigen_bot/publishing_workflow.py lines 199-228, document
all three returned tuple elements and state that strict_published_ads_list is
None when no complete pre-submit snapshot exists; in
src/kleinanzeigen_bot/publishing_submission.py lines 53-56, document that
recovery returns None for a missing baseline or ambiguous match and specify the
retry budget.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2c68b690-7ee8-4aef-a5a4-03e451f6f4a8
⛔ Files ignored due to path filters (1)
src/kleinanzeigen_bot/resources/translations.de.yamlis excluded by none and included by none
📒 Files selected for processing (10)
src/kleinanzeigen_bot/download_flow.pysrc/kleinanzeigen_bot/extract.pysrc/kleinanzeigen_bot/publishing_form.pysrc/kleinanzeigen_bot/publishing_submission.pysrc/kleinanzeigen_bot/publishing_workflow.pytests/unit/test_download_flow.pytests/unit/test_extract.pytests/unit/test_publishing_form.pytests/unit/test_publishing_submission.pytests/unit/test_publishing_workflow.py
| async def test_navigates_back_to_direct_size_selection( | ||
| self, | ||
| test_bot:KleinanzeigenBot, | ||
| base_ad_config:dict[str, Any], | ||
| ) -> None: | ||
| """MODIFY mode retries the tag-agnostic action after navigating back one dialog step.""" | ||
| """A nested dialog state is unwound before selecting a package size.""" | ||
| ad_cfg = self._make_ad_with_options(base_ad_config, ["DHL_5"]) | ||
| events:list[str] = [] | ||
|
|
||
| async def find_action(*_:Any, **__:Any) -> None: | ||
| events.append("find") | ||
| raise TimeoutError("action not on current step") | ||
|
|
||
| async def record_click(selector_type:By, selector_value:str, **_:Any) -> None: | ||
| if selector_type != By.XPATH: | ||
| return | ||
| if selector_value == '//button[contains(., "Zurück")]': | ||
| events.append("back") | ||
| elif selector_value == _OTHER_SHIPPING_METHODS_XPATH: | ||
| events.append("action") | ||
|
|
||
| with ( | ||
| patch.object( | ||
| test_bot, | ||
| "web_find", | ||
| "web_probe", | ||
| new_callable = AsyncMock, | ||
| side_effect = find_action, | ||
| side_effect = [None, None, MagicMock(), MagicMock()], | ||
| ), | ||
| patch.object(test_bot, "web_click", new_callable = AsyncMock, side_effect = record_click), | ||
| patch.object(test_bot, "web_click", new_callable = AsyncMock) as mock_click, | ||
| patch.object(test_bot, "web_sleep", new_callable = AsyncMock), | ||
| patch("kleinanzeigen_bot.publishing_form.set_shipping_options", new_callable = AsyncMock), | ||
| ): | ||
| await _set_configured_shipping_options( | ||
| test_bot, | ||
| ad_cfg, | ||
| AdUpdateStrategy.MODIFY, | ||
| AdUpdateStrategy.REPLACE, | ||
| test_bot.timeout("quick_dom"), | ||
| ) | ||
|
|
||
| assert events == ["find", "back", "find", "back", "action"] | ||
| assert any( | ||
| click.args == (By.XPATH, _SHIPPING_BACK_XPATH) | ||
| for click in mock_click.await_args_list | ||
| ) | ||
| assert not any( | ||
| click.args == (By.XPATH, _OTHER_SHIPPING_METHODS_XPATH) | ||
| for click in mock_click.await_args_list | ||
| ) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Good coverage for the back-navigation route. Add one more case for the failure path.
This test correctly exercises the one-back-step route to direct size selection. The PR objective also calls out reporting "unsupported states clearly" when no route is found after 2 back steps, but no test in this diff drives web_probe to return None for all three iterations to confirm _open_shipping_size_selection raises TimeoutError with the expected message.
Add a case with side_effect = [None] * 9 (3 iterations × 3 probes, minus the skipped last back-button probe) or an equivalent web_probe mock that always returns None, and assert the raised message.
As per coding guidelines, "Add or update tests when changing observable behavior, business logic, error handling, or fixing bugs."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/test_publishing_form.py` around lines 1565 - 1599, Add a
failure-path test alongside test_navigates_back_to_direct_size_selection that
makes web_probe consistently return None across all three back-navigation
iterations, then assert _set_configured_shipping_options or the underlying
_open_shipping_size_selection raises TimeoutError with the expected
unsupported-state message. Preserve the existing successful navigation test
unchanged.
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/kleinanzeigen_bot/publishing_form.py (1)
760-776: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWrap the remaining clicks for the same consistent failure message.
Line 764 wraps
web_findso a missing size radio raises the standard_("Failed to configure shipping options in dialog!")message. Line 762 (click "Andere Versandmethoden") and Line 775 (click the back button) do not have the same protection. If the probed element disappears betweenweb_probeandweb_click(DOM re-render), the rawTimeoutErrorfromweb_clickpropagates without the descriptive message, unlike every other failure path in this function.Wrap the whole loop body in one try/except instead of wrapping each call individually. This keeps the function simple and gives one consistent error surface, matching the pattern used in
set_shipping_options.🛡️ Proposed fix
max_back_steps = 2 - for back_steps in range(max_back_steps + 1): - size_radio = await web.web_probe(By.XPATH, _SHIPPING_SIZE_RADIO_XPATH, timeout = short_timeout) - if size_radio is not None: - LOG.debug("Shipping dialog route: direct size selection (%s back step(s)).", back_steps) - return - - other_methods = await web.web_probe(By.XPATH, _OTHER_SHIPPING_METHODS_XPATH, timeout = short_timeout) - if other_methods is not None: - await web.web_click(By.XPATH, _OTHER_SHIPPING_METHODS_XPATH, timeout = short_timeout) - try: - await web.web_find(By.XPATH, _SHIPPING_SIZE_RADIO_XPATH, timeout = short_timeout) - except TimeoutError as ex: - raise TimeoutError(_("Failed to configure shipping options in dialog!")) from ex - LOG.debug("Shipping dialog route: Andere Versandmethoden (%s back step(s)).", back_steps) - return - - if back_steps == max_back_steps: - break - back_button = await web.web_probe(By.XPATH, _SHIPPING_BACK_XPATH, timeout = short_timeout) - if back_button is None: - break - await web.web_click(By.XPATH, _SHIPPING_BACK_XPATH, timeout = short_timeout) - await web.web_sleep(300, 500) - - raise TimeoutError(_("Failed to configure shipping options in dialog!")) + try: + for back_steps in range(max_back_steps + 1): + size_radio = await web.web_probe(By.XPATH, _SHIPPING_SIZE_RADIO_XPATH, timeout = short_timeout) + if size_radio is not None: + LOG.debug("Shipping dialog route: direct size selection (%s back step(s)).", back_steps) + return + + other_methods = await web.web_probe(By.XPATH, _OTHER_SHIPPING_METHODS_XPATH, timeout = short_timeout) + if other_methods is not None: + await web.web_click(By.XPATH, _OTHER_SHIPPING_METHODS_XPATH, timeout = short_timeout) + await web.web_find(By.XPATH, _SHIPPING_SIZE_RADIO_XPATH, timeout = short_timeout) + LOG.debug("Shipping dialog route: Andere Versandmethoden (%s back step(s)).", back_steps) + return + + if back_steps == max_back_steps: + break + back_button = await web.web_probe(By.XPATH, _SHIPPING_BACK_XPATH, timeout = short_timeout) + if back_button is None: + break + await web.web_click(By.XPATH, _SHIPPING_BACK_XPATH, timeout = short_timeout) + await web.web_sleep(300, 500) + except TimeoutError as ex: + raise TimeoutError(_("Failed to configure shipping options in dialog!")) from ex + + raise TimeoutError(_("Failed to configure shipping options in dialog!"))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/kleinanzeigen_bot/publishing_form.py` around lines 760 - 776, Update the shipping-navigation loop in the relevant publishing-form function to wrap the entire loop body, including the “Andere Versandmethoden” click and back-button click, in one TimeoutError handler. Preserve the existing descriptive _("Failed to configure shipping options in dialog!") message and exception chaining, replacing the narrower web_find-only wrapper so all click failures use the same error surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/kleinanzeigen_bot/publishing_form.py`:
- Around line 760-776: Update the shipping-navigation loop in the relevant
publishing-form function to wrap the entire loop body, including the “Andere
Versandmethoden” click and back-button click, in one TimeoutError handler.
Preserve the existing descriptive _("Failed to configure shipping options in
dialog!") message and exception chaining, replacing the narrower web_find-only
wrapper so all click failures use the same error surface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 80fda102-8d49-48d8-b8cf-73e7bebb462e
📒 Files selected for processing (7)
src/kleinanzeigen_bot/publishing_form.pysrc/kleinanzeigen_bot/publishing_submission.pysrc/kleinanzeigen_bot/publishing_workflow.pytests/unit/test_extract.pytests/unit/test_publishing_form.pytests/unit/test_publishing_submission.pytests/unit/test_publishing_workflow.py
1 participant
ℹ️ Description
The shipping dialog can either open directly on package sizes or require "Andere Versandmethoden", and may retain a nested pane from earlier state. Separately, the redesigned success page may show "Geschafft!" and "Zu meinen Anzeigen" without an ID in the URL. Owned deleted ads can also expose a decorated page title when canonical manage-ads metadata is unavailable.
📋 Changes Summary
Gelöscht •decoration only for owned-overview page-title fallback; canonical API titles and manual downloads remain unchanged.⚙️ Type of Change
✅ Validation
pdm run formatpdm run lintpdm run test— 1501 passed, 4 skipped✅ Checklist
pdm run test).pdm run format).pdm run lint).By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Summary by CodeRabbit