From 8d342c7d59605b30a941e6b934b9ee21cf38daf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=A4in=C3=B6=20Ala-H=C3=A4rk=C3=B6nen?= Date: Wed, 6 Mar 2024 16:24:11 +0200 Subject: [PATCH 1/2] OY-4699 API and functionality for updating all form koodisto values --- spec/ataru/fixtures/form.clj | 54 ++++++++++++ .../virkailija/virkailija_routes_spec.clj | 82 ++++++++++++++++++- src/clj/ataru/forms/form_access_control.clj | 17 +++- src/clj/ataru/koodisto/koodisto.clj | 58 +++++++++---- .../ataru/virkailija/virkailija_routes.clj | 5 ++ .../component_data/value_transformers.cljc | 69 ++++++++++------ 6 files changed, 240 insertions(+), 45 deletions(-) diff --git a/spec/ataru/fixtures/form.clj b/spec/ataru/fixtures/form.clj index 1e88632998..057ee7702b 100644 --- a/spec/ataru/fixtures/form.clj +++ b/spec/ataru/fixtures/form.clj @@ -275,3 +275,57 @@ :metadata metadata :fieldType "attachment" :label {:fi "Liite ilman vastausta"}}]}) + +(def form-with-koodisto-source + {:id 981230123 + :name {:fi "Test fixture for options vs koodisto"} + :key "koodisto-test-form" + :created-by "1.2.246.562.11.11111111111" + :organization-oid "1.2.246.562.10.0439845" + :created-time "2016-07-28T09:58:34.217+03:00" + :locked nil + :locked-by nil + :content [{:id "kysymys_1", + :options [{:value "2"}, + {:value "1"}, + {:value "4"}, + {:value "3"}, + {:value "5"}], + :metadata metadata, + :fieldType "dropdown", + :fieldClass "formField", + :validators ["required"], + :koodisto-source {:uri "kktutkinnot", + :title "Kk-tutkinnot", + :version 1, + :allow-invalid? false}}]}) + +(def form-test-followup-value + {:value "3" + :label {:fi "" :sv ""} + :followups [(assoc (component/text-field metadata) + :id "text")]}) + +(def form-with-koodisto-source-and-followup + {:id 981230123 + :name {:fi "Test fixture for options vs koodisto"} + :key "koodisto-test-form" + :created-by "1.2.246.562.11.11111111111" + :organization-oid "1.2.246.562.10.0439845" + :created-time "2016-07-28T09:58:34.217+03:00" + :locked nil + :locked-by nil + :content [{:id "kysymys_1", + :options [{:value "2" :label {:fi "" :sv ""}}, + {:value "1" :label {:fi "" :sv ""}}, + {:value "4" :label {:fi "" :sv ""}}, + form-test-followup-value, + {:value "5" :label {:fi "" :sv ""}}], + :metadata metadata, + :fieldType "dropdown", + :fieldClass "formField", + :validators ["required"], + :koodisto-source {:uri "kktutkinnot", + :title "Kk-tutkinnot", + :version 1, + :allow-invalid? false}}]}) diff --git a/spec/ataru/virkailija/virkailija_routes_spec.clj b/spec/ataru/virkailija/virkailija_routes_spec.clj index 051f8a239a..36e958fb29 100644 --- a/spec/ataru/virkailija/virkailija_routes_spec.clj +++ b/spec/ataru/virkailija/virkailija_routes_spec.clj @@ -9,13 +9,17 @@ [ataru.organization-service.organization-service :as org-service] [ataru.person-service.person-service :as person-service] [ataru.tarjonta-service.mock-tarjonta-service :as tarjonta-service] + [ataru.cache.cache-service :as cache-service] [ataru.test-utils :refer [login should-have-header]] [ataru.virkailija.editor.form-diff :as form-diff] [ataru.virkailija.virkailija-routes :as v] + [ataru.forms.form-store :as form-store] + [ataru.koodisto.koodisto :as koodisto] [cheshire.core :as json] [clj-ring-db-session.session.session-store :refer [create-session-store]] [ring.mock.request :as mock] - [speclj.core :refer [before describe it run-specs should should-not-be-nil should= tags with]] + [speclj.core :refer [before describe it run-specs should should-contain + should-be-nil should-not-be-nil should= tags with]] [com.stuartsierra.component :as component])) (def virkailija-routes @@ -27,6 +31,11 @@ :kayttooikeus-service (kayttooikeus-service/->FakeKayttooikeusService) :person-service (person-service/->FakePersonService) :audit-logger (audit-log/new-dummy-audit-logger) + :koodisto-cache (reify cache-service/Cache + (get-from [_ _]) + (get-many-from [_ _]) + (remove-from [_ _]) + (clear-all [_])) :application-service (component/using (application-service/new-application-service) [:organization-service @@ -90,6 +99,13 @@ (mock/content-type "application/json") ((deref virkailija-routes)))) +(defn- refresh-form-codes [key] + (-> (mock/request :put (str "/lomake-editori/api/forms/" key "/refresh-codes")) + (update-in [:headers] assoc "cookie" (login @virkailija-routes)) + (mock/content-type "application/json") + ((deref virkailija-routes)) + (update :body (comp (fn [content] (json/parse-string content true)) slurp)))) + (declare resp) (describe "GET /lomake-editori" @@ -365,4 +381,68 @@ status (:status resp)] (should= 200 status)))) +(describe "Refreshing form code values" + (tags :unit :koodisto) + + (defn dummy-koodisto-options [values] + (map (fn [val] + {:value val :label {:fi "" :sv ""}}) values)) + + (defn post-refresh-and-check [expected] + (let [resp (refresh-form-codes "koodisto-test-form") + status (:status resp) + id (get-in resp [:body :id])] + (should= expected status) + id)) + + (defn get-first-option-values [form] + (->> form + :content + first + :options + (map :value) + sort)) + + (it "Should update new codes on form based on new koodisto values" + (with-redefs [koodisto/get-koodisto-options (fn [_ uri _ _] + (case uri + "kktutkinnot" + (dummy-koodisto-options ["1" "2" "3" "4" "5" "6"])))] + (db/init-db-fixture fixtures/form-with-koodisto-source) + (let [id (post-refresh-and-check 200) + new-form (form-store/fetch-by-id id) + new-option-values (get-first-option-values new-form)] + (should= '("" "1" "2" "3" "4" "5" "6") new-option-values)))) + + (it "Should delete existing codes when they do not have followups" + (with-redefs [koodisto/get-koodisto-options (fn [_ uri _ _] + (case uri + "kktutkinnot" + (dummy-koodisto-options ["1" "2" "5"])))] + (db/init-db-fixture fixtures/form-with-koodisto-source) + (let [id (post-refresh-and-check 200) + new-form (form-store/fetch-by-id id) + new-option-values (get-first-option-values new-form)] + (should= '("" "1" "2" "5") new-option-values)))) + + (it "Should keep existing followups while updating koodisto values" + (with-redefs [koodisto/get-koodisto-options (fn [_ uri _ _] + (case uri + "kktutkinnot" + (dummy-koodisto-options ["1" "2" "3" "4" "5"])))] + (db/init-db-fixture fixtures/form-with-koodisto-source-and-followup) + (let [id (post-refresh-and-check 200) + new-form (form-store/fetch-by-id id) + new-options (->> new-form :content first :options)] + (should-contain fixtures/form-test-followup-value new-options)))) + + (it "Should fail if there are followups in option values that would be silently removed" + (with-redefs [koodisto/get-koodisto-options (fn [_ uri _ _] + (case uri + "kktutkinnot" + (dummy-koodisto-options ["1" "2" "4" "5"])))] + (db/init-db-fixture fixtures/form-with-koodisto-source-and-followup) + (let [id (post-refresh-and-check 400)] + (should-be-nil id))))) + (run-specs) diff --git a/src/clj/ataru/forms/form_access_control.clj b/src/clj/ataru/forms/form_access_control.clj index 2814293978..baba4e6976 100644 --- a/src/clj/ataru/forms/form_access_control.clj +++ b/src/clj/ataru/forms/form_access_control.clj @@ -9,6 +9,7 @@ [ataru.organization-service.session-organizations :as session-orgs] [ataru.middleware.user-feedback :refer [user-feedback-exception]] [ataru.tarjonta.haku :as haku] + [ataru.koodisto.koodisto :as koodisto] [taoensso.timbre :as log])) (defn- form-allowed-by-id? @@ -100,14 +101,18 @@ (defn post-form [form session tarjonta-service organization-service audit-logger] (let [organization-oids (map :oid (get-organizations-with-edit-rights session)) first-org-oid (first organization-oids) - form-with-org (assoc form :organization-oid (or (:organization-oid form) first-org-oid))] + form-with-org (assoc form :organization-oid (or (:organization-oid form) first-org-oid)) + key (:key form)] + (log/info "Checking form field ID duplicates with form key" key) (check-form-field-id-duplicates form) + (log/info "Checking form edit authorization with form key" key) (check-edit-authorization form-with-org session tarjonta-service organization-service (fn [] + (log/info "Creating or updating form with key" key) (form-store/create-form-or-increment-version! (assoc form-with-org @@ -150,6 +155,16 @@ updated-form (form-diff/apply-operations coerced-form operations)] (post-form updated-form session tarjonta-service organization-service audit-logger))))) +(defn refresh-form-codes + [form-key session tarjonta-service organization-service koodisto-cache audit-logger] + (log/info "Requested to refresh codes for form, key" form-key) + (let [form (form-store/fetch-by-key form-key) + has-applications? (form-store/form-has-applications form-key) + updated-form (koodisto/populate-form-koodisto-fields koodisto-cache form true)] + (when has-applications? (throw (user-feedback-exception (str "Lomakkeella " (:key form) " on hakemuksia.")))) + (log/info "Saving form with refreshed code values, key" form-key) + (post-form updated-form session tarjonta-service organization-service audit-logger))) + (defn delete-form [form-id session tarjonta-service organization-service audit-logger] (let [form (form-store/fetch-latest-version form-id)] (if (> (:application-count form) 0) diff --git a/src/clj/ataru/koodisto/koodisto.clj b/src/clj/ataru/koodisto/koodisto.clj index 2c2a76023e..5519f69f19 100644 --- a/src/clj/ataru/koodisto/koodisto.clj +++ b/src/clj/ataru/koodisto/koodisto.clj @@ -1,7 +1,9 @@ (ns ataru.koodisto.koodisto (:require [ataru.util :as util] [ataru.component-data.value-transformers :refer [update-options-while-keeping-existing-followups]] - [ataru.cache.cache-service :as cache-service]) + [ataru.cache.cache-service :as cache-service] + [ataru.middleware.user-feedback :refer [user-feedback-exception]] + [taoensso.timbre :as log]) (:import java.time.ZonedDateTime)) (defn encode-koodisto-key [{:keys [uri version]}] @@ -36,24 +38,41 @@ (= "attachment" (:fieldType field)) (-> field :params :fetch-info-from-kouta?))) +(defn- removed-followup-values + [newest-options existing-options remove-existing] + (if remove-existing + (let [new-values (set (map :value newest-options)) + options-to-be-removed (filter #(not (contains? new-values (:value %))) existing-options)] + (filter #(not-empty (:followups %)) options-to-be-removed)) + [])) + (defn- populate-choice-field-from-koodisto - [koodisto-cache field] + [koodisto-cache field remove-existing] (let [{:keys [uri version default-option]} (:koodisto-source field) - empty-option {:value "" :label {:fi "" :sv "" :en ""}} - koodis (map (fn [koodi] (select-keys koodi [:value :label])) + empty-option {:value "" :label {:fi "" :sv "" :en ""}} + koodis (map (fn [koodi] (select-keys koodi [:value :label])) (get-koodisto-options koodisto-cache uri version (:allow-invalid? (:koodisto-source field)))) - koodis-with-default-option (cond->> koodis - (some? default-option) - (map (fn [option] - (cond-> option - (= default-option (-> option :label :fi)) - (assoc :default-value true))))) - koodis-with-followups (update-options-while-keeping-existing-followups koodis-with-default-option (:options field))] - (assoc field :options (if (= (:fieldType field) "dropdown") - (->> koodis-with-followups + koodis-with-default-option (cond->> koodis + (some? default-option) + (map (fn [option] + (cond-> option + (= default-option (-> option :label :fi)) + (assoc :default-value true))))) + removed-followup-values (removed-followup-values koodis-with-default-option + (:options field) + remove-existing) + koodis-with-followups (update-options-while-keeping-existing-followups koodis-with-default-option + (:options field) + remove-existing)] + (when (not-empty removed-followup-values) + (log/warn "Followup questions would be silently removed, field" (:id field) "values" removed-followup-values) + (throw (user-feedback-exception + (str "Kysymyksellä " (:id field) " on jatkokysymyksiä jotka poistettaisiin: " removed-followup-values)))) + (assoc field :options (if (= (:fieldType field) "dropdown") + (->> koodis-with-followups (remove (comp (partial = "") :value)) (cons empty-option)) - koodis-with-followups)))) + koodis-with-followups)))) (def attachment-type-koodisto-uri "liitetyypitamm") (def attachment-type-koodisto-version 1) @@ -72,10 +91,10 @@ (assoc field :label label))) (defn- populate-form-koodisto-field - [koodisto-cache field] + [koodisto-cache remove-existing field] (cond (choice-field-from-koodisto? field) - (populate-choice-field-from-koodisto koodisto-cache field) + (populate-choice-field-from-koodisto koodisto-cache field remove-existing) (attachment-from-kouta? field) (populate-attachment-field-from-koodisto koodisto-cache field) @@ -83,10 +102,13 @@ :else field)) (defn populate-form-koodisto-fields - [koodisto-cache form] + ([koodisto-cache form remove-existing] (update form :content (partial util/map-form-fields (partial populate-form-koodisto-field - koodisto-cache)))) + koodisto-cache + remove-existing)))) + ([koodisto-cache form] + (populate-form-koodisto-fields koodisto-cache form false))) (defn get-postal-office-by-postal-code [koodisto-cache postal-code] diff --git a/src/clj/ataru/virkailija/virkailija_routes.clj b/src/clj/ataru/virkailija/virkailija_routes.clj index 69a55d535c..bf3bfc8333 100644 --- a/src/clj/ataru/virkailija/virkailija_routes.clj +++ b/src/clj/ataru/virkailija/virkailija_routes.clj @@ -293,6 +293,11 @@ :summary "Toggle form locked state" (ok (access-controlled-form/update-form-lock id operation session tarjonta-service organization-service audit-logger))) + (api/PUT "/forms/:form-key/refresh-codes" {session :session} + :summary "Update all code values on form to latest ones from koodisto service" + :path-params [form-key :- s/Str] + (ok (access-controlled-form/refresh-form-codes form-key session tarjonta-service organization-service koodisto-cache audit-logger))) + (api/DELETE "/forms/:id" {session :session} :path-params [id :- Long] :summary "Mark form as deleted" diff --git a/src/cljc/ataru/component_data/value_transformers.cljc b/src/cljc/ataru/component_data/value_transformers.cljc index 4f41e863d3..4bec37978d 100644 --- a/src/cljc/ataru/component_data/value_transformers.cljc +++ b/src/cljc/ataru/component_data/value_transformers.cljc @@ -22,28 +22,47 @@ (Integer/valueOf year)] :cljs [day month year]))))) -(defn update-options-while-keeping-existing-followups [newest-options existing-options] - (let [options (if (empty? existing-options) - newest-options - (let [existing-values (set (map :value existing-options)) - options-that-didnt-exist-before (filter #(not (contains? existing-values (:value %))) newest-options)] - (vec (concat options-that-didnt-exist-before - (map (fn [existing-option] - (if-let [new-option (first (filter #(= (:value %) (:value existing-option)) newest-options))] - (assoc existing-option :label (:label new-option)) - existing-option)) - existing-options))))) - identical (fn [{:keys [value]}] - (keep-indexed (fn [index option] - (when (= value (:value option)) - [index option])) - options)) - remove-hidden? (fn [{:keys [hidden] :as koodi}] - (if (or (nil? koodi) hidden) - true - false))] - (vec (remove remove-hidden? (map-indexed (fn [index option] - (let [last-option (first (last (identical option)))] - (when (= index last-option) - option))) - options))))) +(defn merge-options-keeping-existing + [newest-options existing-options] + (let [existing-values (set (map :value existing-options)) + options-that-didnt-exist-before (filter #(not (contains? existing-values (:value %))) newest-options)] + (vec (concat options-that-didnt-exist-before + (map (fn [existing-option] + (if-let [new-option (first (filter #(= (:value %) (:value existing-option)) newest-options))] + (assoc existing-option :label (:label new-option)) + existing-option)) + existing-options))))) + +; Returns the set of new options with possible old followups added. +(defn merge-options-removing-existing + [newest-options existing-options] + (vec (map (fn [new-option] + (let [existing-option (first (filter #(= (:value %) (:value new-option)) existing-options))] + (if (and existing-option (:followups existing-option)) + (assoc new-option :followups (:followups existing-option)) + new-option))) + newest-options))) + +(defn update-options-while-keeping-existing-followups + ([newest-options existing-options remove-existing] + (let [options (if (empty? existing-options) + newest-options + (if remove-existing + (merge-options-removing-existing newest-options existing-options) + (merge-options-keeping-existing newest-options existing-options))) + identical (fn [{:keys [value]}] + (keep-indexed (fn [index option] + (when (= value (:value option)) + [index option])) + options)) + remove-hidden? (fn [{:keys [hidden] :as koodi}] + (if (or (nil? koodi) hidden) + true + false))] + (vec (remove remove-hidden? (map-indexed (fn [index option] + (let [last-option (first (last (identical option)))] + (when (= index last-option) + option))) + options))))) + ([newest-options existing-options] + (update-options-while-keeping-existing-followups newest-options existing-options false))) From a3dea0ec0ce9e55871c102c461f407005ffed697 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=A4in=C3=B6=20Ala-H=C3=A4rk=C3=B6nen?= Date: Mon, 11 Mar 2024 15:25:48 +0200 Subject: [PATCH 2/2] OY-4699 sort codes according to Finnish labels when updating --- .../virkailija/virkailija_routes_spec.clj | 19 +++++++++++++++++++ src/clj/ataru/koodisto/koodisto.clj | 3 ++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/spec/ataru/virkailija/virkailija_routes_spec.clj b/spec/ataru/virkailija/virkailija_routes_spec.clj index 36e958fb29..49dd084d3c 100644 --- a/spec/ataru/virkailija/virkailija_routes_spec.clj +++ b/spec/ataru/virkailija/virkailija_routes_spec.clj @@ -414,6 +414,25 @@ new-option-values (get-first-option-values new-form)] (should= '("" "1" "2" "3" "4" "5" "6") new-option-values)))) + (it "Should alphabetize based on Finnish labels" + (with-redefs [koodisto/get-koodisto-options (fn [_ uri _ _] + (case uri + "kktutkinnot" + [{:value 1 :label {:fi "Beekkonen" :sv ""}} + {:value 2 :label {:fi "Aakkonen" :sv ""}} + {:value 3 :label {:fi "Ceekkonen" :sv ""}}]))] + (db/init-db-fixture fixtures/form-with-koodisto-source) + (let [id (post-refresh-and-check 200) + new-form (form-store/fetch-by-id id) + new-options (->> new-form + :content + first + :options)] + (should= [{:label {:en "", :fi "", :sv ""}, :value ""} + {:value 2 :label {:fi "Aakkonen" :sv ""}} + {:value 1 :label {:fi "Beekkonen" :sv ""}} + {:value 3 :label {:fi "Ceekkonen" :sv ""}}] new-options)))) + (it "Should delete existing codes when they do not have followups" (with-redefs [koodisto/get-koodisto-options (fn [_ uri _ _] (case uri diff --git a/src/clj/ataru/koodisto/koodisto.clj b/src/clj/ataru/koodisto/koodisto.clj index 5519f69f19..b6a6893cbe 100644 --- a/src/clj/ataru/koodisto/koodisto.clj +++ b/src/clj/ataru/koodisto/koodisto.clj @@ -50,8 +50,9 @@ [koodisto-cache field remove-existing] (let [{:keys [uri version default-option]} (:koodisto-source field) empty-option {:value "" :label {:fi "" :sv "" :en ""}} - koodis (map (fn [koodi] (select-keys koodi [:value :label])) + unsorted-koodis (map (fn [koodi] (select-keys koodi [:value :label])) (get-koodisto-options koodisto-cache uri version (:allow-invalid? (:koodisto-source field)))) + koodis (sort-by (comp :fi :label) unsorted-koodis) koodis-with-default-option (cond->> koodis (some? default-option) (map (fn [option]