Repository: CSCfi/rems
Pull request #1534 (2019-08-22 15:14:18)

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



Patch:
diff --git a/resources/migrations/20190821124800-drop-catalogue-item-title.down.sql b/resources/migrations/20190821124800-drop-catalogue-item-title.down.sql
new file mode 100644
index 000000000..775a83662
--- /dev/null
+++ b/resources/migrations/20190821124800-drop-catalogue-item-title.down.sql
@@ -0,0 +1,5 @@
+ALTER TABLE catalogue_item ADD title varchar(256);
+--;; XXX: Do not copy title from localizations but leave it empty.
+UPDATE catalogue_item SET title='';
+--;;
+ALTER TABLE catalogue_item ALTER COLUMN title SET NOT NULL;
diff --git a/resources/migrations/20190821124800-drop-catalogue-item-title.up.sql b/resources/migrations/20190821124800-drop-catalogue-item-title.up.sql
new file mode 100644
index 000000000..4529a04ca
--- /dev/null
+++ b/resources/migrations/20190821124800-drop-catalogue-item-title.up.sql
@@ -0,0 +1,8 @@
+-- Add localizations for catalogue items with a title but missing localizations.
+INSERT INTO catalogue_item_localization(catid, title, langcode)
+SELECT ci.id, ci.title, 'en'
+FROM catalogue_item ci FULL OUTER JOIN catalogue_item_localization loc
+ON ci.id=loc.catid
+WHERE loc.catid IS NULL;
+--;;
+ALTER TABLE catalogue_item DROP COLUMN title;
diff --git a/resources/sql/queries.sql b/resources/sql/queries.sql
index 161e7a3c2..ab86ae0a4 100644
--- a/resources/sql/queries.sql
+++ b/resources/sql/queries.sql
@@ -7,7 +7,7 @@
 -- - :workflow workflow id to fetch items for
 -- - :form form id to fetch items for
 -- - :archived true if archived items should be included
-SELECT ci.id, ci.title, res.resid, ci.wfid, ci.formid, ci.start, ci.endt as "end", ci.enabled, ci.archived
+SELECT ci.id, res.resid, ci.wfid, ci.formid, ci.start, ci.endt as "end", ci.enabled, ci.archived
 , res.id AS "resource-id"
 /*~ (when (:expand-names? params) */
 , wf.title AS "workflow-name"
@@ -60,8 +60,8 @@ WHERE id = :id;
 -- :name create-catalogue-item! :insert
 -- :doc Create a single catalogue item
 INSERT INTO catalogue_item
-(title, formid, resid, wfid, enabled, archived)
-VALUES (:title, :form, :resid, :wfid,
+(formid, resid, wfid, enabled, archived)
+VALUES (:form, :resid, :wfid,
 --~ (if (contains? params :enabled) ":enabled" "true")
 ,
 --~ (if (contains? params :archived) ":archived" "false")
diff --git a/src/clj/rems/api/catalogue_items.clj b/src/clj/rems/api/catalogue_items.clj
index 05e219116..99f7551d7 100644
--- a/src/clj/rems/api/catalogue_items.clj
+++ b/src/clj/rems/api/catalogue_items.clj
@@ -14,8 +14,7 @@
 ;; TODO resid is misleading: it's the internal id, not the string id
 ;; Should we take the string id instead?
 (s/defschema CreateCatalogueItemCommand
-  {:title s/Str
-   :form s/Int
+  {:form s/Int
    :resid s/Int
    :wfid s/Int
    (s/optional-key :enabled) s/Bool
diff --git a/src/clj/rems/api/schema.clj b/src/clj/rems/api/schema.clj
index 7a6231df3..c1059c839 100644
--- a/src/clj/rems/api/schema.clj
+++ b/src/clj/rems/api/schema.clj
@@ -6,7 +6,6 @@
 
 (s/defschema CatalogueItem
   {:id s/Int
-   :title s/Str
    :wfid s/Int
    (s/optional-key :workflow-name) s/Str
    :formid s/Int
@@ -14,13 +13,14 @@
    :resid s/Str
    :resource-id s/Int
    (s/optional-key :resource-name) s/Str
-   (s/optional-key :langcode) s/Keyword
    :start DateTime
    :end (s/maybe DateTime)
    :enabled s/Bool
    :archived s/Bool
    :expired s/Bool
-   :localizations (s/maybe {s/Any s/Any})})
+   :localizations {s/Keyword {:id s/Int
+                              :langcode s/Keyword
+                              :title s/Str}}})
 
 (s/defschema License
   {:id s/Int
diff --git a/src/clj/rems/api/services/catalogue.clj b/src/clj/rems/api/services/catalogue.clj
index 12175cc83..6216242b7 100644
--- a/src/clj/rems/api/services/catalogue.clj
+++ b/src/clj/rems/api/services/catalogue.clj
@@ -16,7 +16,7 @@
                                        :items [id]})}))
 
 (defn create-catalogue-item! [command]
-  (let [id (:id (db/create-catalogue-item! (select-keys command [:title :form :resid :wfid :enabled :archived])))]
+  (let [id (:id (db/create-catalogue-item! (select-keys command [:form :resid :wfid :enabled :archived])))]
     {:success (not (nil? id))
      :id id}))
 
diff --git a/src/clj/rems/application/model.clj b/src/clj/rems/application/model.clj
index 2d36a2863..35ad2b9d8 100644
--- a/src/clj/rems/application/model.clj
+++ b/src/clj/rems/application/model.clj
@@ -451,8 +451,7 @@
                 {:catalogue-item/id (:catalogue-item/id resource)
                  :resource/ext-id (:resource/ext-id resource)
                  :resource/id (:resource-id item)
-                 :catalogue-item/title (assoc (localization-for :title item)
-                                              :default (:title item))
+                 :catalogue-item/title (localization-for :title item)
                  ;; TODO: remove unused keys
                  :catalogue-item/start (:start item)
                  :catalogue-item/end (:end item)
diff --git a/src/clj/rems/db/catalogue.clj b/src/clj/rems/db/catalogue.clj
index 1fc82b753..8b0101845 100644
--- a/src/clj/rems/db/catalogue.clj
+++ b/src/clj/rems/db/catalogue.clj
@@ -22,7 +22,7 @@
   "Associates localisations into a catalogue item from
   the preloaded state."
   [item]
-  (assoc item :localizations ((cached :localizations) (:id item))))
+  (assoc item :localizations (get (cached :localizations) (:id item) {})))
 
 (defn get-localized-catalogue-items
   ([]
diff --git a/src/cljc/rems/text.cljc b/src/cljc/rems/text.cljc
index 6ae6b74cb..c361382ad 100644
--- a/src/cljc/rems/text.cljc
+++ b/src/cljc/rems/text.cljc
@@ -45,13 +45,16 @@
   (let [lang #?(:clj context/*lang*
                 :cljs @(rf/subscribe [:language]))]
     (or (get m lang)
+        ;; TODO: After all non-localized titles have been removed and guaranteed
+        ;;   that the localizations exist for all languages, this :default
+        ;;   fallback can be removed.
         (get m :default)
         (first (vals m)))))
 
 ;; TODO: replace usages of `get-localized-title` with `localized`
 (defn get-localized-title [item language]
   (or (get-in item [:localizations language :title])
-      (:title item)))
+      (:title (first (vals (get item :localizations))))))
 
 (def ^:private states
   {:application.state/draft :t.applications.states/draft
diff --git a/src/cljs/rems/administration/catalogue_item.cljs b/src/cljs/rems/administration/catalogue_item.cljs
index 521cc3bd0..7f27bef5b 100644
--- a/src/cljs/rems/administration/catalogue_item.cljs
+++ b/src/cljs/rems/administration/catalogue_item.cljs
@@ -6,7 +6,7 @@
             [rems.atoms :as atoms :refer [info-field readonly-checkbox document-title]]
             [rems.collapsible :as collapsible]
             [rems.spinner :as spinner]
-            [rems.text :refer [localize-time text text-format]]
+            [rems.text :refer [get-localized-title localize-time text text-format]]
             [rems.util :refer [dispatch! fetch]]))
 
 (rf/reg-event-fx
@@ -40,10 +40,9 @@
   [:div.spaced-vertically-3
    [collapsible/component
     {:id "catalogue-item"
-     :title [:span (get-in catalogue-item [:localizations language :title] (:title catalogue-item))]
+     :title [:span (get-localized-title catalogue-item language)]
      :always (into [:div]
                    (concat
-                    [[inline-info-field (text :t.administration/title) (:title catalogue-item)]]
                     (for [[langcode localization] (:localizations catalogue-item)]
                       [inline-info-field
                        (str (text :t.administration/title)
diff --git a/src/cljs/rems/administration/create_catalogue_item.cljs b/src/cljs/rems/administration/create_catalogue_item.cljs
index 4d3e76824..bf481629f 100644
--- a/src/cljs/rems/administration/create_catalogue_item.cljs
+++ b/src/cljs/rems/administration/create_catalogue_item.cljs
@@ -46,15 +46,13 @@
        (not (str/blank? (:title localization)))))
 
 (defn- valid-request? [request]
-  (and (not (str/blank? (:title request)))
-       (number? (:wfid request))
+  (and (number? (:wfid request))
        (number? (:resid request))
        (number? (:form request))
        (every? valid-localization? (:localizations request))))
 
 (defn build-request [form languages]
-  (let [request {:title (get (:title form) (first languages))
-                 :wfid (get-in form [:workflow :id])
+  (let [request {:wfid (get-in form [:workflow :id])
                  :resid (get-in form [:resource :id])
                  :form (get-in form [:form :id])
                  :localizations (for [language languages]
diff --git a/src/cljs/rems/application.cljs b/src/cljs/rems/application.cljs
index 2c016f693..a4bc713b1 100644
--- a/src/cljs/rems/application.cljs
+++ b/src/cljs/rems/application.cljs
@@ -740,11 +740,11 @@
             [disabled-items-warning
              {:application/state :application.state/submitted
               :application/resources [{:catalogue-item/enabled true :catalogue-item/archived true
-                                       :catalogue-item/title {:default "Catalogue item 1"}}
+                                       :catalogue-item/title {:en "Catalogue item 1"}}
                                       {:catalogue-item/enabled false :catalogue-item/archived false
-                                       :catalogue-item/title {:default "Catalogue item 2"}}
+                                       :catalogue-item/title {:en "Catalogue item 2"}}
                                       {:catalogue-item/enabled true :catalogue-item/archived false
-                                       :catalogue-item/title {:default "Catalogue item 3"}}]}])
+                                       :catalogue-item/title {:en "Catalogue item 3"}}]}])
 
    (example "link license"
             [:form


Test patch:
diff --git a/src/clj/rems/db/test_data.clj b/src/clj/rems/db/test_data.clj
index 7f54006b9..ff53e848c 100644
--- a/src/clj/rems/db/test_data.clj
+++ b/src/clj/rems/db/test_data.clj
@@ -129,8 +129,7 @@
 
 (defn create-catalogue-item! [{:keys [title resource-id form-id workflow-id]
                                :as command}]
-  (let [result (catalogue/create-catalogue-item! {:title ""
-                                                  :resid (or resource-id (create-resource! {}))
+  (let [result (catalogue/create-catalogue-item! {:resid (or resource-id (create-resource! {}))
                                                   :form (or form-id (create-form! {}))
                                                   :wfid (or workflow-id (create-dynamic-workflow! {}))})
         _ (assert (:success result) {:command command :result result})
diff --git a/test/clj/rems/api/test_administration.clj b/test/clj/rems/api/test_administration.clj
index 0468643a0..4fadd4a16 100644
--- a/test/clj/rems/api/test_administration.clj
+++ b/test/clj/rems/api/test_administration.clj
@@ -27,8 +27,7 @@
                                     :type :dynamic :handlers [user-id]}
                                    api-key user-id))
         catalogue-id (:id (api-call :post "/api/catalogue-items/create"
-                                    {:title "test-item-title"
-                                     :form form-id
+                                    {:form form-id
                                      :resid resource-id
                                      :wfid workflow-id}
                                     api-key user-id))
@@ -83,7 +82,7 @@
       (let [resp (update-resource! {:enabled true :archived true})]
         (is (false? (:success resp)))
         (is (= [{:type "t.administration.errors/resource-in-use"
-                 :catalogue-items [{:id catalogue-id :title "test-item-title" :localizations nil}]}]
+                 :catalogue-items [{:id catalogue-id :localizations {}}]}]
                (:errors resp)))))
 
     (testing "can disable a form"
@@ -93,7 +92,7 @@
       (let [resp (update-form! {:enabled true :archived true})]
         (is (false? (:success resp)))
         (is (= [{:type "t.administration.errors/form-in-use"
-                 :catalogue-items [{:id catalogue-id :title "test-item-title" :localizations nil}]}]
+                 :catalogue-items [{:id catalogue-id :localizations {}}]}]
                (:errors resp)))))
 
     (testing "can disable a workflow"
@@ -103,7 +102,7 @@
       (let [resp (update-workflow! {:enabled true :archived true})]
         (is (false? (:success resp)))
         (is (= [{:type "t.administration.errors/workflow-in-use"
-                 :catalogue-items [{:id catalogue-id :title "test-item-title" :localizations nil}]}]
+                 :catalogue-items [{:id catalogue-id :localizations {}}]}]
                (:errors resp)))))
 
     (testing "can disable a license"
diff --git a/test/clj/rems/api/test_catalogue_items.clj b/test/clj/rems/api/test_catalogue_items.clj
index d11b56f4e..9e5717fc7 100644
--- a/test/clj/rems/api/test_catalogue_items.clj
+++ b/test/clj/rems/api/test_catalogue_items.clj
@@ -22,8 +22,7 @@
       (is (str/starts-with? (:resid item) "urn:")))
     (let [data (-> (request :post "/api/catalogue-items/create")
                    (authenticate api-key "owner")
-                   (json-body {:title "test-item-title"
-                               :form form-id
+                   (json-body {:form form-id
                                :resid 1
                                :wfid 1
                                :archived true})
@@ -37,11 +36,10 @@
                      handler
                      read-body)]
         (is (= {:id id
-                :title "test-item-title"
                 :workflow-name "dynamic workflow"
                 :form-name "form name"
                 :resource-name "urn:nbn:fi:lb-201403262"}
-               (select-keys data [:id :title :workflow-name :form-name :resource-name])))))
+               (select-keys data [:id :workflow-name :form-name :resource-name])))))
     (testing "not found"
       (let [response (-> (request :get "/api/catalogue-items/777777777")
                          (authenticate api-key user-id)
@@ -72,8 +70,7 @@
     (is (= "invalid api key"
            (-> (request :post (str "/api/catalogue-items/create"))
                (assoc-in [:headers "x-rems-api-key"] "invalid-api-key")
-               (json-body {:title "malicious item"
-                           :form 1
+               (json-body {:form 1
                            :resid 1
                            :wfid 1})
                handler
diff --git a/test/clj/rems/api/test_forms.clj b/test/clj/rems/api/test_forms.clj
index 1aa2896d7..649dd8327 100644
--- a/test/clj/rems/api/test_forms.clj
+++ b/test/clj/rems/api/test_forms.clj
@@ -155,8 +155,7 @@
                         read-ok-body))))
     (let [data (-> (request :post "/api/catalogue-items/create")
                    (authenticate api-key user-id)
-                   (json-body {:title "test-item-title"
-                               :form form-id
+                   (json-body {:form form-id
                                :resid 1
                                :wfid 1
                                :archived false})
diff --git a/test/clj/rems/application/test_model.clj b/test/clj/rems/application/test_model.clj
index 963e3d997..508be502f 100644
--- a/test/clj/rems/application/test_model.clj
+++ b/test/clj/rems/application/test_model.clj
@@ -39,7 +39,6 @@
        :resid "urn:11"
        :wfid 50
        :formid 40
-       :title "non-localized title"
        :localizations {:en {:id 10
                             :langcode :en
                             :title "en title"}
@@ -56,7 +55,6 @@
        :resid "urn:21"
        :wfid 50
        :formid 40
-       :title "non-localized title"
        :localizations {:en {:id 20
                             :langcode :en
                             :title "en title"}
@@ -73,7 +71,6 @@
        :resid "urn:31"
        :wfid 50
        :formid 40
-       :title "non-localized title"
        :localizations {:en {:id 20
                             :langcode :en
                             :title "en title"}
@@ -251,8 +248,7 @@
                                                            :resource/id 11
                                                            :resource/ext-id "urn:11"
                                                            :catalogue-item/title {:en "en title"
-                                                                                  :fi "fi title"
-                                                                                  :default "non-localized title"}
+                                                                                  :fi "fi title"}
                                                            :catalogue-item/start (DateTime. 100)
                                                            :catalogue-item/end nil
                                                            :catalogue-item/enabled true
@@ -262,8 +258,7 @@
                                                            :resource/id 21
                                                            :resource/ext-id "urn:21"
                                                            :catalogue-item/title {:en "en title"
-                                                                                  :fi "fi title"
-                                                                                  :default "non-localized title"}
+                                                                                  :fi "fi title"}
                                                            :catalogue-item/start (DateTime. 100)
                                                            :catalogue-item/end nil
                                                            :catalogue-item/enabled true
@@ -429,8 +424,7 @@
                                                                                         :resource/id 31
                                                                                         :resource/ext-id "urn:31"
                                                                                         :catalogue-item/title {:en "en title"
-                                                                                                               :fi "fi title"
-                                                                                                               :default "non-localized title"}
+                                                                                                               :fi "fi title"}
                                                                                         :catalogue-item/start (DateTime. 100)
                                                                                         :catalogue-item/end nil
                                                                                         :catalogue-item/enabled true
@@ -560,8 +554,7 @@
                                                                                             :resource/id 31
                                                                                             :resource/ext-id "urn:31"
                                                                                             :catalogue-item/title {:en "en title"
-                                                                                                                   :fi "fi title"
-                                                                                                                   :default "non-localized title"}
+                                                                                                                   :fi "fi title"}
                                                                                             :catalogue-item/start (DateTime. 100)
                                                                                             :catalogue-item/end nil
                                                                                             :catalogue-item/enabled true
@@ -652,8 +645,7 @@
                                                                                                :resource/id 11
                                                                                                :resource/ext-id "urn:11"
                                                                                                :catalogue-item/title {:en "en title"
-                                                                                                                      :fi "fi title"
-                                                                                                                      :default "non-localized title"}
+                                                                                                                      :fi "fi title"}
                                                                                                :catalogue-item/start (DateTime. 100)
                                                                                                :catalogue-item/end nil
                                                                                                :catalogue-item/enabled true
@@ -663,8 +655,7 @@
                                                                                                :resource/id 31
                                                                                                :resource/ext-id "urn:31"
                                                                                                :catalogue-item/title {:en "en title"
-                                                                                                                      :fi "fi title"
-                                                                                                                      :default "non-localized title"}
+                                                                                                                      :fi "fi title"}
                                                                                                :catalogue-item/start (DateTime. 100)
                                                                                                :catalogue-item/end nil
                                                                                                :catalogue-item/enabled true
diff --git a/test/cljs/rems/administration/test_create_catalogue_item.cljs b/test/cljs/rems/administration/test_create_catalogue_item.cljs
index aa6d785ea..7bd1d2d22 100644
--- a/test/cljs/rems/administration/test_create_catalogue_item.cljs
+++ b/test/cljs/rems/administration/test_create_catalogue_item.cljs
@@ -14,8 +14,7 @@
         languages [:en :fi]]
 
     (testing "valid form"
-      (is (= {:title "en title"
-              :wfid 123
+      (is (= {:wfid 123
               :resid 456
               :form 789
               :localizations [{:langcode "en"