From c3a2305a5f96245add4962c241af01e9d39c70af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Mon, 23 Sep 2024 20:30:49 +0200 Subject: [PATCH] Return client error status code for invalid form submissions (#849) * Returns client error status code for invalid form submissions * fix flaky test --- bookmarks/tests/test_bookmark_edit_view.py | 13 +++++++++++++ bookmarks/tests/test_bookmark_new_view.py | 5 +++++ bookmarks/tests/test_password_change_view.py | 2 ++ bookmarks/tests/test_settings_general_view.py | 9 +++++++-- bookmarks/tests/test_singlefile_service.py | 8 +++++--- bookmarks/views/bookmarks.py | 7 +++---- siteroot/urls.py | 9 ++++++++- 7 files changed, 43 insertions(+), 10 deletions(-) diff --git a/bookmarks/tests/test_bookmark_edit_view.py b/bookmarks/tests/test_bookmark_edit_view.py index 6cc3094..18b04ec 100644 --- a/bookmarks/tests/test_bookmark_edit_view.py +++ b/bookmarks/tests/test_bookmark_edit_view.py @@ -26,6 +26,11 @@ class BookmarkEditViewTestCase(TestCase, BookmarkFactoryMixin): } return {**form_data, **overrides} + def test_should_render_successfully(self): + bookmark = self.setup_bookmark() + response = self.client.get(reverse("bookmarks:edit", args=[bookmark.id])) + self.assertEqual(response.status_code, 200) + def test_should_edit_bookmark(self): bookmark = self.setup_bookmark() form_data = self.create_form_data({"id": bookmark.id}) @@ -46,6 +51,14 @@ class BookmarkEditViewTestCase(TestCase, BookmarkFactoryMixin): self.assertEqual(tags[0].name, "editedtag1") self.assertEqual(tags[1].name, "editedtag2") + def test_should_return_422_with_invalid_form(self): + bookmark = self.setup_bookmark() + form_data = self.create_form_data({"id": bookmark.id, "url": ""}) + response = self.client.post( + reverse("bookmarks:edit", args=[bookmark.id]), form_data + ) + self.assertEqual(response.status_code, 422) + def test_should_edit_unread_state(self): bookmark = self.setup_bookmark() diff --git a/bookmarks/tests/test_bookmark_new_view.py b/bookmarks/tests/test_bookmark_new_view.py index cb18338..404d0d9 100644 --- a/bookmarks/tests/test_bookmark_new_view.py +++ b/bookmarks/tests/test_bookmark_new_view.py @@ -46,6 +46,11 @@ class BookmarkNewViewTestCase(TestCase, BookmarkFactoryMixin): self.assertEqual(tags[0].name, "tag1") self.assertEqual(tags[1].name, "tag2") + def test_should_return_422_with_invalid_form(self): + form_data = self.create_form_data({"url": ""}) + response = self.client.post(reverse("bookmarks:new"), form_data) + self.assertEqual(response.status_code, 422) + def test_should_create_new_unread_bookmark(self): form_data = self.create_form_data({"unread": True}) diff --git a/bookmarks/tests/test_password_change_view.py b/bookmarks/tests/test_password_change_view.py index 3c78973..4ba0fb2 100644 --- a/bookmarks/tests/test_password_change_view.py +++ b/bookmarks/tests/test_password_change_view.py @@ -43,6 +43,7 @@ class PasswordChangeViewTestCase(TestCase, BookmarkFactoryMixin): response = self.client.post(reverse("change_password"), form_data) + self.assertEqual(response.status_code, 422) self.assertIn("old_password", response.context_data["form"].errors) def test_should_return_error_for_mismatching_new_password(self): @@ -54,4 +55,5 @@ class PasswordChangeViewTestCase(TestCase, BookmarkFactoryMixin): response = self.client.post(reverse("change_password"), form_data) + self.assertEqual(response.status_code, 422) self.assertIn("new_password2", response.context_data["form"].errors) diff --git a/bookmarks/tests/test_settings_general_view.py b/bookmarks/tests/test_settings_general_view.py index 3aaab13..534b952 100644 --- a/bookmarks/tests/test_settings_general_view.py +++ b/bookmarks/tests/test_settings_general_view.py @@ -22,6 +22,7 @@ class SettingsGeneralViewTestCase(TestCase, BookmarkFactoryMixin): if not overrides: overrides = {} form_data = { + "update_profile": "", "theme": UserProfile.THEME_AUTO, "bookmark_date_display": UserProfile.BOOKMARK_DATE_DISPLAY_RELATIVE, "bookmark_description_display": UserProfile.BOOKMARK_DESCRIPTION_DISPLAY_INLINE, @@ -195,6 +196,12 @@ class SettingsGeneralViewTestCase(TestCase, BookmarkFactoryMixin): self.assertSuccessMessage(html, "Profile updated") + def test_update_profile_with_invalid_form_returns_422(self): + form_data = self.create_profile_form_data({"items_per_page": "-1"}) + response = self.client.post(reverse("bookmarks:settings.update"), form_data) + + self.assertEqual(response.status_code, 422) + def test_update_profile_should_not_be_called_without_respective_form_action(self): form_data = { "theme": UserProfile.THEME_DARK, @@ -217,7 +224,6 @@ class SettingsGeneralViewTestCase(TestCase, BookmarkFactoryMixin): # Enabling favicons schedules update form_data = self.create_profile_form_data( { - "update_profile": "", "enable_favicons": True, } ) @@ -331,7 +337,6 @@ class SettingsGeneralViewTestCase(TestCase, BookmarkFactoryMixin): # Enabling favicons schedules update form_data = self.create_profile_form_data( { - "update_profile": "", "enable_preview_images": True, } ) diff --git a/bookmarks/tests/test_singlefile_service.py b/bookmarks/tests/test_singlefile_service.py index c46d730..b9b0f7a 100644 --- a/bookmarks/tests/test_singlefile_service.py +++ b/bookmarks/tests/test_singlefile_service.py @@ -1,3 +1,4 @@ +import secrets import gzip import os import subprocess @@ -9,9 +10,10 @@ from bookmarks.services import singlefile class SingleFileServiceTestCase(TestCase): - html_content = "

Hello, World!

" - html_filepath = "temp.html.gz" - temp_html_filepath = "temp.html.gz.tmp" + def setUp(self): + self.html_content = "

Hello, World!

" + self.html_filepath = secrets.token_hex(8) + ".html.gz" + self.temp_html_filepath = self.html_filepath + ".tmp" def tearDown(self): if os.path.exists(self.html_filepath): diff --git a/bookmarks/views/bookmarks.py b/bookmarks/views/bookmarks.py index f8abdc9..5c110bd 100644 --- a/bookmarks/views/bookmarks.py +++ b/bookmarks/views/bookmarks.py @@ -150,7 +150,6 @@ def convert_tag_string(tag_string: str): @login_required def new(request): - status = 200 initial_url = request.GET.get("url") initial_title = request.GET.get("title") initial_description = request.GET.get("description") @@ -169,8 +168,6 @@ def new(request): return HttpResponseRedirect(reverse("bookmarks:close")) else: return HttpResponseRedirect(reverse("bookmarks:index")) - else: - status = 422 else: form = BookmarkForm() if initial_url: @@ -186,6 +183,7 @@ def new(request): if initial_mark_unread: form.initial["unread"] = "true" + status = 422 if request.method == "POST" and not form.is_valid() else 200 context = { "form": form, "auto_close": initial_auto_close, @@ -216,9 +214,10 @@ def edit(request, bookmark_id: int): form.initial["tag_string"] = build_tag_string(bookmark.tag_names, " ") + status = 422 if request.method == "POST" and not form.is_valid() else 200 context = {"form": form, "bookmark_id": bookmark_id, "return_url": return_url} - return render(request, "bookmarks/edit.html", context) + return render(request, "bookmarks/edit.html", context, status=status) def remove(request, bookmark_id: int): diff --git a/siteroot/urls.py b/siteroot/urls.py index 4b14f4f..0e29e57 100644 --- a/siteroot/urls.py +++ b/siteroot/urls.py @@ -40,6 +40,13 @@ class LinkdingLoginView(auth_views.LoginView): return response +class LinkdingPasswordChangeView(auth_views.PasswordChangeView): + def form_invalid(self, form): + response = super().form_invalid(form) + response.status_code = 422 + return response + + urlpatterns = [ path("admin/", linkding_admin_site.urls), path( @@ -50,7 +57,7 @@ urlpatterns = [ path("logout/", auth_views.LogoutView.as_view(), name="logout"), path( "change-password/", - auth_views.PasswordChangeView.as_view(), + LinkdingPasswordChangeView.as_view(), name="change_password", ), path(