From ed395a951c3545306c8095be57d213198142c9f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joakim=20Helle=C5=9Ben?= Date: Sun, 15 Mar 2026 15:13:25 +0100 Subject: [PATCH] Refactor URL change logic, mark all the new entries as read --- discord_rss_bot/main.py | 13 ++++ tests/test_main.py | 144 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+) diff --git a/discord_rss_bot/main.py b/discord_rss_bot/main.py index 7c5e7ac..050de04 100644 --- a/discord_rss_bot/main.py +++ b/discord_rss_bot/main.py @@ -734,6 +734,19 @@ async def post_change_feed_url( except ReaderError as e: raise HTTPException(status_code=400, detail=f"Failed to change feed URL: {e}") from e + # Update the feed with the new URL so we can discover what entries it returns. + # Then mark all unread entries as read so the scheduler doesn't resend them. + try: + reader.update_feed(clean_new_feed_url) + except Exception: + logger.exception("Failed to update feed after URL change: %s", clean_new_feed_url) + + for entry in reader.get_entries(feed=clean_new_feed_url, read=False): + try: + reader.set_entry_read(entry, True) + except Exception: + logger.exception("Failed to mark entry as read after URL change: %s", entry.id) + commit_state_change(reader, f"Change feed URL from {clean_old_feed_url} to {clean_new_feed_url}") return RedirectResponse(url=f"/feed?feed_url={urllib.parse.quote(clean_new_feed_url)}", status_code=303) diff --git a/tests/test_main.py b/tests/test_main.py index dc3ecf5..eee423e 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -6,9 +6,12 @@ from dataclasses import dataclass from dataclasses import field from typing import TYPE_CHECKING from typing import cast +from unittest.mock import MagicMock +from unittest.mock import patch from fastapi.testclient import TestClient +import discord_rss_bot.main as main_module from discord_rss_bot.main import app from discord_rss_bot.main import create_html_for_feed @@ -299,6 +302,147 @@ def test_change_feed_url() -> None: client.post(url="/remove", data={"feed_url": new_feed_url}) +def test_change_feed_url_marks_entries_as_read() -> None: + """After changing a feed URL all entries on the new feed should be marked read to prevent resending.""" + new_feed_url = "https://lovinator.space/rss_test_small.xml" + + # Ensure feeds do not already exist. + client.post(url="/remove", data={"feed_url": feed_url}) + client.post(url="/remove", data={"feed_url": new_feed_url}) + + # Ensure webhook exists. + client.post(url="/delete_webhook", data={"webhook_url": webhook_url}) + client.post(url="/add_webhook", data={"webhook_name": webhook_name, "webhook_url": webhook_url}) + + # Add the original feed. + response: Response = client.post(url="/add", data={"feed_url": feed_url, "webhook_dropdown": webhook_name}) + assert response.status_code == 200, f"Failed to add feed: {response.text}" + + # Patch reader on the main module so we can observe calls. + mock_entry_a = MagicMock() + mock_entry_a.id = "entry-a" + mock_entry_b = MagicMock() + mock_entry_b.id = "entry-b" + + real_reader = main_module.reader + + # Use a no-redirect client so the POST response is inspected directly; the + # redirect target (/feed?feed_url=…) would 404 because change_feed_url is mocked. + no_redirect_client = TestClient(app, follow_redirects=False) + + with ( + patch.object(real_reader, "get_entries", return_value=[mock_entry_a, mock_entry_b]) as mock_get_entries, + patch.object(real_reader, "set_entry_read") as mock_set_read, + patch.object(real_reader, "update_feed") as mock_update_feed, + patch.object(real_reader, "change_feed_url"), + ): + response = no_redirect_client.post( + url="/change_feed_url", + data={"old_feed_url": feed_url, "new_feed_url": new_feed_url}, + ) + assert response.status_code == 303, f"Expected 303 redirect, got {response.status_code}: {response.text}" + + # update_feed should have been called with the new URL. + mock_update_feed.assert_called_once_with(new_feed_url) + + # get_entries should have been called to fetch unread entries on the new URL. + mock_get_entries.assert_called_once_with(feed=new_feed_url, read=False) + + # Every returned entry should have been marked as read. + assert mock_set_read.call_count == 2, f"Expected 2 set_entry_read calls, got {mock_set_read.call_count}" + mock_set_read.assert_any_call(mock_entry_a, True) + mock_set_read.assert_any_call(mock_entry_b, True) + + # Cleanup. + client.post(url="/remove", data={"feed_url": feed_url}) + client.post(url="/remove", data={"feed_url": new_feed_url}) + + +def test_change_feed_url_empty_old_url_returns_400() -> None: + """Submitting an empty old_feed_url should return HTTP 400.""" + response: Response = client.post( + url="/change_feed_url", + data={"old_feed_url": " ", "new_feed_url": "https://example.com/feed.xml"}, + ) + assert response.status_code == 400, f"Expected 400 for empty old URL, got {response.status_code}" + + +def test_change_feed_url_empty_new_url_returns_400() -> None: + """Submitting a blank new_feed_url should return HTTP 400.""" + response: Response = client.post( + url="/change_feed_url", + data={"old_feed_url": feed_url, "new_feed_url": " "}, + ) + assert response.status_code == 400, f"Expected 400 for blank new URL, got {response.status_code}" + + +def test_change_feed_url_nonexistent_old_url_returns_404() -> None: + """Trying to rename a feed that does not exist should return HTTP 404.""" + non_existent = "https://does-not-exist.example.com/rss.xml" + # Make sure it really is absent. + client.post(url="/remove", data={"feed_url": non_existent}) + + response: Response = client.post( + url="/change_feed_url", + data={"old_feed_url": non_existent, "new_feed_url": "https://example.com/new.xml"}, + ) + assert response.status_code == 404, f"Expected 404 for non-existent feed, got {response.status_code}" + + +def test_change_feed_url_new_url_already_exists_returns_409() -> None: + """Changing to a URL that is already tracked should return HTTP 409.""" + second_feed_url = "https://lovinator.space/rss_test_small.xml" + + # Ensure both feeds are absent. + client.post(url="/remove", data={"feed_url": feed_url}) + client.post(url="/remove", data={"feed_url": second_feed_url}) + + # Ensure webhook exists. + client.post(url="/delete_webhook", data={"webhook_url": webhook_url}) + client.post(url="/add_webhook", data={"webhook_name": webhook_name, "webhook_url": webhook_url}) + + # Add both feeds. + client.post(url="/add", data={"feed_url": feed_url, "webhook_dropdown": webhook_name}) + client.post(url="/add", data={"feed_url": second_feed_url, "webhook_dropdown": webhook_name}) + + # Try to rename one to the other. + response: Response = client.post( + url="/change_feed_url", + data={"old_feed_url": feed_url, "new_feed_url": second_feed_url}, + ) + assert response.status_code == 409, f"Expected 409 when new URL already exists, got {response.status_code}" + + # Cleanup. + client.post(url="/remove", data={"feed_url": feed_url}) + client.post(url="/remove", data={"feed_url": second_feed_url}) + + +def test_change_feed_url_same_url_redirects_without_error() -> None: + """Changing a feed's URL to itself should redirect cleanly without any error.""" + # Ensure webhook exists. + client.post(url="/delete_webhook", data={"webhook_url": webhook_url}) + client.post(url="/add_webhook", data={"webhook_name": webhook_name, "webhook_url": webhook_url}) + + # Add the feed. + client.post(url="/remove", data={"feed_url": feed_url}) + response: Response = client.post(url="/add", data={"feed_url": feed_url, "webhook_dropdown": webhook_name}) + assert response.status_code == 200, f"Failed to add feed: {response.text}" + + # Submit the same URL as both old and new. + response = client.post( + url="/change_feed_url", + data={"old_feed_url": feed_url, "new_feed_url": feed_url}, + ) + assert response.status_code == 200, f"Expected 200 redirect for same URL, got {response.status_code}" + + # Feed should still be accessible. + response = client.get(url="/feed", params={"feed_url": feed_url}) + assert response.status_code == 200, f"Feed should still exist after no-op URL change: {response.text}" + + # Cleanup. + client.post(url="/remove", data={"feed_url": feed_url}) + + def test_delete_webhook() -> None: """Test the /delete_webhook page.""" # Remove the feed if it already exists before we run the test.