# Loppukatselmoinnin raportti Laadittu 3.11.2020, tekijänä A.Y ## Mikä tämä asiakirja on? Tämä asiakirja kuvaa Kodavi-projektin **loppukatselmoinnissa** havaittuja asioita, ja mahdollisia puutteita. Tämä asiakirja on tarkoitettu erityisesti projektiorganisaation (ensisijaisesti projektiryhmä, mutta myös vastaava(t) ohjaaja(t) ja tilaajat) # Yleistä ## Ympäristö Katselmointiympäristönä on käytetty _Windows 10 Professional_:in koontiversiota 2004. Lisäksi on käytetty Oraclen VirtualBox 6.1:tä, ilman VirtualBox Extension Packia (pääohjelmisto on GPL2-lisenssillä, Extension Pack vaatisi maksullisen lisenssin). Jotta testaaminen ei aiheuttaisi häiriöitä tuotantoympäristöön, pystytin melko verrannollisen ympäristön virtuaalikoneeseen, seuraten aiemmin hyväksyttyjä asennusohjeita. Sen eroavaisuuksien pääjärjestelmästä ei pitäisi olla merkittäviä verrattuna tuotantojärjestelmään. Sen speksit ovat pääpiirteittäin seuraavat: - Pohjajärjestelmänä **CentOS 7** - Postgresin oma YUM-repo, asennettuna: `sudo yum -y install https://download.postgresql.org/pub/repos/yum/reporpms/EL-7-x86_64/pgdg-redhat-repo-latest.noarch.rpm` - Postgres 12 asennettu. Tietokanta on kopioitu **1.11.2020** mukaisesta tuotantoympäristöstä - Tomcat 9, seuraavin ohjein: `https://linuxize.com/post/how-to-install-tomcat-9-on-centos-7/`, lähteenä `https://mirror-2.hosthink.net/apache/tomcat/tomcat-9/v9.0.39/bin/apache-tomcat-9.0.39.tar.gz` ## Dokumentin rakenne Katselmointiasiakirja on jaoteltu erikseen eri osa-alueisiin; lisäksi huomiot on jaeteltu seuraavasti ### Tyypit - **Kriittinen**: selkeä ongelma, suositellaan hyvin vahvasti korjaamista ellei tilaajan kanssa nimenomaisesti muuta sovita - **Tärkeä**: selvä ja vähäistä isompi puute tai epäselvyys joko toiminnallisesti tai tyylillisesti, suositellaan korjattavaksi ellei tilaajan kanssa muuta sovita - **Vähäinen:** aiheuttaa lähinnä kosmeettisia haittoja tai pienehköä harmistusta jatkokehittäjälle, voidaan melko harmitta siirtää jatkokehitykseen ellei tilaajan kanssa muuta sovita # Katselmointi ## Koodin läpikäynti ### Frontend ##### Tärkeä - `actions`:issa: voisiko `fetch`:iä käyttäviä metodeja yhdistää? Ne näyttävät noudattavan aika pitkälle samaa kaavaa esim. poikkeuksienkäsittelyssä - `chart.jsx` kaipaa ehkä enemmän dokumentaatiota, ulkopuoliselle eri osa-alueiden merkitys ei välttämättä avaudu kovin helposti. Esim. X-askelten (_tick_) renderöintiin liittyy selvästi jotain monimutkaisempaa logiikkaa vaikkapa SVG-polkujen muodostamisen osalta, mutta mitä? - Olisiko vakioiden siirtäminen johonkin tiettyyn alimoduliin järkevää? Esim. `chart.jsx`:n `colors`, voisiko tätä olla tarpeen muokata myöhemmin eri skeemaan? - `searchpage/index.jsx:115`: tälläiset monikerroksiset sisäkkäiset ternääri (`?:`)-rakenteet ovat usein hankalasti ymmärrettäviä, olisiko tätä mahdollista pirstoa pienemmäksi? Sama useille mulle vastaaville `map/reduce/filter`-rakenteille - erottakaa funktioihin jos mahdollista - `searchpage/index.jsx:132`: muista että myös `null` estää elementin renderöinnin, ja on Reactille oikeampi tapa. `undefined` aiheuttaa poikkeustilanteen. ##### Vähäinen - Harkitkaa olisiko `development`/`production`-tarkistukset hyödyllisiä, esim. `REMOTE_API_URL`-vakion kanssa? Minifiointi poistaisi tällöin turhan localhost-muuttujan. - Onko `loginpage` sopiva ilmaisu esim. layoutin osalle enää, jos kirjautumista ei ole? - Muistakaa että SCSS-tiedosto menee jälkikäsittelyn läpi ja pakkaa esim. kuvat staattisiin resursseihin sen mukaan - tämän vuoksi esim. `header-logo` toimii! ### Backend ##### Kriittinen - `service.SurveyResponseService`-luokan funktiot ovat keskimäärin hyvin pitkiä; joskus pitkät funktiot ovat melko vääjäämättömiä, mutta tässä tilanteessa ne olisi myös syytä dokumentoida hyvin - niiden toimintalogiikka ei aukea kovin helposti pikaisella katsomisella, ja niihin pesiytyy helposti bugeja. - Edelliseen, harkitkaa myös onko niissä jotain mitä on mahdollista pirstoa pienempiin palasiin, esim. `getSurveyResponsesByVariableIdAndIndependentVariables` vs `getSurveyResponsesByVariableIdAndIndependentVariable` näyttäisivät melko selviltä kandidaateilta - yksikkötapaus voidaan monesti käsitellä tehokkaasti monikkotapauksen kaltaisena, ja esim. järjestyksen asettamisen voisi pirstoa eri funktioon. Hyödyntäkää rohkeasti IDE:n refaktorointitoimintoja - se tekee tylsät toimenpiteet teidän puolestanne, kun te heilutatte tahtipuikkoa!. ##### Tärkeä - Deprecated-endpointit on syytä poistaa kokonaan (tai ainakin kommentoida mäppäykset/annotaatiot pois) jollei niitä käytetä, muuten ne tulkitaan käytettäviksi - `model.Result`-luokan konstruktoreissa on runsaasti duplikoivaa koodia; nämä kannattaa yhdenmukaistaa `this()`-kutsuilla konstruktoreiden alussa - Harkitkaa `model`-luokkien dokumentoinnin täydentämistä; onko muuttujilla jotain semanttista merkitystä, joka ei selviä pelkästään muuttujan nimestä? Se voi helpottaa jatkokehittäjän perehtymistä koodiin. - `service.ThemeService`: mistä syystä "fi"-kielelle otetaan kaikki, mutta muille vain tietty kieli? Samankaltainen huomio `VariableService`::le - eikö kaikkia kieliä pitäisi kohdella yhdenmukaisesti? Jos erilaiseen kohteluun on peruste, se on syytä dokumentoida - `dao.SurveyResponseDataAccessService`: kovin pitkiä SQL-kyselyitä, olisiko aiheellista dokumentoida tarkemmin? ##### Vähäiset huomiot - `@CrossOrigin`-annotaatio on hyvä kehitysympäristössä, mutta ei välttämättä sovelias tuotantoympäristöön, riippuen siitä halutaanko sallia sivuston ulkopuolelta tulevat API-kutsut selaimella tehtynä (tämä on selaimia koskettava turvaohje, ei yleinen ehdoton rajoite!). Tämän voi muuttaa esim. profiileilla siten että vain kehitysympäristössä sallitaan ristilähteistä. - Useista `api`-luokista (ja muualtakin, esim. `Dao`-rajapinta) puuttuu Javadoc osasta julkisia määritelmiä. On hyvää koodaustapaa dokumentoida ne edes lyhyesti (esim. mitä palauttaa, milloin aiheuttaa poikkeuksen jos aiheuttaa, mitä parametreilta edellytetään, mitä sivuvaikutuksia aiheutuu). `@Override`-annotoituja metodeja ei välttämättä tarvitse erikseen dokumentoida, ellei käytös poikkea olennaisesti rajapinnassa dokumentoidusta ## Testiympäristössä toteutetut testit Jotta katselmointi olisi mahdollisimman kattava, katselmoinnin yhteydessä asennettiin oma paikallinen instanssi ohjelmistosta ja todennettiin asennusohjeiden toimivuus käytännössä. Tämän yhteydessä löytyi seuraavia ongelmia ### API-testit APIa (ei käyttöliittymä, suoraan `theme` ja y.m. endpointit) kokeiltiin käsin, tietoisena siitä miten se toimii. Seuraavia havaintoja ##### Kriittinen Tietokannasta **näyttäisi pääsevän käsiksi sellaisiin muuttujiin joilla ei ole teematunnistetta laisinkaan**: `http://127.0.0.1:8088/response/fi/variable/173/independent_variables/%20/%20/` vs `http://127.0.0.1:8088/response/fi/variable/12/independent_variables/%20/%20/`. Joku nokkela voisi hyvinkin enumeroida raalla voimalla tämän läpi - olisiko aiheellista tarkistaa esim. `EXISTS`-kyselyllä että kyseinen muuttuja tosiaankin on merkitty jollekin teemalle, ja siten käyttöliittymälle saatavissa? ##### Tärkeä Osoite `/theme/-1` tuottaa `500 Internal Error`-virheen. Tämä vihjaa jonkinlaiseen käsittelyongelmaan. Tarkempi lokitieto tässä: ``` org.springframework.dao.EmptyResultDataAccessException: Incorrect result size: expected 1, actual 0 at org.springframework.dao.support.DataAccessUtils.nullableSingleResult(DataAccessUtils.java:97) ~[spring-tx-5.2.5.RELEASE.jar:5.2.5.RELEASE] at org.springframework.jdbc.core.JdbcTemplate.queryForObject(JdbcTemplate.java:784) ~[spring-jdbc-5.2.5.RELEASE.jar:5.2.5.RELEASE] at com.kodavi.dao.ThemeDataAccessService.selectById(ThemeDataAccessService.java:47) ~[classes/:0.0.1-SNAPSHOT] at com.kodavi.dao.ThemeDataAccessService$$FastClassBySpringCGLIB$$f23fda07.invoke() ~[classes/:0.0.1-SNAPSHOT] at org.springframework.cglib.proxy.MethodProxy.invoke(MethodProxy.java:218) ~[spring-core-5.2.5.RELEASE.jar:5.2.5.RELEASE] at org.springframework.aop.framework.CglibAopProxy$CglibMethodInvocation.invokeJoinpoint(CglibAopProxy.java:771) ~[spring-aop-5.2.5.RELEASE.jar:5.2.5.RELEASE] at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:163) ~[spring-aop-5.2.5.RELEASE.jar:5.2.5.RELEASE] at org.springframework.aop.framework.CglibAopProxy$CglibMethodInvocation.proceed(CglibAopProxy.java:749) ~[spring-aop-5.2.5.RELEASE.jar:5.2.5.RELEASE] at org.springframework.dao.support.PersistenceExceptionTranslationInterceptor.invoke(PersistenceExceptionTranslationInterceptor.java:139) ~[spring-tx-5.2.5.RELEASE.jar:5.2.5.RELEASE] at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:186) ~[spring-aop-5.2.5.RELEASE.jar:5.2.5.RELEASE] at org.springframework.aop.framework.CglibAopProxy$CglibMethodInvocation.proceed(CglibAopProxy.java:749) ~[spring-aop-5.2.5.RELEASE.jar:5.2.5.RELEASE] at org.springframework.aop.framework.CglibAopProxy$DynamicAdvisedInterceptor.intercept(CglibAopProxy.java:691) ~[spring-aop-5.2.5.RELEASE.jar:5.2.5.RELEASE] at com.kodavi.dao.ThemeDataAccessService$$EnhancerBySpringCGLIB$$716a7c38.selectById() ~[classes/:0.0.1-SNAPSHOT] at com.kodavi.service.ThemeService.getThemeById(ThemeService.java:55) ~[classes/:0.0.1-SNAPSHOT] at com.kodavi.api.ThemeController.getThemeById(ThemeController.java:58) ~[classes/:0.0.1-SNAPSHOT] at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[na:1.8.0_262] at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[na:1.8.0_262] at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:1.8.0_262] ``` Samanlaista tulosta tulee monilla muilla kokonaisluvuilla, ja myös `variable`-endpointien kanssa. Virheviesti vihjaa siihen että dataa puuttuu- tilannetta ei käsitellä hallitusti - tämä ei olisi ollenkaan niin vakavaa, jos virheviesti olisi ilmeisesti itse kirjoitettu ja tilanne käsitelty. ### Frontend ##### Tärkeä - Suoritettaessa `npm install`, NPM varoittaa riippuvuuksissa ilmenevistä muutamista kriittisistä puutteista seuraavasti: ``` Found 4964 vulnerabilities (4957 low, 7 high) in 1988 scanned packages run `npm audit fix` to fix 4959 of them. 4 vulnerabilities require semver-major dependency updates. 1 vulnerability requires manual review. See the full report for details. ``` Low-tyypin riippuvuuksia on aika silmiäkääntävä määrä, mutta ne näyttäisivät pikaisella katsomisella koskettavan lähinnä tiettyä rajatapausta. Enemmän kiinnitäisin huomiota high-tyyppiin - nekään eivät välttämättä kosketa tätä sovellusta, mutta niistä on syytä olla tietoisia. Harkitkaa, olisiko riippuvuuksien päivitys mahdollista ja aiheellista, ja kuinka paljon työtä se vaatisi. ##### Vähäinen - `Browserslist: caniuse-lite is outdated. Please run the following command: npx browserslist --update-db`. Tehkää tämä seuraavan fronttikäännöksen yhteydessä