Repository: CSCfi/rems
Pull request #1572 (2019-09-03 09:26:31)

Change forms API to have /enabled and /archived instead of /update
Part of #1548

# Definition of Done / Review checklist

## Reviewability
- [ ] link to issue
- [ ] note if PR is on top of other PR
- [ ] note if related change in rems-deploy repo
- [ ] consider adding screenshots for ease of review

## API
- [ ] API is documented and shows up in Swagger UI
- [ ] API is backwards compatible or completely new
- [ ] Events are backwards compatible

## Documentation
- [ ] update changelog if necessary
- [ ] add or update docstrings for namespaces and functions
- [ ] components are added to guide page
- [ ] documentation _at least_ for config options (i.e. docs folder)
- [ ] ADR for major architectural decisions or experiments

## Different installations
- [ ] new configuration options added to rems-deploy repository
- [ ] instance specific translations (i.e. LBR kielivara)

## Testing
- [ ] complex logic is unit tested
- [ ] valuable features are integration / browser / acceptance tested automatically

## Accessibility
- [ ] all icons have the aria-label attribute
- [ ] all fields have a label
- [ ] errors are linked to fields with aria-describedby
- [ ] contrast is checked
- [ ] conscious decision about where to move focus after an action

## Follow-up
- [ ] new tasks are created for pending or remaining tasks
- [ ] no critical TODOs left to implement



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 937af3c21..fd08d0c79 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -18,11 +18,11 @@ Changes since v2.4
 - API for creating catalogue item and its localizations has been changed.
   There is now a single API call that is used to create both a catalogue
   item and the localizations, namely, /api/catalogue-items/create.
-- API for editing workflow has been changed. The API endpoint for
-  editing name and handlers is now /api/workflows/edit, whereas the
-  endpoint for archiving or unarchiving workflow is /api/workflows/archived
-  and the endpoint for enabling or disabling workflow is
-  /api/workflows/enabled.
+- APIs for editing workflow and form have been changed. The API endpoint
+  for editing content of the workflow now /edit (prefixed with /api/workflows).
+  The endpoint for archiving or unarchiving a workflow or a form is /archived
+  and the endpoint for enabling or disabling a workflow or a form is /enabled,
+  prefixed with /api/workflows or /api/forms, respectively.
 - API endpoint for editing forms has been changed from
   /api/forms/[form-id]/edit to /api/forms/edit.
 
diff --git a/resources/sql/queries.sql b/resources/sql/queries.sql
index 30cc61d84..82d5ee2fc 100644
--- a/resources/sql/queries.sql
+++ b/resources/sql/queries.sql
@@ -186,10 +186,17 @@ SET (organization, title, modifierUserId, fields) =
 WHERE
 id = :id;
 
--- :name set-form-template-state! :!
+-- :name set-form-template-enabled! :!
 -- TODO set modifieruserid?
 UPDATE form_template
-SET (enabled, archived) = (:enabled, :archived)
+SET (enabled) = (:enabled)
+WHERE
+id = :id;
+
+-- :name set-form-template-archived! :!
+-- TODO set modifieruserid?
+UPDATE form_template
+SET (archived) = (:archived)
 WHERE
 id = :id;
 
diff --git a/src/clj/rems/api/forms.clj b/src/clj/rems/api/forms.clj
index 8c68060db..2cc2f33c6 100644
--- a/src/clj/rems/api/forms.clj
+++ b/src/clj/rems/api/forms.clj
@@ -1,6 +1,6 @@
 (ns rems.api.forms
   (:require [compojure.api.sweet :refer :all]
-            [rems.api.schema :refer [SuccessResponse UpdateStateCommand FormTemplateOverview NewFieldTemplate FormTemplate]]
+            [rems.api.schema :refer [ArchivedCommand EnabledCommand FormTemplate FormTemplateOverview NewFieldTemplate SuccessResponse]]
             [rems.api.util :refer [not-found-json-response]] ; required for route :roles
             [rems.db.form :as form]
             [rems.util :refer [getx-user-id]]
@@ -70,9 +70,16 @@
       :return SuccessResponse
       (ok (form/edit-form! (getx-user-id) command)))
 
-    (PUT "/update" []
-      :summary "Update form"
+    (PUT "/archived" []
+      :summary "Archive or unarchive form"
       :roles #{:owner}
-      :body [command UpdateStateCommand]
+      :body [command ArchivedCommand]
       :return SuccessResponse
-      (ok (form/update-form! command)))))
+      (ok (form/set-form-archived! command)))
+
+    (PUT "/enabled" []
+      :summary "Enable or disable form"
+      :roles #{:owner}
+      :body [command EnabledCommand]
+      :return SuccessResponse
+      (ok (form/set-form-enabled! command)))))
diff --git a/src/clj/rems/db/form.clj b/src/clj/rems/db/form.clj
index 077b37a2d..2b663382d 100644
--- a/src/clj/rems/db/form.clj
+++ b/src/clj/rems/db/form.clj
@@ -87,11 +87,16 @@
                                      :fields (serialize-fields form)})
             {:success true}))))
 
-(defn update-form! [command]
-  (let [catalogue-items (catalogue-items-for-form (:id command))]
-    (if (and (:archived command) (seq catalogue-items))
+(defn set-form-enabled! [command]
+  (db/set-form-template-enabled! (select-keys command [:id :enabled]))
+  {:success true})
+
+(defn set-form-archived! [{:keys [id archived]}]
+  (let [catalogue-items (catalogue-items-for-form id)]
+    (if (and archived (seq catalogue-items))
       {:success false
        :errors [{:type :t.administration.errors/form-in-use :catalogue-items catalogue-items}]}
       (do
-        (db/set-form-template-state! command)
+        (db/set-form-template-archived! {:id id
+                                         :archived archived})
         {:success true}))))
diff --git a/src/cljs/rems/administration/form.cljs b/src/cljs/rems/administration/form.cljs
index 72987f5cc..0e629ea37 100644
--- a/src/cljs/rems/administration/form.cljs
+++ b/src/cljs/rems/administration/form.cljs
@@ -80,8 +80,8 @@
       [back-button]
       [edit-button id]
       [copy-as-new-button id]
-      [status-flags/enabled-toggle form #(rf/dispatch [:rems.administration.forms/update-form %1 %2 [::enter-page id]])]
-      [status-flags/archived-toggle form #(rf/dispatch [:rems.administration.forms/update-form %1 %2 [::enter-page id]])]])
+      [status-flags/enabled-toggle form #(rf/dispatch [:rems.administration.forms/set-form-enabled %1 %2 [::enter-page id]])]
+      [status-flags/archived-toggle form #(rf/dispatch [:rems.administration.forms/set-form-archived %1 %2 [::enter-page id]])]])
    [form-preview form]])
 ;; TODO Do we support form licenses?
 
diff --git a/src/cljs/rems/administration/forms.cljs b/src/cljs/rems/administration/forms.cljs
index b13311438..814545640 100644
--- a/src/cljs/rems/administration/forms.cljs
+++ b/src/cljs/rems/administration/forms.cljs
@@ -37,16 +37,25 @@
 (rf/reg-sub ::loading? (fn [db _] (::loading? db)))
 
 (rf/reg-event-fx
- ::update-form
+ ::set-form-archived
  (fn [_ [_ form description dispatch-on-finished]]
-   (put! "/api/forms/update"
+   (put! "/api/forms/archived"
          {:params {:id (:form/id form)
-                   :enabled (:enabled form)
                    :archived (:archived form)}
           :handler (flash-message/status-update-handler description #(rf/dispatch dispatch-on-finished))
           :error-handler (flash-message/default-error-handler description)})
    {}))
 
+(rf/reg-event-fx
+ ::set-form-enabled
+ (fn [_ [_ form description dispatch-on-finished]]
+   (put! "/api/forms/enabled"
+         {:params {:id (:form/id form)
+                   :enabled (:enabled form)}
+          :handler (flash-message/status-update-handler description #(rf/dispatch dispatch-on-finished))
+          :error-handler (flash-message/default-error-handler description)})
+   {}))
+
 (rf/reg-event-fx
  ::set-display-old?
  (fn [{:keys [db]} [_ display-old?]]
@@ -93,8 +102,8 @@
                            [to-view-form form]
                            [form/edit-button (:form/id form)]
                            [copy-as-new-form form]
-                           [status-flags/enabled-toggle form #(rf/dispatch [::update-form %1 %2 [::fetch-forms]])]
-                           [status-flags/archived-toggle form #(rf/dispatch [::update-form %1 %2 [::fetch-forms]])]]}})
+                           [status-flags/enabled-toggle form #(rf/dispatch [::set-form-enabled %1 %2 [::fetch-forms]])]
+                           [status-flags/archived-toggle form #(rf/dispatch [::set-form-archived %1 %2 [::fetch-forms]])]]}})
         forms)))
 
 (defn- forms-list []


Test patch:
diff --git a/src/clj/rems/db/test_data.clj b/src/clj/rems/db/test_data.clj
index f466f5028..25b779d0c 100644
--- a/src/clj/rems/db/test_data.clj
+++ b/src/clj/rems/db/test_data.clj
@@ -208,7 +208,7 @@
   (let [id (create-form! {:actor (+fake-users+ :owner)
                           :form/organization "nbn"
                           :form/title "Archived form, should not be seen by applicants"})]
-    (form/update-form! {:id id :enabled true :archived true})))
+    (form/set-form-archived! {:id id :archived true})))
 
 (defn- create-disabled-license! [owner]
   (let [id (create-license! {:actor owner
diff --git a/test/clj/rems/api/services/test_catalogue.clj b/test/clj/rems/api/services/test_catalogue.clj
index f7963b530..f5c6a7eaa 100644
--- a/test/clj/rems/api/services/test_catalogue.clj
+++ b/test/clj/rems/api/services/test_catalogue.clj
@@ -30,9 +30,8 @@
         #(catalogue/update-catalogue-item! {:id item-id
                                             :enabled true
                                             :archived %})
-        archive-form! #(form/update-form! {:id form-id
-                                           :enabled true
-                                           :archived %})
+        archive-form! #(form/set-form-archived! {:id form-id
+                                                 :archived %})
         archive-license! #(licenses/update-license! {:id lic-id
                                                      :enabled true
                                                      :archived %})
diff --git a/test/clj/rems/api/test_administration.clj b/test/clj/rems/api/test_administration.clj
index 0967dff7f..ba4303d8e 100644
--- a/test/clj/rems/api/test_administration.clj
+++ b/test/clj/rems/api/test_administration.clj
@@ -47,12 +47,15 @@
                                             :archived archived}
                                            api-key user-id))
 
-        update-form! (fn [{:keys [enabled archived]}]
-                       (api-call :put "/api/forms/update"
+        form-archived! #(api-call :put "/api/forms/archived"
+                                  {:id form-id
+                                   :archived %}
+                                  api-key user-id)
+
+        form-enabled! #(api-call :put "/api/forms/enabled"
                                  {:id form-id
-                                  :enabled enabled
-                                  :archived archived}
-                                 api-key user-id))
+                                  :enabled %}
+                                 api-key user-id)
 
         update-license! (fn [{:keys [enabled archived]}]
                           (api-call :put "/api/licenses/update"
@@ -90,10 +93,10 @@
                (:errors resp)))))
 
     (testing "can disable a form"
-      (is (:success (update-form! {:enabled false :archived false}))))
+      (is (:success (form-enabled! false))))
 
     (testing "can't archive a form that's in use"
-      (let [resp (update-form! {:enabled true :archived true})]
+      (let [resp (form-archived! true)]
         (is (false? (:success resp)))
         (is (= [{:type "t.administration.errors/form-in-use"
                  :catalogue-items [{:id catalogue-id :localizations {}}]}]
@@ -127,7 +130,7 @@
       (is (:success (update-resource! {:enabled true :archived true}))))
 
     (testing "can archive a form that's not in use"
-      (is (:success (update-form! {:enabled true :archived true}))))
+      (is (:success (form-archived! true))))
 
     (testing "can archive a workflow that's not in use"
       (is (:success (workflow-archived! true))))
diff --git a/test/clj/rems/api/test_forms.clj b/test/clj/rems/api/test_forms.clj
index 0c86a006f..cea7094c4 100644
--- a/test/clj/rems/api/test_forms.clj
+++ b/test/clj/rems/api/test_forms.clj
@@ -200,7 +200,7 @@
                          read-ok-body)]
             (is (= (:form/organization form) "def"))))))))
 
-(deftest form-update-test
+(deftest form-enabled-archived-test
   (let [api-key "42"
         user-id "owner"
         form-id (-> (request :post "/api/forms/create")
@@ -212,11 +212,17 @@
                     read-ok-body
                     :id)]
     (is (not (nil? form-id)))
-    (testing "update"
-      (is (:success (-> (request :put "/api/forms/update")
+    (testing "disable"
+      (is (:success (-> (request :put "/api/forms/enabled")
+                        (authenticate api-key user-id)
+                        (json-body {:id form-id
+                                    :enabled false})
+                        handler
+                        read-ok-body))))
+    (testing "archive"
+      (is (:success (-> (request :put "/api/forms/archived")
                         (authenticate api-key user-id)
                         (json-body {:id form-id
-                                    :enabled false
                                     :archived true})
                         handler
                         read-ok-body))))
@@ -227,14 +233,20 @@
                      read-ok-body)]
         (is (false? (:enabled form)))
         (is (true? (:archived form)))))
-    (testing "update again"
-      (is (:success (-> (request :put "/api/forms/update")
+    (testing "unarchive"
+      (is (:success (-> (request :put "/api/forms/archived")
                         (authenticate api-key user-id)
                         (json-body {:id form-id
-                                    :enabled true
                                     :archived false})
                         handler
                         read-ok-body))))
+    (testing "enable"
+      (is (:success (-> (request :put "/api/forms/enabled")
+                        (authenticate api-key user-id)
+                        (json-body {:id form-id
+                                    :enabled true})
+                        handler
+                        read-ok-body))))
     (testing "fetch again"
       (let [form (-> (request :get (str "/api/forms/" form-id))
                      (authenticate api-key user-id)