mirror of
https://github.com/Second-Hand-Friends/kleinanzeigen-bot.git
synced 2026-07-09 12:41:05 +02:00
feat(delete): add after_delete config for post-delete YAML cleanup (#1000)
## ℹ️ Description - Link to the related issue(s): Issue #994 - Adds a `deleting.after_delete` config toggle that controls what happens to the local ad YAML file after the standalone `delete` command runs. Three modes: `NONE` (default, no change), `RESET` (clear server-side metadata so the ad is eligible for republication as "new"), `DISABLE` (set `active: false` so the ad is skipped until manually re-enabled). ## 📋 Changes Summary - Added `DeletingConfig` model with `after_delete: Literal["NONE", "RESET", "DISABLE"]` field (default `"NONE"`) - Added `deleting: DeletingConfig` field to `Config` class - Added `_apply_after_delete_policy()` helper that mutates `Ad` model and dict in-place based on mode - Integrated policy in `delete_ads` with detection heuristic: cleanup runs when `delete_ad` returns `True` (200) or when `ad_cfg.id` transitions from non-None to None (404) - Policy does NOT apply during publish-time `delete_ad` call sites — only the standalone delete command - Regenerated `docs/config.default.yaml` and `schemas/config.schema.json` ### ⚙️ Type of Change Select the type(s) of change(s) included in this pull request: - [ ] 🐞 Bug fix (non-breaking change which fixes an issue) - [x] ✨ New feature (adds new functionality without breaking existing usage) - [ ] 💥 Breaking change (changes that might break existing user setups, scripts, or configurations) ## ✅ Checklist Before requesting a review, confirm the following: - [x] I have reviewed my changes to ensure they meet the project's standards. - [x] I have tested my changes and ensured that all tests pass (`pdm run test`). - [x] I have formatted the code (`pdm run format`). - [x] I have verified that linting passes (`pdm run lint`). - [x] I have updated documentation where necessary. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added configurable post-delete cleanup options to control how local ad records are handled after deletion (leave, reset metadata, or disable). * **Bug Fixes** * More accurate detection of attempted vs. successful deletions and correct incrementing of deletion counts. * Ensure metadata cleanup is skipped when no targets are found while still handling 404-style removals appropriately. * **Tests** * Added unit tests covering post-delete behaviours and edge cases. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
@@ -167,6 +167,17 @@ publishing:
|
||||
# match old ads by title when deleting (only works with BEFORE_PUBLISH)
|
||||
delete_old_ads_by_title: true
|
||||
|
||||
# ################################################################################
|
||||
# post-delete YAML cleanup configuration
|
||||
deleting:
|
||||
|
||||
# what to do with the local ad YAML after a delete attempt (applies to both 200 and 404 responses)
|
||||
# Examples (choose one):
|
||||
# • NONE
|
||||
# • RESET
|
||||
# • DISABLE
|
||||
after_delete: NONE
|
||||
|
||||
# ################################################################################
|
||||
# Browser configuration
|
||||
browser:
|
||||
|
||||
@@ -398,6 +398,28 @@
|
||||
"title": "ContactDefaults",
|
||||
"type": "object"
|
||||
},
|
||||
"DeletingConfig": {
|
||||
"properties": {
|
||||
"after_delete": {
|
||||
"default": "NONE",
|
||||
"description": "what to do with the local ad YAML after a delete attempt (applies to both 200 and 404 responses)",
|
||||
"enum": [
|
||||
"NONE",
|
||||
"RESET",
|
||||
"DISABLE"
|
||||
],
|
||||
"examples": [
|
||||
"NONE",
|
||||
"RESET",
|
||||
"DISABLE"
|
||||
],
|
||||
"title": "After Delete",
|
||||
"type": "string"
|
||||
}
|
||||
},
|
||||
"title": "DeletingConfig",
|
||||
"type": "object"
|
||||
},
|
||||
"DescriptionAffixes": {
|
||||
"deprecated": true,
|
||||
"properties": {
|
||||
@@ -818,6 +840,10 @@
|
||||
"publishing": {
|
||||
"$ref": "#/$defs/PublishingConfig"
|
||||
},
|
||||
"deleting": {
|
||||
"$ref": "#/$defs/DeletingConfig",
|
||||
"description": "post-delete YAML cleanup configuration"
|
||||
},
|
||||
"browser": {
|
||||
"$ref": "#/$defs/BrowserConfig",
|
||||
"description": "Browser configuration"
|
||||
|
||||
@@ -8,7 +8,7 @@ from dataclasses import dataclass
|
||||
from datetime import datetime
|
||||
from gettext import gettext as _
|
||||
from pathlib import Path
|
||||
from typing import Any, Final, NamedTuple, Sequence, cast
|
||||
from typing import Any, Final, Literal, NamedTuple, Sequence, cast
|
||||
|
||||
import certifi, colorama, nodriver # isort: skip
|
||||
from nodriver.core.connection import ProtocolException
|
||||
@@ -194,6 +194,50 @@ def _relative_ad_path(ad_file:str, config_file_path:str) -> str:
|
||||
return ad_file
|
||||
|
||||
|
||||
_RESET_FIELDS:Final[frozenset[str]] = frozenset({
|
||||
"id", "created_on", "updated_on", "content_hash",
|
||||
"repost_count", "price_reduction_count",
|
||||
})
|
||||
|
||||
|
||||
def _apply_after_delete_policy(
|
||||
ad_cfg:Ad,
|
||||
ad_cfg_orig:dict[str, Any],
|
||||
*,
|
||||
mode:Literal["NONE", "RESET", "DISABLE"],
|
||||
) -> bool:
|
||||
"""Apply post-delete cleanup to in-memory and dict state.
|
||||
|
||||
Called only from ``delete_ads`` — not from publish-time ``delete_ad`` call sites.
|
||||
``ad_cfg.id`` may already be ``None`` from ``delete_ad`` clearing it;
|
||||
the helper only needs to handle ``ad_cfg_orig`` and remaining ``ad_cfg`` fields.
|
||||
|
||||
Returns:
|
||||
True if state was mutated (caller should persist with ``save_dict``).
|
||||
"""
|
||||
if mode == "NONE":
|
||||
return False
|
||||
|
||||
if mode == "RESET":
|
||||
for key in _RESET_FIELDS:
|
||||
ad_cfg_orig.pop(key, None)
|
||||
# ad_cfg.id may already be None from delete_ad — setting again is harmless
|
||||
ad_cfg.id = None
|
||||
ad_cfg.created_on = None
|
||||
ad_cfg.updated_on = None
|
||||
ad_cfg.content_hash = None
|
||||
ad_cfg.repost_count = 0
|
||||
ad_cfg.price_reduction_count = 0
|
||||
return True
|
||||
|
||||
if mode == "DISABLE":
|
||||
ad_cfg.active = False
|
||||
ad_cfg_orig["active"] = False
|
||||
return True
|
||||
|
||||
return False
|
||||
|
||||
|
||||
def _get_marker_value(marker:Element) -> str:
|
||||
"""Extract and normalize a hidden image marker value from a DOM element."""
|
||||
attrs = marker.attrs
|
||||
@@ -2025,14 +2069,31 @@ class KleinanzeigenBot(WebScrapingMixin): # noqa: PLR0904
|
||||
async def delete_ads(self, ad_cfgs:list[tuple[str, Ad, dict[str, Any]]]) -> None:
|
||||
count = 0
|
||||
deleted_count = 0
|
||||
after_delete = self.config.deleting.after_delete
|
||||
|
||||
published_ads = await self._fetch_published_ads()
|
||||
|
||||
for ad_file, ad_cfg, _ad_cfg_orig in ad_cfgs:
|
||||
for ad_file, ad_cfg, ad_cfg_orig in ad_cfgs:
|
||||
count += 1
|
||||
LOG.info("Processing %s/%s: '%s' from [%s]...", count, len(ad_cfgs), ad_cfg.title, ad_file)
|
||||
if await self.delete_ad(ad_cfg, published_ads, delete_old_ads_by_title = self.config.publishing.delete_old_ads_by_title):
|
||||
|
||||
# Record pre-delete id to detect whether delete_ad attempted a deletion.
|
||||
# delete_ad clears ad_cfg.id when targets were found (Phase B ran),
|
||||
# and preserves it on no-match early return.
|
||||
id_before = ad_cfg.id
|
||||
deleted = await self.delete_ad(ad_cfg, published_ads, delete_old_ads_by_title = self.config.publishing.delete_old_ads_by_title)
|
||||
if deleted:
|
||||
deleted_count += 1
|
||||
|
||||
# Apply after_delete policy only when a delete was actually attempted.
|
||||
# Detection: True return (some 200), or id changed from non-None to None (all 404).
|
||||
# When id was already None before the call, only True return is reliable;
|
||||
# a False return could be no-match or title-match all-404, both treated as no cleanup.
|
||||
delete_attempted = deleted or (id_before is not None and ad_cfg.id is None)
|
||||
|
||||
if delete_attempted and after_delete != "NONE":
|
||||
if _apply_after_delete_policy(ad_cfg, ad_cfg_orig, mode = after_delete):
|
||||
dicts.save_dict(ad_file, ad_cfg_orig)
|
||||
await self.web_sleep()
|
||||
|
||||
LOG.info("############################################")
|
||||
|
||||
@@ -240,6 +240,14 @@ class PublishingConfig(ContextualModel):
|
||||
delete_old_ads_by_title:bool = Field(default = True, description = "match old ads by title when deleting (only works with BEFORE_PUBLISH)")
|
||||
|
||||
|
||||
class DeletingConfig(ContextualModel):
|
||||
after_delete:Literal["NONE", "RESET", "DISABLE"] = Field(
|
||||
default = "NONE",
|
||||
description = "what to do with the local ad YAML after a delete attempt (applies to both 200 and 404 responses)",
|
||||
examples = ["NONE", "RESET", "DISABLE"],
|
||||
)
|
||||
|
||||
|
||||
class CaptchaConfig(ContextualModel):
|
||||
auto_restart:bool = Field(
|
||||
default = False, description = "if true, abort when captcha is detected and auto-retry after restart_delay (if false, wait for manual solving)"
|
||||
@@ -456,6 +464,7 @@ if relative paths are specified, then they are relative to this configuration fi
|
||||
|
||||
download:DownloadConfig = Field(default_factory = DownloadConfig)
|
||||
publishing:PublishingConfig = Field(default_factory = PublishingConfig)
|
||||
deleting:DeletingConfig = Field(default_factory = DeletingConfig, description = "post-delete YAML cleanup configuration")
|
||||
browser:BrowserConfig = Field(default_factory = BrowserConfig, description = "Browser configuration")
|
||||
login:LoginConfig = Field(default_factory = LoginConfig.model_construct, description = "Login credentials")
|
||||
captcha:CaptchaConfig = Field(default_factory = CaptchaConfig)
|
||||
|
||||
@@ -404,3 +404,21 @@ def test_diagnostics_legacy_capture_migration(
|
||||
config = Config.model_validate(cfg)
|
||||
assert config.diagnostics is not None
|
||||
assert getattr(config.diagnostics.capture_on, capture_attr) == expected
|
||||
|
||||
|
||||
def test_deleting_config_defaults_to_none(minimal_config:dict[str, object]) -> None:
|
||||
config = Config.model_validate(minimal_config)
|
||||
assert config.deleting.after_delete == "NONE"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("value", ["NONE", "RESET", "DISABLE"])
|
||||
def test_deleting_config_accepts_valid_values(minimal_config:dict[str, object], value:str) -> None:
|
||||
cfg = {**minimal_config, "deleting": {"after_delete": value}}
|
||||
config = Config.model_validate(cfg)
|
||||
assert config.deleting.after_delete == value
|
||||
|
||||
|
||||
def test_deleting_config_rejects_invalid_value(minimal_config:dict[str, object]) -> None:
|
||||
cfg = {**minimal_config, "deleting": {"after_delete": "INVALID"}}
|
||||
with pytest.raises(Exception, match = "after_delete"):
|
||||
Config.model_validate(cfg)
|
||||
|
||||
@@ -13,10 +13,24 @@ import pytest
|
||||
from nodriver.core.connection import ProtocolException
|
||||
from pydantic import ValidationError
|
||||
|
||||
from kleinanzeigen_bot import LOG, SUBMISSION_MAX_RETRIES, AdUpdateStrategy, KleinanzeigenBot, LoginDetectionReason, LoginDetectionResult, misc
|
||||
from kleinanzeigen_bot import (
|
||||
LOG,
|
||||
SUBMISSION_MAX_RETRIES,
|
||||
AdUpdateStrategy,
|
||||
KleinanzeigenBot,
|
||||
LoginDetectionReason,
|
||||
LoginDetectionResult,
|
||||
misc,
|
||||
)
|
||||
from kleinanzeigen_bot._version import __version__
|
||||
from kleinanzeigen_bot.model.ad_model import Ad
|
||||
from kleinanzeigen_bot.model.config_model import AdDefaults, AutoPriceReductionConfig, Config, DiagnosticsConfig, PublishingConfig
|
||||
from kleinanzeigen_bot.model.config_model import (
|
||||
AdDefaults,
|
||||
AutoPriceReductionConfig,
|
||||
Config,
|
||||
DiagnosticsConfig,
|
||||
PublishingConfig,
|
||||
)
|
||||
from kleinanzeigen_bot.utils import dicts, loggers, xdg_paths
|
||||
from kleinanzeigen_bot.utils.exceptions import PublishedAdsFetchIncompleteError, PublishSubmissionUncertainError
|
||||
from kleinanzeigen_bot.utils.web_scraping_mixin import By, Element
|
||||
@@ -5261,3 +5275,100 @@ class TestTrackingFallback:
|
||||
result = await test_bot._try_recover_ad_id_from_redirect()
|
||||
|
||||
assert result == 11223344
|
||||
|
||||
|
||||
class TestDeleteAdsAfterDeletePolicy:
|
||||
"""Tests for delete_ads orchestration with after_delete policy integration."""
|
||||
|
||||
@staticmethod
|
||||
def _make_ad(minimal_ad_config:dict[str, Any], tmp_path:Path) -> tuple[str, Ad, dict[str, Any]]:
|
||||
ad_cfg = Ad.model_validate(minimal_ad_config | {
|
||||
"id": 12345, "active": True,
|
||||
"created_on": "2024-06-01T12:00:00", "updated_on": "2024-06-10T08:30:00",
|
||||
"content_hash": "abc123", "repost_count": 3, "price_reduction_count": 1,
|
||||
})
|
||||
return str(tmp_path / "ad.yaml"), ad_cfg, ad_cfg.model_dump()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(("mode", "expected_active", "expect_metadata_cleared"), [
|
||||
("RESET", True, True),
|
||||
("DISABLE", False, False),
|
||||
("NONE", True, False),
|
||||
])
|
||||
async def test_after_delete_policy_applied(
|
||||
self, test_bot:KleinanzeigenBot, minimal_ad_config:dict[str, Any],
|
||||
tmp_path:Path, mode:str, expected_active:bool, expect_metadata_cleared:bool,
|
||||
) -> None:
|
||||
"""Each mode applies correct mutations and persistence behavior after successful delete."""
|
||||
test_bot.config.deleting.after_delete = mode # type: ignore[assignment]
|
||||
ad_file, ad_cfg, ad_cfg_orig = self._make_ad(minimal_ad_config, tmp_path)
|
||||
|
||||
async def fake_delete(ad:Ad, _published:list[dict[str, Any]], **__:Any) -> bool:
|
||||
ad.id = None # real delete_ad clears id on attempted deletion
|
||||
return True
|
||||
|
||||
with (
|
||||
patch.object(test_bot, "_fetch_published_ads", new_callable = AsyncMock, return_value = []),
|
||||
patch.object(test_bot, "delete_ad", new_callable = AsyncMock, side_effect = fake_delete),
|
||||
patch.object(test_bot, "web_sleep", new_callable = AsyncMock),
|
||||
patch("kleinanzeigen_bot.dicts.save_dict") as mock_save,
|
||||
):
|
||||
await test_bot.delete_ads([(ad_file, ad_cfg, ad_cfg_orig)])
|
||||
|
||||
assert ad_cfg.active == expected_active
|
||||
assert ad_cfg_orig.get("active", True) == expected_active
|
||||
if expect_metadata_cleared:
|
||||
assert ad_cfg.id is None
|
||||
assert ad_cfg.repost_count == 0
|
||||
assert "id" not in ad_cfg_orig
|
||||
mock_save.assert_called_once()
|
||||
else:
|
||||
assert "id" in ad_cfg_orig
|
||||
assert ad_cfg.repost_count == 3
|
||||
if mode == "NONE":
|
||||
mock_save.assert_not_called()
|
||||
else: # DISABLE
|
||||
mock_save.assert_called_once()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_cleanup_on_404_detection(
|
||||
self, test_bot:KleinanzeigenBot, minimal_ad_config:dict[str, Any], tmp_path:Path,
|
||||
) -> None:
|
||||
"""Cleanup runs when delete_ad returns False but cleared the id (404 path)."""
|
||||
test_bot.config.deleting.after_delete = "RESET"
|
||||
ad_file, ad_cfg, ad_cfg_orig = self._make_ad(minimal_ad_config, tmp_path)
|
||||
|
||||
async def fake_delete(ad:Ad, _published:list[dict[str, Any]], **__:Any) -> bool:
|
||||
ad.id = None # Phase B ran and cleared the id
|
||||
return False # but all responses were 404
|
||||
|
||||
with (
|
||||
patch.object(test_bot, "_fetch_published_ads", new_callable = AsyncMock, return_value = []),
|
||||
patch.object(test_bot, "delete_ad", new_callable = AsyncMock, side_effect = fake_delete),
|
||||
patch.object(test_bot, "web_sleep", new_callable = AsyncMock),
|
||||
patch("kleinanzeigen_bot.dicts.save_dict") as mock_save,
|
||||
):
|
||||
await test_bot.delete_ads([(ad_file, ad_cfg, ad_cfg_orig)])
|
||||
|
||||
assert ad_cfg.repost_count == 0
|
||||
assert "id" not in ad_cfg_orig
|
||||
mock_save.assert_called_once()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_no_cleanup_when_delete_not_attempted(
|
||||
self, test_bot:KleinanzeigenBot, minimal_ad_config:dict[str, Any], tmp_path:Path,
|
||||
) -> None:
|
||||
"""No cleanup when delete_ad returns False with id preserved (no match)."""
|
||||
test_bot.config.deleting.after_delete = "RESET"
|
||||
ad_file, ad_cfg, ad_cfg_orig = self._make_ad(minimal_ad_config, tmp_path)
|
||||
|
||||
with (
|
||||
patch.object(test_bot, "_fetch_published_ads", new_callable = AsyncMock, return_value = []),
|
||||
patch.object(test_bot, "delete_ad", new_callable = AsyncMock, return_value = False),
|
||||
patch.object(test_bot, "web_sleep", new_callable = AsyncMock),
|
||||
patch("kleinanzeigen_bot.dicts.save_dict") as mock_save,
|
||||
):
|
||||
await test_bot.delete_ads([(ad_file, ad_cfg, ad_cfg_orig)])
|
||||
|
||||
mock_save.assert_not_called()
|
||||
assert ad_cfg.id == 12345
|
||||
|
||||
Reference in New Issue
Block a user