-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new internal endpoint for updating public csv data into S3 #645
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Joitakin ehdotuksia. Oleellisin varmaankin nuo middlewaret.
etp-core/etp-backend/src/main/clj/solita/etp/service/csv_to_s3.clj
Outdated
Show resolved
Hide resolved
etp-core/etp-backend/src/main/clj/solita/etp/service/csv_to_s3.clj
Outdated
Show resolved
Hide resolved
etp-core/etp-backend/src/test/clj/solita/etp/service/csv_to_s3_test.clj
Outdated
Show resolved
Hide resolved
etp-core/etp-backend/src/main/clj/solita/etp/api/public_csv.clj
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Osasin tässä vaiheessa lähinnä peukutella Juhon kommentteja, eli niiden jälkeen approved. 👍
etp-core/etp-backend/src/main/clj/solita/etp/service/csv_to_s3.clj
Outdated
Show resolved
Hide resolved
…l data from public columns as a set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pikkuparannusehdotuksia vielä
etp-core/etp-backend/src/test/clj/solita/etp/service/csv_to_s3_test.clj
Outdated
Show resolved
Hide resolved
(let [hidden-headers #{"tila-id" "laatija-id" "yritys"}] | ||
;; Verify hidden columns don't exist | ||
(t/is (empty? (clojure.set/intersection hidden-headers header-set))))))) | ||
expected-headers (columns->header-strings public-columns) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mietin että kannattaisiko vaan ottaa muodostaa tuo public columns
- hidden-columns
tähän testiin hardcodattuna ja testata että otsikkojoukko on täysin sama. Nämä muuttuvat kuitenkin todella harvoin, ja tämä varmistaisi että jos public-columnsiin tulee joku yllättävä muutos (nyt taitaa olla hardcodattu, mutta varalta jos asia muuttuu), niin testi nappaa kiinni edelleen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uusimmassa commitissa on nyt kolumnit kovakoodattuna testin sisälle, toivottavasti ymmärsin tämän kommentin oikein
;; - todistus-1 with kayttotarkoitus "PK" | ||
;; - todistus-2 with kayttotarkoitus "PK" | ||
;; - todistus-3 with kayttotarkoitus "YAT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Olisikohan huono idea testata nuo kaikki käyttötarkoitusluokat tässä? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Olisi se kyllä tarpeen, mutta näihin testeihin on kyllä jo alkuperäisen työmääräarvion lisäksi kulunut hieman ylimääräistä aikaa domainiin tutustumisen takia. Voisin kysyä tiimiltä mielipidettä myös
Aika pitkälti olemassaolevaa totetusta mukaillen toteutettu uusi sisäinen endpoint julkisen puolen CSV-tiedostojen lataamiselle S3:een.
Aineisto.clj -tiedostossa tulee nyt hieman duplikaatti koodia ja ajattelin yhtenäistää toteutusta kunhan saan palautetta että olenhan tajunnut toiminnallisuuden oikein.
Testasin MinIO Consolen kautta ja näytti tulevan oikean kansion alle CSV:t