From b9bee24047f21f9b86a76682ab43d047e3f76f07 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?=
 <sascha.issbruecker@gmail.com>
Date: Sun, 9 Mar 2025 23:11:48 +0100
Subject: [PATCH] Move more logic into bookmark form

---
 bookmarks/forms.py                          | 94 +++++++++++++++++++++
 bookmarks/models.py                         | 50 -----------
 bookmarks/templates/bookmarks/edit.html     |  3 +-
 bookmarks/templates/bookmarks/form.html     |  6 +-
 bookmarks/templates/bookmarks/new.html      |  3 +-
 bookmarks/templatetags/bookmarks.py         | 18 ----
 bookmarks/tests/test_bookmark_new_view.py   |  4 +-
 bookmarks/tests/test_bookmark_validation.py | 15 +++-
 bookmarks/views/bookmarks.py                | 42 ++-------
 9 files changed, 119 insertions(+), 116 deletions(-)
 create mode 100644 bookmarks/forms.py

diff --git a/bookmarks/forms.py b/bookmarks/forms.py
new file mode 100644
index 0000000..3d3db88
--- /dev/null
+++ b/bookmarks/forms.py
@@ -0,0 +1,94 @@
+from django import forms
+
+from bookmarks.models import Bookmark, build_tag_string
+from bookmarks.validators import BookmarkURLValidator
+from bookmarks.type_defs import HttpRequest
+from bookmarks.services.bookmarks import create_bookmark, update_bookmark
+
+
+class BookmarkForm(forms.ModelForm):
+    # Use URLField for URL
+    url = forms.CharField(validators=[BookmarkURLValidator()])
+    tag_string = forms.CharField(required=False)
+    # Do not require title and description as they may be empty
+    title = forms.CharField(max_length=512, required=False)
+    description = forms.CharField(required=False, widget=forms.Textarea())
+    unread = forms.BooleanField(required=False)
+    shared = forms.BooleanField(required=False)
+    # Hidden field that determines whether to close window/tab after saving the bookmark
+    auto_close = forms.CharField(required=False)
+
+    class Meta:
+        model = Bookmark
+        fields = [
+            "url",
+            "tag_string",
+            "title",
+            "description",
+            "notes",
+            "unread",
+            "shared",
+            "auto_close",
+        ]
+
+    def __init__(self, request: HttpRequest, instance: Bookmark = None):
+        self.request = request
+
+        initial = None
+        if instance is None and request.method == "GET":
+            initial = {
+                "url": request.GET.get("url"),
+                "title": request.GET.get("title"),
+                "description": request.GET.get("description"),
+                "notes": request.GET.get("notes"),
+                "auto_close": "auto_close" in request.GET,
+                "unread": request.user_profile.default_mark_unread,
+            }
+        if instance is not None and request.method == "GET":
+            initial = {"tag_string": build_tag_string(instance.tag_names, " ")}
+        data = request.POST if request.method == "POST" else None
+        super().__init__(data, instance=instance, initial=initial)
+
+    @property
+    def is_auto_close(self):
+        return self.data.get("auto_close", False) == "True" or self.initial.get(
+            "auto_close", False
+        )
+
+    @property
+    def has_notes(self):
+        return self.initial.get("notes", None) or (
+            self.instance and self.instance.notes
+        )
+
+    def save(self, commit=False):
+        tag_string = convert_tag_string(self.data["tag_string"])
+        bookmark = super().save(commit=False)
+        if self.instance.pk:
+            return update_bookmark(bookmark, tag_string, self.request.user)
+        else:
+            return create_bookmark(bookmark, tag_string, self.request.user)
+
+    def clean_url(self):
+        # When creating a bookmark, the service logic prevents duplicate URLs by
+        # updating the existing bookmark instead, which is also communicated in
+        # the form's UI. When editing a bookmark, there is no assumption that
+        # it would update a different bookmark if the URL is a duplicate, so
+        # raise a validation error in that case.
+        url = self.cleaned_data["url"]
+        if self.instance.pk:
+            is_duplicate = (
+                Bookmark.objects.filter(owner=self.instance.owner, url=url)
+                .exclude(pk=self.instance.pk)
+                .exists()
+            )
+            if is_duplicate:
+                raise forms.ValidationError("A bookmark with this URL already exists.")
+
+        return url
+
+
+def convert_tag_string(tag_string: str):
+    # Tag strings coming from inputs are space-separated, however services.bookmarks functions expect comma-separated
+    # strings
+    return tag_string.replace(" ", ",")
diff --git a/bookmarks/models.py b/bookmarks/models.py
index 72b11d2..b11e5af 100644
--- a/bookmarks/models.py
+++ b/bookmarks/models.py
@@ -150,56 +150,6 @@ def bookmark_asset_deleted(sender, instance, **kwargs):
                 logger.error(f"Failed to delete asset file: {filepath}", exc_info=error)
 
 
-class BookmarkForm(forms.ModelForm):
-    # Use URLField for URL
-    url = forms.CharField(validators=[BookmarkURLValidator()])
-    tag_string = forms.CharField(required=False)
-    # Do not require title and description as they may be empty
-    title = forms.CharField(max_length=512, required=False)
-    description = forms.CharField(required=False, widget=forms.Textarea())
-    unread = forms.BooleanField(required=False)
-    shared = forms.BooleanField(required=False)
-    # Hidden field that determines whether to close window/tab after saving the bookmark
-    auto_close = forms.CharField(required=False)
-
-    class Meta:
-        model = Bookmark
-        fields = [
-            "url",
-            "tag_string",
-            "title",
-            "description",
-            "notes",
-            "unread",
-            "shared",
-            "auto_close",
-        ]
-
-    @property
-    def has_notes(self):
-        return self.initial.get("notes", None) or (
-            self.instance and self.instance.notes
-        )
-
-    def clean_url(self):
-        # When creating a bookmark, the service logic prevents duplicate URLs by
-        # updating the existing bookmark instead, which is also communicated in
-        # the form's UI. When editing a bookmark, there is no assumption that
-        # it would update a different bookmark if the URL is a duplicate, so
-        # raise a validation error in that case.
-        url = self.cleaned_data["url"]
-        if self.instance.pk:
-            is_duplicate = (
-                Bookmark.objects.filter(owner=self.instance.owner, url=url)
-                .exclude(pk=self.instance.pk)
-                .exists()
-            )
-            if is_duplicate:
-                raise forms.ValidationError("A bookmark with this URL already exists.")
-
-        return url
-
-
 class BookmarkSearch:
     SORT_ADDED_ASC = "added_asc"
     SORT_ADDED_DESC = "added_desc"
diff --git a/bookmarks/templates/bookmarks/edit.html b/bookmarks/templates/bookmarks/edit.html
index de9b76b..4e1b766 100644
--- a/bookmarks/templates/bookmarks/edit.html
+++ b/bookmarks/templates/bookmarks/edit.html
@@ -1,5 +1,4 @@
 {% extends 'bookmarks/layout.html' %}
-{% load bookmarks %}
 
 {% block content %}
   <div class="bookmarks-form-page">
@@ -9,7 +8,7 @@
       </div>
       <form action="{% url 'linkding:bookmarks.edit' bookmark_id %}?return_url={{ return_url|urlencode }}" method="post"
             novalidate>
-        {% bookmark_form form return_url bookmark_id %}
+        {% include 'bookmarks/form.html' %}
       </form>
     </section>
   </div>
diff --git a/bookmarks/templates/bookmarks/form.html b/bookmarks/templates/bookmarks/form.html
index f47674f..6245754 100644
--- a/bookmarks/templates/bookmarks/form.html
+++ b/bookmarks/templates/bookmarks/form.html
@@ -91,12 +91,12 @@
   {% endif %}
   <div class="divider"></div>
   <div class="form-group d-flex justify-between">
-    {% if auto_close %}
+    {% if form.is_auto_close %}
       <input type="submit" value="Save and close" class="btn btn-primary btn-wide">
     {% else %}
       <input type="submit" value="Save" class="btn btn-primary btn btn-primary btn-wide">
     {% endif %}
-    <a href="{{ cancel_url }}" class="btn">Nevermind</a>
+    <a href="{{ return_url }}" class="btn">Nevermind</a>
   </div>
   <script type="application/javascript">
     /**
@@ -112,7 +112,7 @@
       const unreadCheckbox = document.getElementById('{{ form.unread.id_for_label }}');
       const sharedCheckbox = document.getElementById('{{ form.shared.id_for_label }}');
       const bookmarkExistsHint = document.querySelector('.form-input-hint.bookmark-exists');
-      const editedBookmarkId = {{ bookmark_id }};
+      const editedBookmarkId = {{ form.instance.id|default:0 }};
       let isTitleModified = !!titleInput.value;
       let isDescriptionModified = !!descriptionInput.value;
 
diff --git a/bookmarks/templates/bookmarks/new.html b/bookmarks/templates/bookmarks/new.html
index bc2825e..6fa27a8 100644
--- a/bookmarks/templates/bookmarks/new.html
+++ b/bookmarks/templates/bookmarks/new.html
@@ -1,5 +1,4 @@
 {% extends 'bookmarks/layout.html' %}
-{% load bookmarks %}
 
 {% block content %}
   <div class="bookmarks-form-page">
@@ -8,7 +7,7 @@
         <h2>New bookmark</h2>
       </div>
       <form action="{% url 'linkding:bookmarks.new' %}" method="post" novalidate>
-        {% bookmark_form form return_url auto_close=auto_close %}
+        {% include 'bookmarks/form.html' %}
       </form>
     </section>
   </div>
diff --git a/bookmarks/templatetags/bookmarks.py b/bookmarks/templatetags/bookmarks.py
index d7706ce..560d5dc 100644
--- a/bookmarks/templatetags/bookmarks.py
+++ b/bookmarks/templatetags/bookmarks.py
@@ -3,7 +3,6 @@ from typing import List
 from django import template
 
 from bookmarks.models import (
-    BookmarkForm,
     BookmarkSearch,
     BookmarkSearchForm,
     User,
@@ -12,23 +11,6 @@ from bookmarks.models import (
 register = template.Library()
 
 
-@register.inclusion_tag("bookmarks/form.html", name="bookmark_form", takes_context=True)
-def bookmark_form(
-    context,
-    form: BookmarkForm,
-    cancel_url: str,
-    bookmark_id: int = 0,
-    auto_close: bool = False,
-):
-    return {
-        "request": context["request"],
-        "form": form,
-        "auto_close": auto_close,
-        "bookmark_id": bookmark_id,
-        "cancel_url": cancel_url,
-    }
-
-
 @register.inclusion_tag(
     "bookmarks/search.html", name="bookmark_search", takes_context=True
 )
diff --git a/bookmarks/tests/test_bookmark_new_view.py b/bookmarks/tests/test_bookmark_new_view.py
index abfee9f..eb80f4b 100644
--- a/bookmarks/tests/test_bookmark_new_view.py
+++ b/bookmarks/tests/test_bookmark_new_view.py
@@ -147,7 +147,7 @@ class BookmarkNewViewTestCase(TestCase, BookmarkFactoryMixin):
         html = response.content.decode()
 
         self.assertInHTML(
-            '<input type="hidden" name="auto_close" id="id_auto_close">',
+            '<input type="hidden" name="auto_close" value="False" id="id_auto_close">',
             html,
         )
 
@@ -169,7 +169,7 @@ class BookmarkNewViewTestCase(TestCase, BookmarkFactoryMixin):
         self.assertRedirects(response, reverse("linkding:bookmarks.index"))
 
     def test_auto_close_should_redirect_to_close_view(self):
-        form_data = self.create_form_data({"auto_close": "true"})
+        form_data = self.create_form_data({"auto_close": "True"})
 
         response = self.client.post(reverse("linkding:bookmarks.new"), form_data)
 
diff --git a/bookmarks/tests/test_bookmark_validation.py b/bookmarks/tests/test_bookmark_validation.py
index 151374f..d965a13 100644
--- a/bookmarks/tests/test_bookmark_validation.py
+++ b/bookmarks/tests/test_bookmark_validation.py
@@ -2,8 +2,10 @@ import datetime
 
 from django.core.exceptions import ValidationError
 from django.test import TestCase, override_settings
+from django.test.client import RequestFactory
 
-from bookmarks.models import BookmarkForm, Bookmark
+from bookmarks.forms import BookmarkForm
+from bookmarks.models import Bookmark
 from bookmarks.tests.helpers import BookmarkFactoryMixin
 
 ENABLED_URL_VALIDATION_TEST_CASES = [
@@ -62,12 +64,15 @@ class BookmarkValidationTestCase(TestCase, BookmarkFactoryMixin):
         self._run_bookmark_model_url_validity_checks(DISABLED_URL_VALIDATION_TEST_CASES)
 
     def test_bookmark_form_should_validate_required_fields(self):
-        form = BookmarkForm(data={"url": ""})
+        rf = RequestFactory()
+        request = rf.post("/", data={"url": ""})
+        form = BookmarkForm(request)
 
         self.assertEqual(len(form.errors), 1)
         self.assertIn("required", str(form.errors))
 
-        form = BookmarkForm(data={"url": None})
+        request = rf.post("/", data={})
+        form = BookmarkForm(request)
 
         self.assertEqual(len(form.errors), 1)
         self.assertIn("required", str(form.errors))
@@ -102,7 +107,9 @@ class BookmarkValidationTestCase(TestCase, BookmarkFactoryMixin):
     def _run_bookmark_form_url_validity_checks(self, cases):
         for case in cases:
             url, expectation = case
-            form = BookmarkForm(data={"url": url})
+            rf = RequestFactory()
+            request = rf.post("/", data={"url": url})
+            form = BookmarkForm(request)
 
             if expectation:
                 self.assertEqual(len(form.errors), 0)
diff --git a/bookmarks/views/bookmarks.py b/bookmarks/views/bookmarks.py
index 9ae2989..42062cf 100644
--- a/bookmarks/views/bookmarks.py
+++ b/bookmarks/views/bookmarks.py
@@ -12,16 +12,13 @@ from django.shortcuts import render
 from django.urls import reverse
 
 from bookmarks import queries, utils
+from bookmarks.forms import BookmarkForm
 from bookmarks.models import (
     Bookmark,
-    BookmarkForm,
     BookmarkSearch,
-    build_tag_string,
 )
 from bookmarks.services import assets as asset_actions, tasks
 from bookmarks.services.bookmarks import (
-    create_bookmark,
-    update_bookmark,
     archive_bookmark,
     archive_bookmarks,
     unarchive_bookmark,
@@ -151,37 +148,17 @@ def convert_tag_string(tag_string: str):
 
 @login_required
 def new(request: HttpRequest):
-    initial_auto_close = True if "auto_close" in request.GET else None
-
+    form = BookmarkForm(request)
     if request.method == "POST":
-        form = BookmarkForm(request.POST)
-        auto_close = form.data["auto_close"]
         if form.is_valid():
-            current_user = request.user
-            tag_string = convert_tag_string(form.data["tag_string"])
-            create_bookmark(form.save(commit=False), tag_string, current_user)
-            if auto_close:
+            form.save()
+            if form.is_auto_close:
                 return HttpResponseRedirect(reverse("linkding:bookmarks.close"))
             else:
                 return HttpResponseRedirect(reverse("linkding:bookmarks.index"))
-    else:
-        form = BookmarkForm(
-            initial={
-                "url": request.GET.get("url"),
-                "title": request.GET.get("title"),
-                "description": request.GET.get("description"),
-                "notes": request.GET.get("notes"),
-                "auto_close": initial_auto_close,
-                "unread": request.user_profile.default_mark_unread,
-            }
-        )
 
     status = 422 if request.method == "POST" and not form.is_valid() else 200
-    context = {
-        "form": form,
-        "auto_close": initial_auto_close,
-        "return_url": reverse("linkding:bookmarks.index"),
-    }
+    context = {"form": form, "return_url": reverse("linkding:bookmarks.index")}
 
     return render(request, "bookmarks/new.html", context, status=status)
 
@@ -189,20 +166,15 @@ def new(request: HttpRequest):
 @login_required
 def edit(request: HttpRequest, bookmark_id: int):
     bookmark = access.bookmark_write(request, bookmark_id)
+    form = BookmarkForm(request, instance=bookmark)
     return_url = get_safe_return_url(
         request.GET.get("return_url"), reverse("linkding:bookmarks.index")
     )
 
     if request.method == "POST":
-        form = BookmarkForm(request.POST, instance=bookmark)
         if form.is_valid():
-            tag_string = convert_tag_string(form.data["tag_string"])
-            update_bookmark(form.save(commit=False), tag_string, request.user)
+            form.save()
             return HttpResponseRedirect(return_url)
-    else:
-        form = BookmarkForm(instance=bookmark)
-
-    form.fields["tag_string"].initial = 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}