Skip to content

Conversation

@hemati
Copy link

@hemati hemati commented Sep 28, 2025

ℹ️ Description

Provide a concise summary of the changes introduced in this pull request.

  • Link to the related issue(s): Issue #
  • Describe the motivation and context for this change.

📋 Changes Summary

Bullet-point key changes introduced.
Mention any dependencies, configuration changes, or additional requirements introduced.

⚙️ Type of Change

Select the type(s) of change(s) included in this pull request:

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • ✨ 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:

  • I have reviewed my changes to ensure they meet the project's standards.
  • I have tested my changes and ensured that all tests pass (pdm run test).
  • I have formatted the code (pdm run format).
  • I have verified that linting passes (pdm run lint).
  • 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.

@github-actions github-actions bot added the enhancement New feature or request label Sep 28, 2025
@codecov
Copy link

codecov bot commented Sep 28, 2025

Codecov Report

❌ Patch coverage is 22.91667% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.14%. Comparing base (a8a3f83) to head (bfbd95d).

Files with missing lines Patch % Lines
src/kleinanzeigen_bot/message.py 22.91% 37 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #632      +/-   ##
==========================================
- Coverage   79.56%   78.14%   -1.43%     
==========================================
  Files          20       21       +1     
  Lines        1860     1908      +48     
  Branches      370      372       +2     
==========================================
+ Hits         1480     1491      +11     
- Misses        243      280      +37     
  Partials      137      137              
Flag Coverage Δ
integration-tests 78.14% <22.91%> (-1.43%) ⬇️
smoke-tests 78.14% <22.91%> (-1.43%) ⬇️
unit-tests 78.14% <22.91%> (-1.43%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@1cu 1cu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required before merge:

  • Fix linting errors
  • Remove German comments and variables
  • Remove commented-out code
  • Add tests
  • Add proper docstrings

Strongly recommended

  • Improve error handling and messages
  • Fix type annotations
  • Extract command setup logic

await self.create_browser_session()
await self.login()
await self.download_ads()
case "message":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds quite a lot of new statements and the linter is failing cause of too many statements. Do avoid a larger refactor of the commands you could use a new method to handle the commands in this case. I'm unsure - what do you think @Heavenfighter ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also unsure about the command structure.

LOG:Final[loggers.Logger] = loggers.get_logger(__name__)


class Messenger(WebScrapingMixin):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty low test coverage for this one. Please add tests for send_message_to_listing(), fetch_conversation() and fetch_conversation()

# public API
# ---------------------------
async def send_message_to_listing(self, listing_url:str, message_text:str) -> bool:
# LOG.info(i18n.gettext("Opening ad page: %s"), listing_url)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not used - remove it

# LOG.info(i18n.gettext("Opening ad page: %s"), listing_url)
await self.web_open(listing_url, timeout = 15_000)

# Kleiner Scroll, damit „Kontakt“-Button gerendert ist
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

English please

send_btn_candidates, desc = "send message button", timeout = 6
)

# 4) kurze Heuristik/Abschluss
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

English please

case "message":
self.configure_file_logging()
self.load_config()
# Optional, wie bei anderen Commands:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

English please.

LOG.info("DONE: Message could not be confirmed.")
LOG.info("############################################")

case "fetch-conversations":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All commands have a similar setup - could be extracted to common setup logic.

(By.TAG_NAME, "textarea"),
]
textarea = await self._try_find(
textarea_candidates, desc = "message textarea", timeout = 8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several magic numbers that should be constants and explained.

),
]
await self._try_click(
open_btn_candidates, desc = "message open button", timeout = 6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several magic numbers that should be constants and explained.

import urllib.parse as urllib_parse
from gettext import gettext as _
from typing import Any, Final
from typing import Any, Dict, Final
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use dict

@1cu
Copy link
Contributor

1cu commented Oct 11, 2025

This PR lacks any summary or explanation in the summary...

@Heavenfighter
Copy link
Contributor

I'm unsure about the purpose of the extension. The bot is mainly supposed to post/update ads.
Where do you get the ID if you want to write a message about an ad? You can only get it manually, right?
What is the point of the downloaded messages? They are only output as JSON.
I see more of a risk here that this feature could be used as a spam generator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Development

Successfully merging this pull request may close these issues.

3 participants