Skip to content
This repository has been archived by the owner on Feb 7, 2024. It is now read-only.

Add option to search valvonta by asiakirjapohja #921

Merged
merged 4 commits into from
Jun 30, 2023
Merged

Add option to search valvonta by asiakirjapohja #921

merged 4 commits into from
Jun 30, 2023

Conversation

solita-antti-mottonen
Copy link
Contributor

No description provided.

@solita-antti-mottonen solita-antti-mottonen force-pushed the feature/AE-1895-filter-kaytonvalvonta-by-template-used branch 2 times, most recently from e22146e to 9be7436 Compare June 29, 2023 10:15
@solita-antti-mottonen solita-antti-mottonen force-pushed the feature/AE-1895-filter-kaytonvalvonta-by-template-used branch from 9be7436 to 5a21e6d Compare June 29, 2023 10:16
Juholei
Juholei suggested changes Jun 30, 2023
View reviewed changes
etp-backend/src/test/clj/solita/etp/valvonta_test.clj Outdated
@@ -1,5 +1,7 @@
(ns solita.etp.valvonta-test
(:require
[clojure.core]
[clojure.set]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Miksi nämä on requireina? Corea ei tarvitsisi ikinä requireta erikseen ja tällaisenaan nämä eivät tee mitään, kun ei ole määritelty aliasta tai sieltä referoitavia funktioita. Tämän tyyliset requiret ovat tarpeen ainoastaan, jos pelkkä namespacen require aiheuttaa jo sivuvaikutuksia.

etp-backend/src/test/clj/solita/etp/valvonta_test.clj Outdated
@@ -22,12 +24,21 @@
; Mimics real handler usage with test assets
(handler/handler (merge req {:db ts/*db* :aws-s3-client ts/*aws-s3-client*})))

(defn- non-null-key-to-string [m k]
(if (some? (k m)) (assoc m k (.toString (k m))) m))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tässä voisi olla varmaankin .toStringin sijaan vain str-kutsu. Rivittäisin myös luettavuuden vuoksi ifin ehdon ja haarat omille riveilleen

etp-backend/src/test/clj/solita/etp/valvonta_test.clj Outdated
:rooli 2
}))
)))
(with-redefs [solita.etp.service.suomifi-viestit/send-message! assert-suomifi-message-sent]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tähän mieluummin tuo namespacen alias takaisin kuin koko fully qualified namespacen nimi

etp-backend/src/test/clj/solita/etp/valvonta_test.clj Outdated
(generators/complete valvonta-schema/ValvontaSave)
(assoc :ilmoituspaikka-id 1
:valvoja-id kayttaja-id)
((fn [valvonta] (assoc valvonta :id (valvonta-service/add-valvonta! ts/*db* valvonta))))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mieluusti tämä funktion määritys tuohon ylempään letiin, niin on mielestäni luettavampi kuin että luodaan anonyymi funktio ja kutsutaan sitä heti samassa yhteydessä

etp-backend/src/test/clj/solita/etp/valvonta_test.clj Outdated
((fn [valvonta] (assoc valvonta :id (valvonta-service/add-valvonta! ts/*db* valvonta))))))
;; Create 6 toimenpide for each valvonta
_ (doall (->> (repeatedly 12 (fn [] (generators/complete {} valvonta-schema/ToimenpideAdd)))
(map vector (flatten (repeatedly (constantly '(1 2)))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'()-syntaksilla määriteltäviä listoja on harvemmin tapana käyttää koodissa, mieluummin käyttää vektoreita kun niitä yleensä voi käyttää ihan huoletta toisiaan vastaavina asioina. Eli [1 2] siinä tapauksessa tähän.

etp-backend/src/test/clj/solita/etp/valvonta_test.clj Outdated
:valvoja-id kayttaja-id)
((fn [valvonta] (assoc valvonta :id (valvonta-service/add-valvonta! ts/*db* valvonta))))))
;; Create 6 toimenpide for each valvonta
_ (doall (->> (repeatedly 12 (fn [] (generators/complete {} valvonta-schema/ToimenpideAdd)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pääsisikö doallista eroon jos tässä käyttäisi mapin sijaan mapv:tä?

etp-backend/src/test/clj/solita/etp/valvonta_test.clj Outdated
(map (fn [[template-id toimenpide]] (assoc toimenpide :template-id template-id)))
(map (fn [toimenpide] (assoc toimenpide :type-id 1)))
(map vector (flatten (repeat (map :id valvonnat))))
(map (fn [[valvonta-id toimenpide]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lopullisesta paluuarvosta ei olla ilmeisesti kiinnostuneita, joten tarvitaanko tämän mapin sisällä tuota merge-kutsua? Myöskin koska tässä tehdään sivuvaikutuksia, ottaisin ehkä tuolla ylempänä letissä kiinni paluuarvon tuohon edelliseen steppiin asti ja korvaisin tämän sitten letin sisällä tehtävällä doseqillä, koska se on tarkoitettu sivuvaikutusten tekemiseen.

etp-backend/src/test/clj/solita/etp/valvonta_test.clj Outdated
(map vector (flatten (repeat (map :id valvonnat))))
(map (fn [[valvonta-id toimenpide]]
(merge toimenpide (valvonta-service/add-toimenpide! ts/*db* ts/*aws-s3-client* {} valvonta-id toimenpide))))))]
(t/testing "Valvonnalle palautetataan 6 toimenpidettä"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kirjoitusvirhe

etp-backend/src/test/clj/solita/etp/valvonta_test.clj Outdated
@@ -22,12 +24,21 @@
; Mimics real handler usage with test assets
(handler/handler (merge req {:db ts/*db* :aws-s3-client ts/*aws-s3-client*})))

(defn- non-null-key-to-string [m k]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tälle olisi ehkä idiomaattisempi nimi non-nil-key->string

etp-backend/src/test/clj/solita/etp/valvonta_test.clj Outdated
:valvoja-id kayttaja-id)
((fn [valvonta] (assoc valvonta :id (valvonta-service/add-valvonta! ts/*db* valvonta))))))
;; Create a toimenpide for each valvonta
toimenpiteet (doall (->> (repeatedly 2 (fn [] (generators/complete {} valvonta-schema/ToimenpideAdd)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Samat mietteet doallin, mapin, vektorin ja doseqin käytöstä tähänkin kuin ylempänä.

Juholei
Juholei reviewed Jun 30, 2023
View reviewed changes
etp-backend/src/test/clj/solita/etp/valvonta_test.clj Outdated
@@ -39,6 +38,12 @@
(mock/header "x-amzn-oidc-identity" "paakayttaja@solita.fi")
(mock/header "x-amzn-oidc-data" oidc-data)))

(defn- add-valvonta-and-map-id [valvonta]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Näihin kahteen funktioon vielä ! nimen perään, koska tallentavat kantaan dataa ja sen myötä aiheuttavat sivuvaikutuksia

@solita-antti-mottonen solita-antti-mottonen merged commit 0041501 into develop Jun 30, 2023
4 checks passed
@solita-antti-mottonen solita-antti-mottonen deleted the feature/AE-1895-filter-kaytonvalvonta-by-template-used branch August 24, 2023 09:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@muep muep muep approved these changes

@Juholei Juholei Juholei approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@solita-antti-mottonen @muep @Juholei