Repository: CSCfi/rems
Pull request #1573 (2019-09-03 09:55:03)

Change resources 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 fd08d0c79..a61cdfc3a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -18,11 +18,12 @@ 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.
-- 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.
+- APIs for editing workflow, form and resource have been changed. The API endpoint
+  for editing content of the workflow is now /edit (prefixed with /api/workflows).
+  The endpoint for archiving or unarchiving a workflow, a form, or a resource
+  is /archived and the endpoint for enabling or disabling a workflow, a form, or a
+  resource is /enabled, prefixed with /api/workflows, /api/forms, or api/resources,
+  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 82d5ee2fc..df379cf82 100644
--- a/resources/sql/queries.sql
+++ b/resources/sql/queries.sql
@@ -109,17 +109,16 @@ VALUES (:resid, :organization, :owneruserid, :modifieruserid,
  /*~ (if (:end params) */ :end /*~*/ NULL /*~ ) ~*/
 );
 
--- :name set-resource-state! :insert
+-- :name set-resource-enabled! :!
 -- TODO set modifieruserid?
 UPDATE resource
-SET
-/*~ (when (boolean? (:enabled params)) */
-  enabled = :enabled,
-/*~ ) ~*/
-/*~ (when (boolean? (:archived params)) */
-  archived = :archived,
-/*~ ) ~*/
-  id = id
+SET (enabled) = (:enabled)
+WHERE id = :id;
+
+-- :name set-resource-archived! :!
+-- TODO set modifieruserid?
+UPDATE resource
+SET (archived) = (:archived)
 WHERE id = :id;
 
 -- :name get-database-name :? :1
diff --git a/src/clj/rems/api/resources.clj b/src/clj/rems/api/resources.clj
index 7427f1aa2..ee0c87bc2 100644
--- a/src/clj/rems/api/resources.clj
+++ b/src/clj/rems/api/resources.clj
@@ -64,9 +64,16 @@
       :return CreateResourceResponse
       (ok (resource/create-resource! command (getx-user-id))))
 
-    (PUT "/update" []
-      :summary "Update resource"
+    (PUT "/archived" []
+      :summary "Archive or unarchive resource"
       :roles #{:owner}
-      :body [command UpdateStateCommand]
+      :body [command ArchivedCommand]
       :return SuccessResponse
-      (ok (resource/update-resource! command)))))
+      (ok (resource/set-resource-archived! command)))
+
+    (PUT "/enabled" []
+      :summary "Enable or disable resource"
+      :roles #{:owner}
+      :body [command EnabledCommand]
+      :return SuccessResponse
+      (ok (resource/set-resource-enabled! command)))))
diff --git a/src/clj/rems/api/services/resource.clj b/src/clj/rems/api/services/resource.clj
index 80fad1638..6e6fa7c63 100644
--- a/src/clj/rems/api/services/resource.clj
+++ b/src/clj/rems/api/services/resource.clj
@@ -29,7 +29,11 @@
          :errors [{:type :t.administration.errors/duplicate-resid :resid resid}]}
         (throw e)))))
 
-(defn update-resource! [{:keys [id] :as command}]
+(defn set-resource-enabled! [command]
+  (db/set-resource-enabled! (select-keys command [:id :enabled]))
+  {:success true})
+
+(defn set-resource-archived! [{:keys [id archived]}]
   (let [catalogue-items
         (->> (catalogue/get-localized-catalogue-items {:resource-id id
                                                        :archived false})
@@ -37,19 +41,20 @@
         licenses (licenses/get-resource-licenses id)
         archived-licenses (filter :archived licenses)]
     (cond
-      (and (:archived command) (seq catalogue-items))
+      (and archived (seq catalogue-items))
       {:success false
        :errors [{:type :t.administration.errors/resource-in-use
                  :catalogue-items catalogue-items}]}
 
-      (and (not (:archived command)) (seq archived-licenses))
+      (and (not archived) (seq archived-licenses))
       {:success false
        :errors [{:type :t.administration.errors/license-archived
                  :licenses archived-licenses}]}
 
       :else
       (do
-        (db/set-resource-state! (select-keys command [:id :enabled :archived]))
+        (db/set-resource-archived! {:id id
+                                    :archived archived})
         {:success true}))))
 
 (defn get-resource [id] (resource/get-resource id))
diff --git a/src/cljs/rems/administration/resource.cljs b/src/cljs/rems/administration/resource.cljs
index 8f6501da8..63740adc1 100644
--- a/src/cljs/rems/administration/resource.cljs
+++ b/src/cljs/rems/administration/resource.cljs
@@ -54,8 +54,8 @@
    (let [id (:id resource)]
      [:div.col.commands
       [back-button]
-      [status-flags/enabled-toggle resource #(rf/dispatch [:rems.administration.resources/update-resource %1 %2 [::enter-page id]])]
-      [status-flags/archived-toggle resource #(rf/dispatch [:rems.administration.resources/update-resource %1 %2 [::enter-page id]])]])])
+      [status-flags/enabled-toggle resource #(rf/dispatch [:rems.administration.resources/set-resource-enabled %1 %2 [::enter-page id]])]
+      [status-flags/archived-toggle resource #(rf/dispatch [:rems.administration.resources/set-resource-archived %1 %2 [::enter-page id]])]])])
 
 (defn resource-page []
   (let [resource (rf/subscribe [::resource])
diff --git a/src/cljs/rems/administration/resources.cljs b/src/cljs/rems/administration/resources.cljs
index e4c090566..255b24532 100644
--- a/src/cljs/rems/administration/resources.cljs
+++ b/src/cljs/rems/administration/resources.cljs
@@ -38,10 +38,19 @@
 (rf/reg-sub ::loading? (fn [db _] (::loading? db)))
 
 (rf/reg-event-fx
- ::update-resource
+ ::set-resource-archived
  (fn [_ [_ item description dispatch-on-finished]]
-   (put! "/api/resources/update"
-         {:params (select-keys item [:id :enabled :archived])
+   (put! "/api/resources/archived"
+         {:params (select-keys item [:id :archived])
+          :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-resource-enabled
+ (fn [_ [_ item description dispatch-on-finished]]
+   (put! "/api/resources/enabled"
+         {:params (select-keys item [:id :enabled])
           :handler (flash-message/status-update-handler description #(rf/dispatch dispatch-on-finished))
           :error-handler (flash-message/default-error-handler description)})
    {}))
@@ -84,8 +93,8 @@
                       :sort-value (if checked? 1 2)})
            :commands {:td [:td.commands
                            [to-view-resource (:id resource)]
-                           [status-flags/enabled-toggle resource #(rf/dispatch [::update-resource %1 %2 [::fetch-resources]])]
-                           [status-flags/archived-toggle resource #(rf/dispatch [::update-resource %1 %2 [::fetch-resources]])]]}})
+                           [status-flags/enabled-toggle resource #(rf/dispatch [::set-resource-enabled %1 %2 [::fetch-resources]])]
+                           [status-flags/archived-toggle resource #(rf/dispatch [::set-resource-archived %1 %2 [::fetch-resources]])]]}})
         resources)))
 
 (defn- resources-list []


Test patch:
diff --git a/test/clj/rems/api/services/test_catalogue.clj b/test/clj/rems/api/services/test_catalogue.clj
index f5c6a7eaa..abe0c9284 100644
--- a/test/clj/rems/api/services/test_catalogue.clj
+++ b/test/clj/rems/api/services/test_catalogue.clj
@@ -35,9 +35,8 @@
         archive-license! #(licenses/update-license! {:id lic-id
                                                      :enabled true
                                                      :archived %})
-        archive-resource! #(resource/update-resource! {:id res-id
-                                                       :enabled true
-                                                       :archived %})
+        archive-resource! #(resource/set-resource-archived! {:id res-id
+                                                             :archived %})
         archive-workflow! #(workflow/set-workflow-archived! {:id workflow-id
                                                              :archived %})]
     (testing "new catalogue items are enabled and not archived"
diff --git a/test/clj/rems/api/services/test_resources.clj b/test/clj/rems/api/services/test_resources.clj
index b332e2b00..e76833be2 100644
--- a/test/clj/rems/api/services/test_resources.clj
+++ b/test/clj/rems/api/services/test_resources.clj
@@ -12,7 +12,7 @@
   (-> (resources/get-resource res-id)
       (select-keys [:enabled :archived])))
 
-(deftest test-update-resource!
+(deftest resource-enabled-archived-test
   (let [lic-id (test-data/create-license! {})
         res-id (test-data/create-resource! {:license-ids [lic-id]})
         res-id2 (test-data/create-resource! {})
@@ -27,55 +27,58 @@
              (status-flags res-id))))
 
     ;; reset all to false for the following tests
-    (resources/update-resource! {:id res-id
-                                 :enabled false
-                                 :archived false})
+    (resources/set-resource-enabled! {:id res-id
+                                      :enabled false})
+    (resources/set-resource-archived! {:id res-id
+                                       :archived false})
 
     (testing "enable"
-      (resources/update-resource! {:id res-id
-                                   :enabled true})
+      (resources/set-resource-enabled! {:id res-id
+                                        :enabled true})
       (is (= {:enabled true
               :archived false}
              (status-flags res-id))))
 
     (testing "disable"
-      (resources/update-resource! {:id res-id
-                                   :enabled false})
+      (resources/set-resource-enabled! {:id res-id
+                                        :enabled false})
       (is (= {:enabled false
               :archived false}
              (status-flags res-id))))
 
     (testing "archive"
-      (resources/update-resource! {:id res-id
-                                   :archived true})
+      (resources/set-resource-archived! {:id res-id
+                                         :archived true})
       (is (= {:enabled false
               :archived true}
              (status-flags res-id))))
 
     (testing "unarchive"
-      (resources/update-resource! {:id res-id
-                                   :archived false})
+      (resources/set-resource-archived! {:id res-id
+                                         :archived false})
       (is (= {:enabled false
               :archived false}
              (status-flags res-id))))
 
     (testing "cannot unarchive if license is archived"
-      (resources/update-resource! {:id res-id
-                                   :archived true})
+      (resources/set-resource-archived! {:id res-id
+                                         :archived true})
       (archive-license! true)
-      (is (not (:success (resources/update-resource! {:id res-id
-                                                      :archived false}))))
+      (is (not (:success (resources/set-resource-archived! {:id res-id
+                                                            :archived false}))))
       (archive-license! false)
-      (is (:success (resources/update-resource! {:id res-id
-                                                 :archived false}))))
+      (is (:success (resources/set-resource-archived! {:id res-id
+                                                       :archived false}))))
 
     (testing "does not affect unrelated resources"
-      (resources/update-resource! {:id res-id
-                                   :enabled true
-                                   :archived true})
-      (resources/update-resource! {:id res-id2
-                                   :enabled false
-                                   :archived false})
+      (resources/set-resource-enabled! {:id res-id
+                                        :enabled true})
+      (resources/set-resource-archived! {:id res-id
+                                         :archived true})
+      (resources/set-resource-enabled! {:id res-id2
+                                        :enabled false})
+      (resources/set-resource-archived! {:id res-id2
+                                         :archived false})
       (is (= {:enabled true
               :archived true}
              (status-flags res-id)))
diff --git a/test/clj/rems/api/test_administration.clj b/test/clj/rems/api/test_administration.clj
index ba4303d8e..be1ec0d93 100644
--- a/test/clj/rems/api/test_administration.clj
+++ b/test/clj/rems/api/test_administration.clj
@@ -33,12 +33,15 @@
                                      :localizations {}}
                                     api-key user-id))
 
-        update-resource! (fn [{:keys [enabled archived]}]
-                           (api-call :put "/api/resources/update"
-                                     {:id resource-id
-                                      :enabled enabled
-                                      :archived archived}
-                                     api-key user-id))
+        resource-archived! #(api-call :put "/api/resources/archived"
+                                      {:id resource-id
+                                       :archived %}
+                                      api-key user-id)
+
+        resource-enabled! #(api-call :put "/api/resources/enabled"
+                                      {:id resource-id
+                                       :enabled %}
+                                      api-key user-id)
 
         update-catalogue-item! (fn [{:keys [enabled archived]}]
                                  (api-call :put "/api/catalogue-items/update"
@@ -83,10 +86,10 @@
     (db/create-workflow-license! {:wfid workflow-id :licid license-id})
 
     (testing "can disable a resource"
-      (is (:success (update-resource! {:enabled false :archived false})))
+      (is (:success (resource-enabled! false)))
 
       (testing "can't archive resource if it is part of an active catalogue item"
-      (let [resp (update-resource! {:enabled true :archived true})]
+      (let [resp (resource-archived! true)]
         (is (false? (:success resp)))
         (is (= [{:type "t.administration.errors/resource-in-use"
                  :catalogue-items [{:id catalogue-id :localizations {}}]}]
@@ -127,7 +130,7 @@
       (is (:success (update-catalogue-item! {:enabled true :archived true}))))
 
     (testing "can archive a resource that's not in use"
-      (is (:success (update-resource! {:enabled true :archived true}))))
+      (is (:success (resource-archived! true))))
 
     (testing "can archive a form that's not in use"
       (is (:success (form-archived! true))))
@@ -139,7 +142,7 @@
       (is (= {:success true} (update-license! {:enabled true :archived true}))))
 
     (testing "cannot unarchive a resource with an archived license"
-      (let [resp (update-resource! {:enabled true :archived false})]
+      (let [resp (resource-archived! false)]
         (is (false? (:success resp)))
         (is (= "t.administration.errors/license-archived"
                (get-in resp [:errors 0 :type]))))))))
diff --git a/test/clj/rems/api/test_resources.clj b/test/clj/rems/api/test_resources.clj
index 98ac5dad7..310c9d317 100644
--- a/test/clj/rems/api/test_resources.clj
+++ b/test/clj/rems/api/test_resources.clj
@@ -16,8 +16,15 @@
       handler
       read-ok-body))
 
-(defn- update-resource! [command api-key user-id]
-  (-> (request :put "/api/resources/update")
+(defn- resource-archived! [command api-key user-id]
+  (-> (request :put "/api/resources/archived")
+      (authenticate api-key user-id)
+      (json-body command)
+      handler
+      read-ok-body))
+
+(defn- resource-enabled! [command api-key user-id]
+  (-> (request :put "/api/resources/enabled")
       (authenticate api-key user-id)
       (json-body command)
       handler
@@ -37,26 +44,26 @@
                                                :organization "abc"
                                                :licenses []}
                                               api-key user-id))
-            _ (update-resource! {:id enabled-id
-                                 :enabled true
-                                 :archived false}
-                                api-key user-id)
+            _ (resource-enabled! {:id enabled-id :enabled true}
+                                 api-key user-id)
+            _ (resource-archived! {:id enabled-id :archived false}
+                                  api-key user-id)
             disabled-id (:id (create-resource! {:resid "disabled"
                                                 :organization "abc"
                                                 :licenses []}
                                                api-key user-id))
-            _ (update-resource! {:id disabled-id
-                                 :enabled false
-                                 :archived false}
-                                api-key user-id)
+            _ (resource-enabled! {:id disabled-id :enabled false}
+                                 api-key user-id)
+            _ (resource-archived! {:id disabled-id :archived false}
+                                  api-key user-id)
             archived-id (:id (create-resource! {:resid "archived"
                                                 :organization "abc"
                                                 :licenses []}
                                                api-key user-id))
-            _ (update-resource! {:id archived-id
-                                 :enabled true
-                                 :archived true}
-                                api-key user-id)]
+            _ (resource-enabled! {:id archived-id :enabled true}
+                                 api-key user-id)
+            _ (resource-archived! {:id archived-id :archived true}
+                                  api-key user-id)]
         (testing "hides disabled and archived by default"
           (let [data (-> (request :get "/api/resources")
                          (authenticate api-key user-id)