Use a separate endpoint for AI settings

This matches the UI and makes for a smaller, more focused view.

lms/routes.py

@@ -148,6 +148,7 @@ def includeme(config):  # pylint:disable=too-many-statements
     config.add_route("admin.instance.create", "/admin/instance/create")
     config.add_route("admin.instance.upgrade", "/admin/instance/upgrade")
     config.add_route("admin.instance", "/admin/instance/{id_}/")
+    config.add_route("admin.instance.settings", "/admin/instance/{id_}/settings")
     config.add_route("admin.instance.downgrade", "/admin/instance/{id_}/downgrade")
     config.add_route("admin.instance.move_org", "/admin/instance/{id_}/move_org")
 

lms/templates/admin/application_instance/show.html.jinja2

@@ -108,7 +108,7 @@
                 </li>
                 <li>
                     <form method="POST"
-                          action="{{ request.route_url("admin.instance", id_=instance.id) }}">
+                          action="{{ request.route_url("admin.instance.settings", id_=instance.id) }}">
                         <input type="hidden" name="csrf_token" value="{{ get_csrf_token() }}">
                         <fieldset class="box">
                             <legend class="label has-text-centered">General settings</legend>

lms/views/admin/application_instance/update.py

@@ -19,8 +19,6 @@ class UpdateApplicationInstanceSchema(PyramidRequestSchema):
     name = fields.Str(required=True, validate=validate.Length(min=1))
     lms_url = fields.URL(required=False)
     deployment_id = fields.Str(required=False)
-    developer_key = fields.Str(required=False)
-    developer_secret = fields.Str(required=False)
 
 
 class UpdateApplicationInstanceView(BaseApplicationInstanceView):
@@ -51,7 +49,34 @@ def update_instance(self):
             developer_secret=self.request.params.get("developer_secret", "").strip(),
         )
 
+        # Notes are displayed in the main info tab but are stored alongside notes
+        notes = self.request.params.get("hypothesis.notes")
+        notes = notes.strip() if notes else None
+        ai.settings.set("hypothesis", "notes", notes)
+
+        self.request.session.flash(f"Updated application instance {ai.id}", "messages")
+        return self._redirect("admin.instance", id_=ai.id)
+
+    @view_config(
+        route_name="admin.instance.settings",
+        request_method="POST",
+        require_csrf=True,
+        permission=Permissions.ADMIN,
+    )
+    def update_instance_settings(self):
+        ai = self.application_instance
+
+        self.application_instance_service.update_application_instance(
+            ai,
+            developer_key=self.request.params.get("developer_key", "").strip(),
+            developer_secret=self.request.params.get("developer_secret", "").strip(),
+        )
+
         for field in ApplicationSettings.fields:
+            # Notes are updated in the main `info` tab, skip it here
+            if field.compound_key == "hypothesis.notes":
+                continue
+
             value = self.request.params.get(field.compound_key)
             value = value.strip() if value else None
 
@@ -69,6 +94,8 @@ def update_instance(self):
                 assert field.format == str
                 ai.settings.set(field.group, field.key, value)
 
-        self.request.session.flash(f"Updated application instance {ai.id}", "messages")
+        self.request.session.flash(
+            f"Updated application instance settings for {ai.id}", "messages"
+        )
 
         return self._redirect("admin.instance", id_=ai.id)

tests/unit/lms/views/admin/application_instance/update_test.py

@@ -17,8 +17,23 @@ def test_update_instance(self, views, pyramid_request, ai_from_matchdict):
             pyramid_request.route_url("admin.instance", id_=ai_from_matchdict.id)
         )
 
+    @pytest.mark.parametrize(
+        "setting,sub_setting,value,expected",
+        (
+            ("hypothesis", "notes", "  NOTES ", "NOTES"),
+            ("hypothesis", "notes", "", None),
+        ),
+    )
     def test_update_application_instance(
-        self, views, pyramid_request, ai_from_matchdict, application_instance_service
+        self,
+        views,
+        pyramid_request,
+        ai_from_matchdict,
+        application_instance_service,
+        setting,
+        sub_setting,
+        value,
+        expected,
     ):
         pyramid_request.params = pyramid_request.POST = {
             "name": "  NAME  ",
@@ -27,6 +42,7 @@ def test_update_application_instance(
             "developer_key": "  DEVELOPER KEY  ",
             "developer_secret": " DEVELOPER SECRET  ",
         }
+        pyramid_request.params[f"{setting}.{sub_setting}"] = value
 
         views.update_instance()
 
@@ -38,6 +54,7 @@ def test_update_application_instance(
             developer_key="DEVELOPER KEY",
             developer_secret="DEVELOPER SECRET",
         )
+        assert ai_from_matchdict.settings.get(setting, sub_setting) == expected
 
     @pytest.mark.usefixtures("with_minimal_fields_for_update")
     def test_update_application_instance_with_minimal_arguments(
@@ -67,7 +84,6 @@ def test_update_application_instance_with_invalid_arguments(
 
         application_instance_service.update_application_instance.assert_not_called()
 
-    @pytest.mark.usefixtures("with_minimal_fields_for_update")
     @pytest.mark.parametrize(
         "setting,sub_setting,value,expected",
         (
@@ -89,8 +105,6 @@ def test_update_application_instance_with_invalid_arguments(
             ("vitalsource", "api_key", "api_key", "api_key"),
             ("vitalsource", "user_lti_param", "user_id", "user_id"),
             ("vitalsource", "user_lti_pattern", "name_(.*)", "name_(.*)"),
-            ("hypothesis", "notes", "  NOTES ", "NOTES"),
-            ("hypothesis", "notes", "", None),
         ),
     )
     def test_update_instance_saves_ai_settings(
@@ -105,11 +119,10 @@ def test_update_instance_saves_ai_settings(
     ):
         pyramid_request.params[f"{setting}.{sub_setting}"] = value
 
-        views.update_instance()
+        views.update_instance_settings()
 
         assert ai_from_matchdict.settings.get(setting, sub_setting) == expected
 
-    @pytest.mark.usefixtures("with_minimal_fields_for_update")
     @pytest.mark.parametrize(
         "setting,sub_setting", (("desire2learn", "client_secret"),)
     )
@@ -125,7 +138,7 @@ def test_update_instance_saves_secret_settings(
         pyramid_request.params[f"{setting}.{sub_setting}"] = "SECRET"
 
         with patch.object(ai_from_matchdict.settings, "set_secret"):
-            views.update_instance()
+            views.update_instance_settings()
 
             ai_from_matchdict.settings.set_secret.assert_called_once_with(
                 aes_service, setting, sub_setting, "SECRET"