From 490784c71b361d11a5ca3ee3e8f8c760258b6861 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?V=C3=A4in=C3=B6=20Ala-H=C3=A4rk=C3=B6nen?= <vae@wonna.fi>
Date: Thu, 19 Dec 2024 14:38:58 +0200
Subject: [PATCH] OK-719 review style fixes and comments

---
 ...cation_payment_status_updater_job_spec.clj | 32 ++++++++++++++-----
 .../ataru/applications/application_store.clj  |  8 +++++
 ...application_payment_status_updater_job.clj | 14 ++++----
 3 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/spec/ataru/kk_application_payment/kk_application_payment_status_updater_job_spec.clj b/spec/ataru/kk_application_payment/kk_application_payment_status_updater_job_spec.clj
index f5f4ace75c..3dd722040f 100644
--- a/spec/ataru/kk_application_payment/kk_application_payment_status_updater_job_spec.clj
+++ b/spec/ataru/kk_application_payment/kk_application_payment_status_updater_job_spec.clj
@@ -85,7 +85,7 @@
 (declare conn)
 (declare spec)
 
-(defn- get-payment-obligation-review [application-key hakukohde]
+(defn- get-tuition-payment-obligation-review [application-key hakukohde]
   (jdbc/with-db-transaction [conn {:datasource (db/get-datasource :db)}]
                             (->> (jdbc/query
                                    conn
@@ -93,7 +93,7 @@
                                     application-key "payment-obligation" hakukohde])
                                  first)))
 
-(defn- store-not-obligated-review [application-key hakukohde]
+(defn- store-tuition-fee-not-required-review [application-key hakukohde]
   (jdbc/with-db-transaction [conn {:datasource (db/get-datasource :db)}]
                             (jdbc/insert! conn "application_hakukohde_reviews"
                                           {:application_key application-key
@@ -403,6 +403,9 @@
                                                check-mail-fn]}))))
 
           (it "should set tuition fee obligation for non fi/sv hakukohde when payment state changes to awaiting"
+              ; Initial state: hakukohde in the application has only english as teaching language,
+              ; and the application / person has no exemption, meaning the application should require both
+              ; application fee AND tuition fee for the hakukohde.
               (let [application-id (unit-test-db/init-db-fixture
                                      form-fixtures/payment-exemption-test-form
                                      application-fixtures/application-without-hakemusmaksu-exemption
@@ -411,13 +414,17 @@
                         {:person_oid test-person-oid :term test-term :year test-year} runner)
                     application-key (:key (application-store/get-application application-id))
                     payment (first (payment/get-raw-payments [application-key]))
-                    obligation (get-payment-obligation-review application-key "payment-info-test-kk-hakukohde")]
+                    obligation (get-tuition-payment-obligation-review application-key "payment-info-test-kk-hakukohde")]
                 (should= (:awaiting payment/all-states) (:state payment))
                 (should= {:application_key application-key, :requirement "payment-obligation",
                           :state "obligated", :hakukohde "payment-info-test-kk-hakukohde"}
                          (select-keys obligation [:application_key :requirement :state :hakukohde]))))
 
           (it "should set tuition fee obligation for non fi/sv hakukohde when payment state changes to ok-by-proxy"
+              ; Initial state: hakukohde in both applications has only english as teaching language,
+              ; and the application / person has no exemption, and we mark one of the applications paid manually.
+              ; This means the other application should not require application payment BUT should still require
+              ; tuition fee.
               (let [application-ids (unit-test-db/init-db-fixture
                                       form-fixtures/payment-exemption-test-form
                                       [application-fixtures/application-without-hakemusmaksu-exemption
@@ -438,13 +445,16 @@
                     _ (updater-job/update-kk-payment-status-for-person-handler
                         {:person_oid test-person-oid :term test-term :year test-year} runner)
                     payment (first (payment/get-raw-payments [second-key]))
-                    obligation (get-payment-obligation-review second-key "payment-info-test-kk-hakukohde")]
+                    obligation (get-tuition-payment-obligation-review second-key "payment-info-test-kk-hakukohde")]
                 (should= (:ok-by-proxy payment/all-states) (:state payment))
                 (should= {:application_key second-key, :requirement "payment-obligation",
                           :state "obligated", :hakukohde "payment-info-test-kk-hakukohde"}
                          (select-keys obligation [:application_key :requirement :state :hakukohde]))))
 
           (it "should not set tuition fee obligation for non fi/sv hakukohde when payment state changes to not-required"
+              ; Initial state: hakukohde in the application has only english as teaching language,
+              ; but the application / person has an exemption, meaning the application should require neither
+              ; application fee nor tuition fee for the hakukohde.
               (let [application-id (unit-test-db/init-db-fixture
                                      form-fixtures/payment-exemption-test-form
                                      application-fixtures/application-with-hakemusmaksu-exemption
@@ -453,11 +463,14 @@
                         {:person_oid test-person-oid :term test-term :year test-year} runner)
                     application-key (:key (application-store/get-application application-id))
                     payment (first (payment/get-raw-payments [application-key]))
-                    obligation (get-payment-obligation-review application-key "payment-info-test-kk-hakukohde")]
+                    obligation (get-tuition-payment-obligation-review application-key "payment-info-test-kk-hakukohde")]
                 (should= (:not-required payment/all-states) (:state payment))
                 (should-be-nil obligation)))
 
           (it "should not set tuition fee obligation for fi/sv hakukohde"
+              ; Initial state: hakukohde in the application has swedish and/or finnish in its teaching languages,
+              ; so even the application / person has no exemption, the application should require only an
+              ; application fee, but no tuition fee for the hakukohde.
               (let [application-id (unit-test-db/init-db-fixture
                                      form-fixtures/payment-exemption-test-form
                                      (merge
@@ -468,21 +481,24 @@
                         {:person_oid test-person-oid :term test-term :year test-year} runner)
                     application-key (:key (application-store/get-application application-id))
                     payment (first (payment/get-raw-payments [application-key]))
-                    obligation (get-payment-obligation-review application-key "payment-info-test-kk-fisv-hakukohde")]
+                    obligation (get-tuition-payment-obligation-review application-key "payment-info-test-kk-fisv-hakukohde")]
                 (should= (:awaiting payment/all-states) (:state payment))
                 (should-be-nil obligation)))
 
           (it "should not override a non-automatic obligation"
+              ; Initial state: hakukohde in the application has only english as teaching language,
+              ; and application / person has no exemption, but there's a human review already for the tuition.
+              ; Application fee should be required, but tuition fee state should not change automatically anymore.
               (let [application-id (unit-test-db/init-db-fixture
                                      form-fixtures/payment-exemption-test-form
                                      application-fixtures/application-without-hakemusmaksu-exemption
                                      nil)
                     application-key (:key (application-store/get-application application-id))
-                    _ (store-not-obligated-review application-key "payment-info-test-kk-hakukohde")
+                    _ (store-tuition-fee-not-required-review application-key "payment-info-test-kk-hakukohde")
                     _ (updater-job/update-kk-payment-status-for-person-handler
                         {:person_oid test-person-oid :term test-term :year test-year} runner)
                     payment (first (payment/get-raw-payments [application-key]))
-                    obligation (get-payment-obligation-review application-key "payment-info-test-kk-hakukohde")]
+                    obligation (get-tuition-payment-obligation-review application-key "payment-info-test-kk-hakukohde")]
                 (should= (:awaiting payment/all-states) (:state payment))
                 (should= {:application_key application-key, :requirement "payment-obligation",
                           :state "not-obligated", :hakukohde "payment-info-test-kk-hakukohde"}
diff --git a/src/clj/ataru/applications/application_store.clj b/src/clj/ataru/applications/application_store.clj
index 315c0d6841..71f5a2ed94 100644
--- a/src/clj/ataru/applications/application_store.clj
+++ b/src/clj/ataru/applications/application_store.clj
@@ -842,6 +842,8 @@
           (queries/yesql-add-application-event<! event connection))))))
 
 (defn save-payment-obligation-automatically-changed
+  "Only used by automatic tuition fee obligation logic. Sets new obligation state if previous one was not set by
+   virkailija and the state transition is allowed. If successful, adds an event marking the obligation was set automatically."
   [application-key hakukohde-oid hakukohde-review-requirement hakukohde-review-state]
   (jdbc/with-db-transaction [conn {:datasource (db/get-datasource :db)}]
     (let [connection                  {:connection conn}
@@ -856,6 +858,11 @@
                                            :event_type
                                            (= "payment-obligation-automatically-changed"))
           existing-requirement-review (first (queries/yesql-get-existing-requirement-review review-to-store connection))]
+      ; Main idea:
+      ; - When the state is still unreviewed, it can be always automatically changed.
+      ; - When virkailija has made the latest modification to tuition fee obligation (not automatically-changed?)
+      ;   we should never override that state automatically anymore.
+      ; - Once the state is automatically set as obligated, don't modify further without virkailija input.
       (when (or (and (= "not-obligated" (:state review-to-store))
                      (= "unreviewed" (:state existing-requirement-review "unreviewed")))
                 (and (= "unreviewed" (:state review-to-store))
@@ -866,6 +873,7 @@
                 (and (= "obligated" (:state review-to-store))
                      (= "not-obligated" (:state existing-requirement-review))
                      automatically-changed?))
+        ; Whenever we set the obligation automatically, also add an event for auditing and possible further state changes.
         (queries/yesql-upsert-application-hakukohde-review! review-to-store connection)
         (let [event {:application_key          application-key
                      :event_type               "payment-obligation-automatically-changed"
diff --git a/src/clj/ataru/kk_application_payment/kk_application_payment_status_updater_job.clj b/src/clj/ataru/kk_application_payment/kk_application_payment_status_updater_job.clj
index ab84cfed7b..0732c0f390 100644
--- a/src/clj/ataru/kk_application_payment/kk_application_payment_status_updater_job.clj
+++ b/src/clj/ataru/kk_application_payment/kk_application_payment_status_updater_job.clj
@@ -120,16 +120,15 @@
       (payment/get-valid-payment-info-for-application-id tarjonta-service application-id)
       (payment/get-valid-payment-info-for-application-key tarjonta-service application-key))))
 
-(defn- oid-if-needs-tuition-fee
+(defn- needs-tuition-fee?
   [hakukohde]
   (let [codes (->> (:opetuskieli-koodi-urit hakukohde)
                    (map #(first (str/split % #"#")))
                    set)]
-    (when (and (seq codes)
-               (not (contains? codes "oppilaitoksenopetuskieli_1"))  ; fi
-               (not (contains? codes "oppilaitoksenopetuskieli_2"))  ; sv
-               (not (contains? codes "oppilaitoksenopetuskieli_3"))) ; fi/sv
-      (:oid hakukohde))))
+    (and (seq codes)
+         (not (contains? codes "oppilaitoksenopetuskieli_1"))    ; fi
+         (not (contains? codes "oppilaitoksenopetuskieli_2"))    ; sv
+         (not (contains? codes "oppilaitoksenopetuskieli_3"))))) ; fi/sv
 
 (defn- mark-tuition-fee-obligated
   "Marks tuition fee (lukuvuosimaksu) obligation for application key for every hakukohde that does not organize
@@ -140,7 +139,8 @@
         hakukohteet            (tarjonta/get-hakukohteet
                                  tarjonta-service
                                  (remove nil? hakukohde-oids))
-        tuition-hakukohde-oids (remove nil? (map oid-if-needs-tuition-fee hakukohteet))]
+        tuition-hakukohde-oids (remove nil?
+                                       (map #(when (needs-tuition-fee? %) (:oid %)) hakukohteet))]
     (doseq [hakukohde-oid tuition-hakukohde-oids]
       (log/info "Marking tuition payment obligation due to kk application fee eligibility for application key"
                 application-key "and hakukohde oid" hakukohde-oid)