fix: rename form-saving methods etc. for consistency in MasterView

also adds redirect methods where missing

src/wuttaweb/views/batch.py

@@ -269,7 +269,7 @@ class BatchMasterView(MasterView):
         # otherwise normal logic is fine
         return super().objectify(form)
 
-    def redirect_after_create(self, obj):
+    def redirect_after_create(self, result):
         """
         If the new batch requires initial population, we launch a
         thread for that and show the "progress" page.
@@ -277,7 +277,7 @@ class BatchMasterView(MasterView):
         Otherwise this will do the normal thing of redirecting to the
         "view" page for the new batch.
         """
-        batch = obj
+        batch = result
 
         # just view batch if should not populate
         if not self.batch_handler.should_populate(batch):

src/wuttaweb/views/master.py

@@ -29,6 +29,7 @@ import datetime
 import logging
 import os
 import threading
+import warnings
 
 import sqlalchemy as sa
 from sqlalchemy import orm
@@ -560,66 +561,80 @@ class MasterView(View):  # pylint: disable=too-many-public-methods
         """
         View to "create" a new model record.
 
-        This usually corresponds to a URL like ``/widgets/new``.
+        This usually corresponds to URL like ``/widgets/new``
 
-        By default, this view is included only if :attr:`creatable` is
+        By default, this route is included only if :attr:`creatable`
-        true.
+        is true.
 
-        The default "create" view logic will show a form with field
+        The default logic calls :meth:`make_create_form()` and shows
-        widgets, allowing user to submit new values which are then
+        that to the user.  When they submit valid data, it calls
-        persisted to the DB (assuming typical SQLAlchemy model).
+        :meth:`save_create_form()` and then
-
+        :meth:`redirect_after_create()`.
-        Subclass normally should not override this method, but rather
-        one of the related methods which are called (in)directly by
-        this one:
-
-        * :meth:`make_model_form()`
-        * :meth:`configure_form()`
-        * :meth:`create_save_form()`
-        * :meth:`redirect_after_create()`
         """
         self.creating = True
-        session = self.Session()
+        form = self.make_create_form()
-        form = self.make_model_form(cancel_url_fallback=self.get_index_url())
 
         if form.validate():
-            obj = self.create_save_form(form)
+            try:
-            session.flush()
+                result = self.save_create_form(form)
-            return self.redirect_after_create(obj)
+            except Exception as err:  # pylint: disable=broad-exception-caught
+                log.warning("failed to save 'create' form", exc_info=True)
+                self.request.session.flash(f"Create failed: {err}", "error")
+            else:
+                return self.redirect_after_create(result)
 
-        context = {
+        context = {"form": form}
-            "form": form,
-        }
         return self.render_to_response("create", context)
 
-    def create_save_form(self, form):
+    def make_create_form(self):
         """
-        This method is responsible for "converting" the validated form
+        Make the "create" model form.  This is called by
-        data to a model instance, and then "saving" the result,
+        :meth:`create()`.
-        e.g. to DB.  It is called by :meth:`create()`.
 
-        Subclass may override this, or any of the related methods
+        Default logic calls :meth:`make_model_form()`.
-        called by this one:
 
-        * :meth:`objectify()`
+        :returns: :class:`~wuttaweb.forms.base.Form` instance
-        * :meth:`persist()`
-
-        :returns: Should return the resulting model instance, e.g. as
-           produced by :meth:`objectify()`.
         """
+        return self.make_model_form(cancel_url_fallback=self.get_index_url())
+
+    def save_create_form(self, form):
+        """
+        Save the "create" form.  This is called by :meth:`create()`.
+
+        Default logic calls :meth:`objectify()` and then
+        :meth:`persist()`.  Subclass is expected to override for
+        non-standard use cases.
+
+        As for return value, by default it will be whatever came back
+        from the ``objectify()`` call.  In practice a subclass can
+        return whatever it likes.  The value is only used as input to
+        :meth:`redirect_after_create()`.
+
+        :returns: Usually the model instance, but can be "anything"
+        """
+        if hasattr(self, "create_save_form"):  # pragma: no cover
+            warnings.warn(
+                "MasterView.create_save_form() method name is deprecated; "
+                f"please refactor to save_create_form() instead for {self.__class__.__name__}",
+                DeprecationWarning,
+            )
+            return self.create_save_form(form)
+
         obj = self.objectify(form)
         self.persist(obj)
         return obj
 
-    def redirect_after_create(self, obj):
+    def redirect_after_create(self, result):
         """
-        Usually, this returns a redirect to which we send the user,
+        Must return a redirect, following successful save of the
-        after a new model record has been created.  By default this
+        "create" form.  This is called by :meth:`create()`.
-        sends them to the "view" page for the record.
 
-        It is called automatically by :meth:`create()`.
+        By default this redirects to the "view" page for the new
+        record.
+
+        :returns: :class:`~pyramid.httpexceptions.HTTPFound` instance
         """
-        return self.redirect(self.get_action_url("view", obj))
+        return self.redirect(self.get_action_url("view", result))
 
     ##############################
     # view methods
@@ -627,32 +642,30 @@ class MasterView(View):  # pylint: disable=too-many-public-methods
 
     def view(self):
         """
-        View to "view" details of an existing model record.
+        View to "view" a model record.
 
-        This usually corresponds to a URL like ``/widgets/XXX``
+        This usually corresponds to URL like ``/widgets/XXX``
-        where ``XXX`` represents the key/ID for the record.
 
-        By default, this view is included only if :attr:`viewable` is
+        By default, this route is included only if :attr:`viewable` is
         true.
 
-        The default view logic will show a read-only form with field
+        The default logic here is as follows:
-        values displayed.
 
-        Subclass normally should not override this method, but rather
+        First, if :attr:`has_rows` is true then
-        one of the related methods which are called (in)directly by
+        :meth:`make_row_model_grid()` is called.
-        this one:
 
-        * :meth:`make_model_form()`
+        If ``has_rows`` is true *and* the request has certain special
-        * :meth:`configure_form()`
+        params relating to the grid, control may exit early.  Mainly
-        * :meth:`make_row_model_grid()` - if :attr:`has_rows` is true
+        this happens when a "partial" page is requested, which means
+        we just return grid data and nothing else.  (Used for backend
+        sorting and pagination etc.)
+
+        Otherwise :meth:`make_view_form()` is called, and the template
+        is rendered.
         """
         self.viewing = True
         obj = self.get_instance()
-        form = self.make_model_form(obj, readonly=True)
+        context = {"instance": obj}
-        context = {
-            "instance": obj,
-            "form": form,
-        }
 
         if self.has_rows:
 
@@ -677,47 +690,51 @@ class MasterView(View):  # pylint: disable=too-many-public-methods
 
             context["rows_grid"] = grid
 
+        context["form"] = self.make_view_form(obj)
         context["xref_buttons"] = self.get_xref_buttons(obj)
-
         return self.render_to_response("view", context)
 
+    def make_view_form(self, obj, readonly=True):
+        """
+        Make the "view" model form.  This is called by
+        :meth:`view()`.
+
+        Default logic calls :meth:`make_model_form()`.
+
+        :returns: :class:`~wuttaweb.forms.base.Form` instance
+        """
+        return self.make_model_form(obj, readonly=readonly)
+
     ##############################
     # edit methods
     ##############################
 
     def edit(self):
         """
-        View to "edit" details of an existing model record.
+        View to "edit" a model record.
 
-        This usually corresponds to a URL like ``/widgets/XXX/edit``
+        This usually corresponds to URL like ``/widgets/XXX/edit``
-        where ``XXX`` represents the key/ID for the record.
 
-        By default, this view is included only if :attr:`editable` is
+        By default, this route is included only if :attr:`editable` is
         true.
 
-        The default "edit" view logic will show a form with field
+        The default logic calls :meth:`make_edit_form()` and shows
-        widgets, allowing user to modify and submit new values which
+        that to the user.  When they submit valid data, it calls
-        are then persisted to the DB (assuming typical SQLAlchemy
+        :meth:`save_edit_form()` and then
-        model).
+        :meth:`redirect_after_edit()`.
-
-        Subclass normally should not override this method, but rather
-        one of the related methods which are called (in)directly by
-        this one:
-
-        * :meth:`make_model_form()`
-        * :meth:`configure_form()`
-        * :meth:`edit_save_form()`
         """
         self.editing = True
         instance = self.get_instance()
-
+        form = self.make_edit_form(instance)
-        form = self.make_model_form(
-            instance, cancel_url_fallback=self.get_action_url("view", instance)
-        )
 
         if form.validate():
-            self.edit_save_form(form)
+            try:
-            return self.redirect(self.get_action_url("view", instance))
+                result = self.save_edit_form(form)
+            except Exception as err:  # pylint: disable=broad-exception-caught
+                log.warning("failed to save 'edit' form", exc_info=True)
+                self.request.session.flash(f"Edit failed: {err}", "error")
+            else:
+                return self.redirect_after_edit(result)
 
         context = {
             "instance": instance,
@@ -725,51 +742,74 @@ class MasterView(View):  # pylint: disable=too-many-public-methods
         }
         return self.render_to_response("edit", context)
 
-    def edit_save_form(self, form):
+    def make_edit_form(self, obj):
         """
-        This method is responsible for "converting" the validated form
+        Make the "edit" model form.  This is called by
-        data to a model instance, and then "saving" the result,
+        :meth:`edit()`.
-        e.g. to DB.  It is called by :meth:`edit()`.
 
-        Subclass may override this, or any of the related methods
+        Default logic calls :meth:`make_model_form()`.
-        called by this one:
 
-        * :meth:`objectify()`
+        :returns: :class:`~wuttaweb.forms.base.Form` instance
-        * :meth:`persist()`
-
-        :returns: Should return the resulting model instance, e.g. as
-           produced by :meth:`objectify()`.
         """
+        return self.make_model_form(
+            obj, cancel_url_fallback=self.get_action_url("view", obj)
+        )
+
+    def save_edit_form(self, form):
+        """
+        Save the "edit" form.  This is called by :meth:`edit()`.
+
+        Default logic calls :meth:`objectify()` and then
+        :meth:`persist()`.  Subclass is expected to override for
+        non-standard use cases.
+
+        As for return value, by default it will be whatever came back
+        from the ``objectify()`` call.  In practice a subclass can
+        return whatever it likes.  The value is only used as input to
+        :meth:`redirect_after_edit()`.
+
+        :returns: Usually the model instance, but can be "anything"
+        """
+        if hasattr(self, "edit_save_form"):  # pragma: no cover
+            warnings.warn(
+                "MasterView.edit_save_form() method name is deprecated; "
+                f"please refactor to save_edit_form() instead for {self.__class__.__name__}",
+                DeprecationWarning,
+            )
+            return self.edit_save_form(form)
+
         obj = self.objectify(form)
         self.persist(obj)
         return obj
 
+    def redirect_after_edit(self, result):
+        """
+        Must return a redirect, following successful save of the
+        "edit" form.  This is called by :meth:`edit()`.
+
+        By default this redirects to the "view" page for the record.
+
+        :returns: :class:`~pyramid.httpexceptions.HTTPFound` instance
+        """
+        return self.redirect(self.get_action_url("view", result))
+
     ##############################
     # delete methods
     ##############################
 
     def delete(self):
         """
-        View to delete an existing model instance.
+        View to "delete" a model record.
 
-        This usually corresponds to a URL like ``/widgets/XXX/delete``
+        This usually corresponds to URL like ``/widgets/XXX/delete``
-        where ``XXX`` represents the key/ID for the record.
 
-        By default, this view is included only if :attr:`deletable` is
+        By default, this route is included only if :attr:`deletable`
-        true.
+        is true.
 
-        The default "delete" view logic will show a "psuedo-readonly"
+        The default logic calls :meth:`make_delete_form()` and shows
-        form with no fields editable, but with a submit button so user
+        that to the user.  When they submit, it calls
-        must confirm, before deletion actually occurs.
+        :meth:`save_delete_form()` and then
-
+        :meth:`redirect_after_delete()`.
-        Subclass normally should not override this method, but rather
-        one of the related methods which are called (in)directly by
-        this one:
-
-        * :meth:`make_model_form()`
-        * :meth:`configure_form()`
-        * :meth:`delete_save_form()`
-        * :meth:`delete_instance()`
         """
         self.deleting = True
         instance = self.get_instance()
@@ -777,21 +817,20 @@ class MasterView(View):  # pylint: disable=too-many-public-methods
         if not self.is_deletable(instance):
             return self.redirect(self.get_action_url("view", instance))
 
-        # nb. this form proper is not readonly..
+        form = self.make_delete_form(instance)
-        form = self.make_model_form(
-            instance,
-            cancel_url_fallback=self.get_action_url("view", instance),
-            button_label_submit="DELETE Forever",
-            button_icon_submit="trash",
-            button_type_submit="is-danger",
-        )
-        # ..but *all* fields are readonly
-        form.readonly_fields = set(form.fields)
 
         # nb. validate() often returns empty dict here
         if form.validate() is not False:
-            self.delete_save_form(form)
+
-            return self.redirect(self.get_index_url())
+            try:
+                result = self.save_delete_form(  # pylint: disable=assignment-from-none
+                    form
+                )
+            except Exception as err:  # pylint: disable=broad-exception-caught
+                log.warning("failed to save 'delete' form", exc_info=True)
+                self.request.session.flash(f"Delete failed: {err}", "error")
+            else:
+                return self.redirect_after_delete(result)
 
         context = {
             "instance": instance,
@@ -799,19 +838,70 @@ class MasterView(View):  # pylint: disable=too-many-public-methods
         }
         return self.render_to_response("delete", context)
 
-    def delete_save_form(self, form):
+    def make_delete_form(self, obj):
         """
-        Perform the delete operation(s) based on the given form data.
+        Make the "delete" model form.  This is called by
+        :meth:`delete()`.
 
-        Default logic simply calls :meth:`delete_instance()` on the
+        Default logic calls :meth:`make_model_form()` but with a
-        form's :attr:`~wuttaweb.forms.base.Form.model_instance`.
+        twist:
 
-        This method is called by :meth:`delete()` after it has
+        The form proper is *not* readonly; this ensures the form has a
-        validated the form.
+        submit button etc.  But then all fields in the form are
+        explicitly marked readonly.
+
+        :returns: :class:`~wuttaweb.forms.base.Form` instance
         """
+        # nb. this form proper is not readonly..
+        form = self.make_model_form(
+            obj,
+            cancel_url_fallback=self.get_action_url("view", obj),
+            button_label_submit="DELETE Forever",
+            button_icon_submit="trash",
+            button_type_submit="is-danger",
+        )
+
+        # ..but *all* fields are readonly
+        form.readonly_fields = set(form.fields)
+        return form
+
+    def save_delete_form(self, form):
+        """
+        Save the "delete" form.  This is called by :meth:`delete()`.
+
+        Default logic calls :meth:`delete_instance()`.  Normally
+        subclass would override that for non-standard use cases, but
+        it could also/instead override this method.
+
+        As for return value, by default this returns ``None``.  In
+        practice a subclass can return whatever it likes.  The value
+        is only used as input to :meth:`redirect_after_delete()`.
+
+        :returns: Usually ``None``, but can be "anything"
+        """
+        if hasattr(self, "delete_save_form"):  # pragma: no cover
+            warnings.warn(
+                "MasterView.delete_save_form() method name is deprecated; "
+                f"please refactor to save_delete_form() instead for {self.__class__.__name__}",
+                DeprecationWarning,
+            )
+            self.delete_save_form(form)
+            return
+
         obj = form.model_instance
         self.delete_instance(obj)
 
+    def redirect_after_delete(self, result):  # pylint: disable=unused-argument
+        """
+        Must return a redirect, following successful save of the
+        "delete" form.  This is called by :meth:`delete()`.
+
+        By default this redirects back to the :meth:`index()` page.
+
+        :returns: :class:`~pyramid.httpexceptions.HTTPFound` instance
+        """
+        return self.redirect(self.get_index_url())
+
     def delete_instance(self, obj):
         """
         Delete the given model instance.
@@ -820,7 +910,7 @@ class MasterView(View):  # pylint: disable=too-many-public-methods
         raise ``NotImplementedError``.  Subclass should override if
         needed.
 
-        This method is called by :meth:`delete_save_form()`.
+        This method is called by :meth:`save_delete_form()`.
         """
         session = self.app.get_session(obj)
         session.delete(obj)
@@ -2787,21 +2877,34 @@ class MasterView(View):  # pylint: disable=too-many-public-methods
         """
         return True
 
-    def make_model_form(self, model_instance=None, **kwargs):
+    def make_model_form(self, model_instance=None, fields=None, **kwargs):
         """
-        Create and return a :class:`~wuttaweb.forms.base.Form`
+        Make a form for the "model" represented by this subclass.
-        for the view model.
 
-        Note that this method is called for multiple "CRUD" views,
+        This method is normally called by all CRUD views:
-        e.g.:
 
+        * :meth:`create()`
         * :meth:`view()`
         * :meth:`edit()`
+        * :meth:`delete()`
 
-        See also related methods, which are called by this one:
+        The form need not have a ``model_instance``, as in the case of
+        :meth:`create()`.  And it can be readonly as in the case of
+        :meth:`view()` and :meth:`delete()`.
 
-        * :meth:`get_form_fields()`
+        If ``fields`` are not provided, :meth:`get_form_fields()` is
-        * :meth:`configure_form()`
+        called.  Usually a subclass will define :attr:`form_fields`
+        but it's only required if :attr:`model_class` is not set.
+
+        Then :meth:`configure_form()` is called, so subclass can go
+        crazy with that as needed.
+
+        :param model_instance: Model instance/record with which to
+           initialize the form data.  Not needed for "create" forms.
+
+        :param fields: Optional fields list for the form.
+
+        :returns: :class:`~wuttaweb.forms.base.Form` instance
         """
         if "model_class" not in kwargs:
             model_class = self.get_model_class()
@@ -2810,7 +2913,7 @@ class MasterView(View):  # pylint: disable=too-many-public-methods
 
         kwargs["model_instance"] = model_instance
 
-        if not kwargs.get("fields"):
+        if not fields:
             fields = self.get_form_fields()
         if fields:
             kwargs["fields"] = fields
@@ -2864,6 +2967,8 @@ class MasterView(View):  # pylint: disable=too-many-public-methods
 
         self.set_labels(form)
 
+        # mark key fields as readonly to prevent edit.  see also
+        # related comments in the objectify() method
         if self.editing:
             for key in self.get_model_key():
                 form.set_readonly(key)
@@ -2881,23 +2986,41 @@ class MasterView(View):  # pylint: disable=too-many-public-methods
 
         This is called by various other form-saving methods:
 
-        * :meth:`edit_save_form()`
+        * :meth:`save_create_form()`
+        * :meth:`save_edit_form()`
         * :meth:`create_row_save_form()`
 
+        See also :meth:`persist()`.
+
         :param form: Reference to the *already validated*
            :class:`~wuttaweb.forms.base.Form` object.  See the form's
            :attr:`~wuttaweb.forms.base.Form.validated` attribute for
            the data.
         """
 
-        # use ColanderAlchemy magic if possible
+        # ColanderAlchemy schema has an objectify() method which will
+        # return a populated model instance
         schema = form.get_schema()
         if hasattr(schema, "objectify"):
-            # this returns a model instance
             return schema.objectify(form.validated, context=form.model_instance)
 
-        # otherwise return data dict as-is
+        # at this point we likely have no model class, so have to
-        return form.validated
+        # assume we're operating on a simple dict record.  we (mostly)
+        # want to return that as-is, unless subclass overrides.
+        data = dict(form.validated)
+
+        # nb. we have a unique scenario when *editing* for a simple
+        # dict record (no model class).  we mark the key fields as
+        # readonly in configure_form(), so they aren't part of the
+        # data here, but we need to add them back for sake of
+        # e.g. generating the 'view' route kwargs for redirect.
+        if self.editing:
+            obj = self.get_instance()
+            for key in self.get_model_key():
+                if key not in data:
+                    data[key] = obj[key]
+
+        return data
 
     def persist(self, obj, session=None):
         """
@@ -2914,7 +3037,8 @@ class MasterView(View):  # pylint: disable=too-many-public-methods
         :param obj: Model instance object as produced by
            :meth:`objectify()`.
 
-        See also :meth:`edit_save_form()` which calls this method.
+        See also :meth:`save_create_form()` and
+        :meth:`save_edit_form()`, which call this method.
         """
         model = self.app.model
         model_class = self.get_model_class()
@@ -3215,8 +3339,8 @@ class MasterView(View):  # pylint: disable=too-many-public-methods
 
         form = self.make_row_model_form(cancel_url_fallback=parent_url)
         if form.validate():
-            row = self.create_row_save_form(form)
+            result = self.create_row_save_form(form)
-            return self.redirect_after_create_row(row)
+            return self.redirect_after_create_row(result)
 
         index_link = tags.link_to(self.get_index_title(), self.get_index_url())
         parent_link = tags.link_to(self.get_instance_title(parent), parent_url)

tests/views/test_master.py

@@ -9,7 +9,7 @@ from unittest.mock import MagicMock, patch
 from sqlalchemy import orm
 from pyramid import testing
 from pyramid.response import Response
-from pyramid.httpexceptions import HTTPNotFound
+from pyramid.httpexceptions import HTTPNotFound, HTTPFound
 
 from wuttjamaican.conf import WuttaConfig
 from wuttaweb.views import master as mod
@@ -977,7 +977,7 @@ class TestMasterView(WebTestCase):
             form = view.make_model_form(fields=["name", "description"])
             form.validated = {"name": "first"}
             obj = view.objectify(form)
-            self.assertIs(obj, form.validated)
+            self.assertEqual(obj, form.validated)
 
         # explicit model class (editing)
         with patch.multiple(
@@ -1115,6 +1115,40 @@ class TestMasterView(WebTestCase):
             # setting did not change in DB
             self.assertEqual(self.app.get_setting(self.session, "foo.bar"), "fraggle")
 
+            # post again to save setting
+            with patch.multiple(
+                self.request,
+                method="POST",
+                POST={
+                    "name": "foo.bar",
+                    "value": "friggle",
+                },
+            ):
+                with patch.object(view, "persist", new=persist):
+                    response = view.create()
+                    self.assertIsInstance(response, HTTPFound)
+                    self.assertFalse(self.request.session.peek_flash("error"))
+                    self.assertEqual(
+                        self.app.get_setting(self.session, "foo.bar"), "friggle"
+                    )
+
+            # and this time force an error on save
+            with patch.multiple(
+                self.request,
+                method="POST",
+                POST={"name": "foo.bar", "value": "froooooggle"},
+            ):
+                with patch.object(
+                    view, "save_create_form", side_effect=RuntimeError("testing")
+                ):
+                    response = view.create()
+                    self.assertEqual(response.status_code, 200)
+                    # nb. flash error is already gone, b/c template is rendered
+                    self.assertFalse(self.request.session.peek_flash("error"))
+                    self.assertEqual(
+                        self.app.get_setting(self.session, "foo.bar"), "friggle"
+                    )
+
     def test_view(self):
         self.pyramid_config.include("wuttaweb.views.common")
         self.pyramid_config.include("wuttaweb.views.auth")
@@ -1210,6 +1244,7 @@ class TestMasterView(WebTestCase):
         with patch.multiple(
             mod.MasterView,
             create=True,
+            # nb. not actually using the model_class here
             model_name="Setting",
             model_key="name",
             get_index_url=MagicMock(return_value="/settings/"),
@@ -1231,23 +1266,24 @@ class TestMasterView(WebTestCase):
                     self.session.commit()
 
                 # post request to save settings
-                self.request.method = "POST"
+                with patch.multiple(
-                self.request.POST = {
+                    self.request,
-                    "name": "foo.bar",
+                    method="POST",
-                    "value": "froogle",
+                    POST={"name": "foo.bar", "value": "froogle"},
-                }
+                ):
                     with patch.object(view, "persist", new=persist):
                         response = view.edit()
-                # nb. should get redirect back to view page
+                        self.assertIsInstance(response, HTTPFound)
-                self.assertEqual(response.status_code, 302)
+                        self.assertEqual(
-                # setting should be updated in DB
+                            response.location, "http://example.com/settings/foo.bar"
+                        )
+                        # setting is saved in DB
                         self.assertEqual(
                             self.app.get_setting(self.session, "foo.bar"), "froogle"
                         )
 
                 # try another post with invalid data (value is required)
-                self.request.method = "POST"
+                with patch.multiple(self.request, method="POST", POST={}):
-                self.request.POST = {}
                     with patch.object(view, "persist", new=persist):
                         response = view.edit()
                     # nb. should get a form with errors
@@ -1258,6 +1294,27 @@ class TestMasterView(WebTestCase):
                         self.app.get_setting(self.session, "foo.bar"), "froogle"
                     )
 
+                # once more with forced error
+                with patch.multiple(
+                    self.request,
+                    method="POST",
+                    POST={
+                        "name": "foo.bar",
+                        "value": "froooooggle",
+                    },
+                ):
+                    with patch.object(
+                        view, "save_edit_form", side_effect=RuntimeError("testing")
+                    ):
+                        response = view.edit()
+                        self.assertEqual(response.status_code, 200)
+                        # nb. flash error is already gone, b/c template is rendered
+                        self.assertFalse(self.request.session.peek_flash("error"))
+                        # setting did not change in DB
+                        self.assertEqual(
+                            self.app.get_setting(self.session, "foo.bar"), "froogle"
+                        )
+
     def test_delete(self):
         self.pyramid_config.include("wuttaweb.views.common")
         self.pyramid_config.include("wuttaweb.views.auth")
@@ -1266,18 +1323,21 @@ class TestMasterView(WebTestCase):
         self.pyramid_config.add_route("settings.edit", "/settings/{name}/edit")
         model = self.app.model
         self.app.save_setting(self.session, "foo.bar", "frazzle")
+        self.app.save_setting(self.session, "another", "fun-value")
         self.session.commit()
-        self.assertEqual(self.session.query(model.Setting).count(), 1)
+        self.assertEqual(self.session.query(model.Setting).count(), 2)
 
         def get_instance():
-            setting = self.session.get(model.Setting, "foo.bar")
+            name = self.request.matchdict["name"]
+            setting = self.session.get(model.Setting, name)
+            if not setting:
+                raise view.notfound()
             return {
                 "name": setting.name,
                 "value": setting.value,
             }
 
         # sanity/coverage check using /settings/XXX/delete
-        self.request.matchdict = {"name": "foo.bar"}
         with patch.multiple(
             mod.MasterView,
             create=True,
@@ -1290,6 +1350,7 @@ class TestMasterView(WebTestCase):
             with patch.object(view, "get_instance", new=get_instance):
 
                 # get the form page
+                with patch.object(self.request, "matchdict", new={"name": "foo.bar"}):
                     response = view.delete()
                     self.assertIsInstance(response, Response)
                     self.assertEqual(response.status_code, 200)
@@ -1298,8 +1359,9 @@ class TestMasterView(WebTestCase):
                 def delete_instance(setting):
                     self.app.delete_setting(self.session, setting["name"])
 
-                self.request.method = "POST"
+                with patch.multiple(
-                self.request.POST = {}
+                    self.request, matchdict={"name": "foo.bar"}, method="POST", POST={}
+                ):
                     with patch.object(view, "delete_instance", new=delete_instance):
 
                         # enforces "instance not deletable" rules
@@ -1308,14 +1370,31 @@ class TestMasterView(WebTestCase):
                         # nb. should get redirect back to view page
                         self.assertEqual(response.status_code, 302)
                         # setting remains in DB
-                    self.assertEqual(self.session.query(model.Setting).count(), 1)
+                        self.assertEqual(self.session.query(model.Setting).count(), 2)
 
                         # post request to delete setting
                         response = view.delete()
                         # nb. should get redirect back to view page
                         self.assertEqual(response.status_code, 302)
                         # setting should be gone from DB
-                    self.assertEqual(self.session.query(model.Setting).count(), 0)
+                        self.assertEqual(self.session.query(model.Setting).count(), 1)
+
+                # try to delete 2nd setting, but force an error
+                with patch.multiple(
+                    self.request, matchdict={"name": "another"}, method="POST", POST={}
+                ):
+                    with patch.object(
+                        view, "save_delete_form", side_effect=RuntimeError("testing")
+                    ):
+                        response = view.delete()
+                        self.assertEqual(response.status_code, 200)
+                        # nb. flash error is already gone, b/c template is rendered
+                        self.assertFalse(self.request.session.peek_flash("error"))
+                        # setting is still in DB
+                        self.assertEqual(self.session.query(model.Setting).count(), 1)
+                        self.assertEqual(
+                            self.app.get_setting(self.session, "another"), "fun-value"
+                        )
 
     def test_delete_instance(self):
         model = self.app.model