mirror of
https://github.com/Second-Hand-Friends/kleinanzeigen-bot.git
synced 2026-07-09 12:41:05 +02:00
feat: show APR status indicators (#1180)
## ℹ️ Description - Link to the related issue(s): Issue # - Adds display-only APR indicators to the local status table so users can see repost/update APR state without running publish/update flows. ## 📋 Changes Summary - Adds optional `APR repost` and `APR update` columns to `kleinanzeigen-bot status` when APR is effectively enabled for at least one active ad. - Reuses existing `evaluate_auto_price_reduction()` logic for REPLACE and MODIFY modes; does not apply or mutate APR decisions. - Keeps APR output plain while preserving existing status color behavior. - Adds German translations and unit coverage for rendering, evaluator call modes, guardrails, and mutation safety. - No new dependencies or configuration changes. ### ⚙️ 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`: 1344 passed, 4 skipped). - [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** * Status tables now show optional APR-related columns for repost and update pricing decisions when available. * APR results are displayed in a compact, easy-to-read format such as due, not due, error, or off. * **Bug Fixes** * Improved status table layout and spacing so translated status text fits more reliably. * Status summaries and row rendering now handle coloured and uncoloured cells more consistently. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
Jens
GitHub
@@ -14,6 +14,8 @@ from typing import TYPE_CHECKING, Any
|
||||
import colorama
|
||||
|
||||
from . import ad_loading
|
||||
from . import price_reduction as _price_reduction
|
||||
from .model.ad_model import AdUpdateStrategy
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from .model.ad_model import Ad
|
||||
@@ -26,6 +28,8 @@ class StatusRow:
|
||||
title:str # ad.title
|
||||
ad_id:str # "-" if None, else str(ad.id)
|
||||
status:str # One of: "disabled", "draft", "changed", "due", "published-local"
|
||||
apr_repost:str | None = None # APR repost cell; ``None`` → rendered as "off"
|
||||
apr_update:str | None = None # APR update cell; ``None`` → rendered as "off"
|
||||
|
||||
|
||||
def _translate_status(status:str) -> str:
|
||||
@@ -65,6 +69,23 @@ def _colorize_status(status:str, text:str) -> str:
|
||||
return f"{prefix}{text}{colorama.Style.RESET_ALL}"
|
||||
|
||||
|
||||
def _format_apr_status(decision:_price_reduction.PriceReductionDecision) -> str | None:
|
||||
"""Format a price-reduction decision into a compact APR cell string.
|
||||
|
||||
Returns ``None`` when the decision is not effective (rendered as ``off``).
|
||||
Otherwise one of: ``due: <price>``, ``not due``, ``error``.
|
||||
"""
|
||||
if not decision.enabled:
|
||||
return None
|
||||
if decision.mode == AdUpdateStrategy.MODIFY and not decision.on_update:
|
||||
return None
|
||||
if decision.reason in {"missing_price", "calculation_failed"}:
|
||||
return _("error")
|
||||
if decision.cycle_advanced and decision.result_price is not None:
|
||||
return _("due: %s") % decision.result_price
|
||||
return _("not due")
|
||||
|
||||
|
||||
def compute_ad_status(
|
||||
ad:Ad,
|
||||
ad_cfg_orig:dict[str, Any],
|
||||
@@ -98,20 +119,68 @@ def build_status_rows(
|
||||
*,
|
||||
now:datetime | None = None,
|
||||
) -> list[StatusRow]:
|
||||
"""Build status rows from ad-file / Ad / raw-dict triples."""
|
||||
"""Build status rows from ad-file / Ad / raw-dict triples.
|
||||
|
||||
The first element of each triple is the **relative** ad file path,
|
||||
used for APR evaluation (``evaluate_auto_price_reduction``).
|
||||
"""
|
||||
rows:list[StatusRow] = []
|
||||
for _ad_file, ad_cfg, ad_cfg_orig in ads:
|
||||
for ad_file_rel, ad_cfg, ad_cfg_orig in ads:
|
||||
status = compute_ad_status(ad_cfg, ad_cfg_orig, now = now)
|
||||
|
||||
apr_repost:str | None = None
|
||||
apr_update:str | None = None
|
||||
if ad_cfg.active:
|
||||
replace_dec = _price_reduction.evaluate_auto_price_reduction(
|
||||
ad_cfg, ad_file_rel, mode = AdUpdateStrategy.REPLACE,
|
||||
)
|
||||
apr_repost = _format_apr_status(replace_dec)
|
||||
if ad_cfg.id is not None:
|
||||
modify_dec = _price_reduction.evaluate_auto_price_reduction(
|
||||
ad_cfg, ad_file_rel, mode = AdUpdateStrategy.MODIFY,
|
||||
)
|
||||
apr_update = _format_apr_status(modify_dec)
|
||||
|
||||
rows.append(
|
||||
StatusRow(
|
||||
title = ad_cfg.title,
|
||||
ad_id = "-" if ad_cfg.id is None else str(ad_cfg.id),
|
||||
status = status,
|
||||
apr_repost = apr_repost,
|
||||
apr_update = apr_update,
|
||||
)
|
||||
)
|
||||
return rows
|
||||
|
||||
|
||||
def _apr_cell(value:str | None) -> str:
|
||||
"""Return the display text for an APR cell — ``off`` when ``None``."""
|
||||
return _("off") if value is None else value
|
||||
|
||||
|
||||
def _has_apr(rows:list[StatusRow]) -> bool:
|
||||
"""Return ``True`` if any row has non-``None`` APR data (show APR columns)."""
|
||||
return any(r.apr_repost is not None or r.apr_update is not None for r in rows)
|
||||
|
||||
|
||||
def _apr_layout(rows:list[StatusRow]) -> tuple[bool, str, str, int, int]:
|
||||
"""Compute APR column layout: (show, h_repost, h_update, w_repost, w_update).
|
||||
|
||||
Returns ``show=False`` with empty strings and zero widths when no
|
||||
effective APR data exists across *rows*.
|
||||
"""
|
||||
if not _has_apr(rows):
|
||||
return False, "", "", 0, 0
|
||||
h_repost = _("APR repost")
|
||||
h_update = _("APR update")
|
||||
off = _("off")
|
||||
cells_repost = [off if r.apr_repost is None else r.apr_repost for r in rows]
|
||||
cells_update = [off if r.apr_update is None else r.apr_update for r in rows]
|
||||
w_repost = max(len(h_repost), *[len(c) for c in cells_repost], 0)
|
||||
w_update = max(len(h_update), *[len(c) for c in cells_update], 0)
|
||||
return True, h_repost, h_update, w_repost, w_update
|
||||
|
||||
|
||||
def render_status_rows(rows:list[StatusRow], *, color:bool = False) -> str:
|
||||
"""Format status rows into an ASCII table string.
|
||||
|
||||
@@ -131,65 +200,53 @@ def render_status_rows(rows:list[StatusRow], *, color:bool = False) -> str:
|
||||
col_id = max(len(h_id), max((len(r.ad_id) for r in rows), default = 0))
|
||||
|
||||
# Translated labels for column width calculation.
|
||||
# Uses ``_translate_status()`` which contains explicit ``_("...")`` calls
|
||||
# that the translation-coverage scanner can find.
|
||||
translated_statuses = [_translate_status(s) for s in _STATUS_ORDER]
|
||||
col_status = max(len(h_status), *[len(s) for s in translated_statuses], 0)
|
||||
col_status = max(len(h_status), *[len(_translate_status(s)) for s in _STATUS_ORDER], 0)
|
||||
|
||||
# Title width is data-driven, but clamp to at least header width
|
||||
col_title = max(len(h_title), max((len(r.title) for r in rows), default = 0))
|
||||
|
||||
sep = (
|
||||
"+"
|
||||
+ "-" * (col_id + 2)
|
||||
+ "+"
|
||||
+ "-" * (col_title + 2)
|
||||
+ "+"
|
||||
+ "-" * (col_status + 2)
|
||||
+ "+"
|
||||
)
|
||||
header = (
|
||||
"| "
|
||||
+ h_id.ljust(col_id)
|
||||
+ " | "
|
||||
+ h_title.ljust(col_title)
|
||||
+ " | "
|
||||
+ h_status.ljust(col_status)
|
||||
+ " |"
|
||||
)
|
||||
# APR columns — only if any row has non-None APR data
|
||||
apr_show, h_apr_r, h_apr_u, w_apr_r, w_apr_u = _apr_layout(rows)
|
||||
|
||||
lines:list[str] = [sep, header, sep]
|
||||
# Build separator and header
|
||||
separator_parts = ["+", "-" * (col_id + 2), "+", "-" * (col_title + 2), "+", "-" * (col_status + 2)]
|
||||
header_parts = ["| ", h_id.ljust(col_id), " | ", h_title.ljust(col_title), " | ", h_status.ljust(col_status)]
|
||||
if apr_show:
|
||||
separator_parts += ["+", "-" * (w_apr_r + 2), "+", "-" * (w_apr_u + 2)]
|
||||
header_parts += [" | ", h_apr_r.ljust(w_apr_r), " | ", h_apr_u.ljust(w_apr_u)]
|
||||
separator = "".join(separator_parts) + "+"
|
||||
header = "".join(header_parts) + " |"
|
||||
|
||||
lines:list[str] = [separator, header, separator]
|
||||
|
||||
for r in rows:
|
||||
label = _translate_status(r.status)
|
||||
# Pad with plain label first, then wrap in colour if enabled.
|
||||
# This keeps stripped (ANSI-free) column widths identical.
|
||||
padded = label.ljust(col_status)
|
||||
display = _colorize_status(r.status, padded) if color else padded
|
||||
lines.append(
|
||||
"| "
|
||||
+ r.ad_id.ljust(col_id)
|
||||
+ " | "
|
||||
+ r.title.ljust(col_title)
|
||||
+ " | "
|
||||
+ display
|
||||
+ " |"
|
||||
)
|
||||
label = _translate_status(r.status).ljust(col_status)
|
||||
cell = _colorize_status(r.status, label) if color else label
|
||||
|
||||
lines.append(sep)
|
||||
row_parts = [
|
||||
"| ", r.ad_id.ljust(col_id), " | ", r.title.ljust(col_title), " | ", cell,
|
||||
]
|
||||
if apr_show:
|
||||
row_parts.extend([
|
||||
" | ", _apr_cell(r.apr_repost).ljust(w_apr_r),
|
||||
" | ", _apr_cell(r.apr_update).ljust(w_apr_u),
|
||||
])
|
||||
row_parts.append(" |")
|
||||
lines.append("".join(row_parts))
|
||||
|
||||
lines.append(separator)
|
||||
|
||||
# Summary line — always plain
|
||||
counts:dict[str, int] = {}
|
||||
for r in rows:
|
||||
counts[r.status] = counts.get(r.status, 0) + 1
|
||||
|
||||
summary_parts:list[str] = [
|
||||
f"{_translate_status(label)}: {counts[label]}"
|
||||
for label in _STATUS_ORDER
|
||||
if label in counts
|
||||
]
|
||||
|
||||
summary = _("Summary: %s (%d total)") % (", ".join(summary_parts), len(rows))
|
||||
lines.append(summary)
|
||||
lines.append(
|
||||
_("Summary: %s (%d total)")
|
||||
% (", ".join(
|
||||
f"{_translate_status(s)}: {counts[s]}"
|
||||
for s in _STATUS_ORDER if s in counts
|
||||
), len(rows))
|
||||
)
|
||||
|
||||
return "\n".join(lines) + "\n"
|
||||
|
||||
@@ -264,7 +264,7 @@ class KleinanzeigenBot(WebScrapingMixin): # noqa: PLR0904
|
||||
return
|
||||
|
||||
now = _misc.now()
|
||||
ads_for_status = [(abspath, ad_cfg, raw) for abspath, _relpath, ad_cfg, raw in loaded]
|
||||
ads_for_status = [(relpath, ad_cfg, raw) for _abspath, relpath, ad_cfg, raw in loaded]
|
||||
rows = ad_status.build_status_rows(ads_for_status, now = now)
|
||||
use_color = _color.should_use_color()
|
||||
output = ad_status.render_status_rows(rows, color = use_color)
|
||||
|
||||
@@ -455,6 +455,19 @@ kleinanzeigen_bot/ad_status.py:
|
||||
"due": "Fällig"
|
||||
"published-local": "Veröffentlicht (lokal)"
|
||||
|
||||
_apr_cell:
|
||||
"off": "aus"
|
||||
|
||||
_apr_layout:
|
||||
"off": "aus"
|
||||
"APR repost": "APR-Repost"
|
||||
"APR update": "APR-Update"
|
||||
|
||||
_format_apr_status:
|
||||
"error": "Fehler"
|
||||
"due: %s": "fällig: %s"
|
||||
"not due": "nicht fällig"
|
||||
|
||||
render_status_rows:
|
||||
"Ad ID": "Anzeigen-ID"
|
||||
"Title": "Titel"
|
||||
|
||||
@@ -5,6 +5,7 @@
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import copy
|
||||
import re
|
||||
from datetime import datetime, timedelta, timezone
|
||||
from typing import Any
|
||||
@@ -12,6 +13,7 @@ from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
||||
import kleinanzeigen_bot.price_reduction as _pr_mod # noqa: PLC0414 — module import for patching in tests
|
||||
from kleinanzeigen_bot.ad_status import (
|
||||
StatusRow,
|
||||
build_status_rows,
|
||||
@@ -20,7 +22,7 @@ from kleinanzeigen_bot.ad_status import (
|
||||
)
|
||||
from kleinanzeigen_bot.app import KleinanzeigenBot
|
||||
from kleinanzeigen_bot.cli import parse_args
|
||||
from kleinanzeigen_bot.model.ad_model import Ad, AdPartial
|
||||
from kleinanzeigen_bot.model.ad_model import Ad, AdPartial, AdUpdateStrategy
|
||||
from kleinanzeigen_bot.model.config_model import Config
|
||||
from kleinanzeigen_bot.runtime_config import VALID_COMMANDS, RuntimeState
|
||||
from kleinanzeigen_bot.utils import xdg_paths
|
||||
@@ -312,6 +314,185 @@ class TestRenderStatusRows:
|
||||
assert plain == coloured, "Unmapped status should not be coloured"
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# APR column rendering
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
||||
|
||||
class TestAprRendering:
|
||||
"""APR columns: presence, cell formatting, no-colour contamination."""
|
||||
|
||||
# -- render: columns absent when no active APR ------------------------- #
|
||||
|
||||
def test_no_apr_no_columns(self) -> None:
|
||||
"""APR columns absent when no effective APR is configured."""
|
||||
rows = [
|
||||
StatusRow(title = "A", ad_id = "1", status = "published-local"),
|
||||
StatusRow(title = "B", ad_id = "2", status = "draft"),
|
||||
]
|
||||
output = render_status_rows(rows)
|
||||
assert "APR" not in output
|
||||
assert "off" not in output
|
||||
|
||||
# -- render: columns present when APR active --------------------------- #
|
||||
|
||||
def test_columns_present_when_replace_apr_enabled(self) -> None:
|
||||
"""APR columns visible when REPLACE decision has effective APR."""
|
||||
rows = [
|
||||
StatusRow(title = "A", ad_id = "1", status = "published-local", apr_repost = "due: 9"),
|
||||
]
|
||||
output = render_status_rows(rows)
|
||||
assert "APR repost" in output
|
||||
assert "APR update" in output
|
||||
assert "due: 9" in output
|
||||
|
||||
def test_columns_present_when_update_apr_enabled(self) -> None:
|
||||
"""APR columns visible when MODIFY decision has effective APR."""
|
||||
rows = [
|
||||
StatusRow(title = "A", ad_id = "1", status = "published-local", apr_update = "due: 7"),
|
||||
]
|
||||
output = render_status_rows(rows)
|
||||
assert "APR repost" in output
|
||||
assert "APR update" in output
|
||||
assert "due: 7" in output
|
||||
|
||||
# -- render: cell values ----------------------------------------------- #
|
||||
|
||||
def test_apr_cell_off_for_none(self) -> None:
|
||||
"""None APR displayed as 'off'."""
|
||||
rows = [
|
||||
StatusRow(title = "A", ad_id = "1", status = "published-local", apr_repost = "due: 5"),
|
||||
StatusRow(title = "B", ad_id = "2", status = "published-local", apr_repost = None),
|
||||
]
|
||||
output = render_status_rows(rows)
|
||||
assert "off" in output
|
||||
|
||||
def test_apr_cell_error(self) -> None:
|
||||
"""Error reason displayed as 'error'."""
|
||||
rows = [
|
||||
StatusRow(title = "A", ad_id = "1", status = "published-local", apr_repost = "error"),
|
||||
]
|
||||
output = render_status_rows(rows)
|
||||
assert "error" in output
|
||||
|
||||
def test_apr_cell_not_due(self) -> None:
|
||||
"""Not-due displayed."""
|
||||
rows = [
|
||||
StatusRow(title = "A", ad_id = "1", status = "published-local", apr_repost = "not due"),
|
||||
]
|
||||
output = render_status_rows(rows)
|
||||
assert "not due" in output
|
||||
|
||||
# -- render: APR unaffected by colour ---------------------------------- #
|
||||
|
||||
def test_apr_columns_not_coloured(self) -> None:
|
||||
"""APR columns contain no ANSI codes when status is coloured."""
|
||||
rows = [
|
||||
StatusRow(title = "A", ad_id = "1", status = "published-local", apr_repost = "due: 5", apr_update = "not due"),
|
||||
]
|
||||
output = render_status_rows(rows, color = True)
|
||||
# Status column has ANSI, but APR columns should not
|
||||
# Find the data line and split by |
|
||||
data_lines = [line for line in output.splitlines() if "|" in line and "APR" not in line and not line.startswith("+") and not line.startswith("S")]
|
||||
for line in data_lines:
|
||||
cells = [c.strip() for c in line.split("|") if c.strip()]
|
||||
# cells[0]=id, cells[1]=title, cells[2]=status (coloured), cells[3]=apr_repost, cells[4]=apr_update
|
||||
# cells[2] should have ANSI; cells[3] and cells[4] should not
|
||||
if len(cells) >= 5:
|
||||
assert "\x1b[" not in cells[3], "APR repost cell must not be coloured"
|
||||
assert "\x1b[" not in cells[4], "APR update cell must not be coloured"
|
||||
|
||||
# -- build_status_rows: APR evaluation integration --------------------- #
|
||||
# These test that evaluate_auto_price_reduction is called correctly
|
||||
# by patching it and asserting call arguments.
|
||||
|
||||
def test_apr_eval_called_for_active_published(self) -> None:
|
||||
"""Active published row calls evaluator with REPLACE and MODIFY."""
|
||||
ad = _ad(active = True, id = 1)
|
||||
raw = _raw()
|
||||
with patch.object(_pr_mod, "evaluate_auto_price_reduction") as mock_eval:
|
||||
mock_eval.return_value = _pr_mod.PriceReductionDecision(
|
||||
mode = AdUpdateStrategy.REPLACE,
|
||||
enabled = False, on_update = False,
|
||||
base_price = None, restored_price = None, result_price = None,
|
||||
applied_cycles = 0, next_cycle = None, cycle_advanced = False,
|
||||
reason = "not_configured",
|
||||
total_reposts = 0, delay_reposts = 0, eligible_cycles = 0,
|
||||
delay_days = 0, elapsed_days = None, reference = None,
|
||||
delay_reposts_ignored = False,
|
||||
)
|
||||
build_status_rows([("ads/test.yaml", ad, raw)], now = _now())
|
||||
|
||||
assert mock_eval.call_count == 2
|
||||
replace_call, modify_call = mock_eval.call_args_list
|
||||
# REPLACE call
|
||||
assert replace_call.kwargs["mode"] == AdUpdateStrategy.REPLACE
|
||||
assert replace_call.args[1] == "ads/test.yaml"
|
||||
# MODIFY call
|
||||
assert modify_call.kwargs["mode"] == AdUpdateStrategy.MODIFY
|
||||
assert modify_call.args[1] == "ads/test.yaml"
|
||||
|
||||
def test_apr_eval_not_called_for_disabled(self) -> None:
|
||||
"""Inactive/disabled row calls evaluator neither REPLACE nor MODIFY."""
|
||||
ad = _ad(active = False, id = 1)
|
||||
raw = _raw()
|
||||
with patch.object(_pr_mod, "evaluate_auto_price_reduction") as mock_eval:
|
||||
build_status_rows([("ads/test.yaml", ad, raw)], now = _now())
|
||||
mock_eval.assert_not_called()
|
||||
|
||||
def test_apr_eval_draft_calls_replace_only(self) -> None:
|
||||
"""Active draft (no id) calls REPLACE only, not MODIFY."""
|
||||
ad = _ad(active = True, id = None)
|
||||
raw = _raw()
|
||||
with patch.object(_pr_mod, "evaluate_auto_price_reduction") as mock_eval:
|
||||
mock_eval.return_value = _pr_mod.PriceReductionDecision(
|
||||
mode = AdUpdateStrategy.REPLACE,
|
||||
enabled = False, on_update = False,
|
||||
base_price = None, restored_price = None, result_price = None,
|
||||
applied_cycles = 0, next_cycle = None, cycle_advanced = False,
|
||||
reason = "not_configured",
|
||||
total_reposts = 0, delay_reposts = 0, eligible_cycles = 0,
|
||||
delay_days = 0, elapsed_days = None, reference = None,
|
||||
delay_reposts_ignored = False,
|
||||
)
|
||||
build_status_rows([("ads/test.yaml", ad, raw)], now = _now())
|
||||
|
||||
assert mock_eval.call_count == 1
|
||||
only_call = mock_eval.call_args_list[0]
|
||||
assert only_call.kwargs["mode"] == AdUpdateStrategy.REPLACE
|
||||
|
||||
def test_apr_eval_apply_not_called(self) -> None:
|
||||
"""apply_auto_price_reduction is never called during build_status_rows."""
|
||||
ad = _ad(active = True, id = 1)
|
||||
raw = _raw()
|
||||
with patch.object(_pr_mod, "apply_auto_price_reduction") as mock_apply:
|
||||
build_status_rows([("ads/test.yaml", ad, raw)], now = _now())
|
||||
mock_apply.assert_not_called()
|
||||
|
||||
def test_apr_eval_does_not_mutate_models(self) -> None:
|
||||
"""Ad model and raw dict unchanged after build_status_rows."""
|
||||
ad = _ad(
|
||||
active = True,
|
||||
id = 1,
|
||||
price = 1000,
|
||||
repost_count = 1,
|
||||
auto_price_reduction = {
|
||||
"enabled": True,
|
||||
"strategy": "PERCENTAGE",
|
||||
"amount": 10,
|
||||
"min_price": 1,
|
||||
"delay_reposts": 0,
|
||||
"delay_days": 0,
|
||||
},
|
||||
)
|
||||
raw = _raw(price = 1000)
|
||||
ad_before = ad.model_dump(mode = "json")
|
||||
raw_before = copy.deepcopy(raw)
|
||||
build_status_rows([("ads/test.yaml", ad, raw)], now = _now())
|
||||
assert ad.model_dump(mode = "json") == ad_before, "Ad model should not be mutated"
|
||||
assert raw == raw_before, "Raw dict should not be mutated"
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# Guardrail: status does not call load_ads or open browser
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
||||
Reference in New Issue
Block a user