hajame / warehouse

Tietokantasovellus-harjoitustyö. Aihe: Varastonhallinta. Python, Flask, Postgresql + SQLAlchemy datamodels. HTML, CSS, Bootstrap
1 stars 0 forks source link

Koodikatselmus 2 #2

Closed anonOstrich closed 6 years ago

anonOstrich commented 6 years ago

Koodikatselmointi

Ladattu 27.8. klo 16:37

Dokumentoinnista

Dokumentointi on erittäin yksityiskohtainen ja vastaa melko hyvin sovelluksen tilaa. Erityisesti dokumentoinnin rakenne on helppo hahmottaa sopivien otsikoiden valinnalla, ja käyttöohjeen kuvat takaavat ymmärrettävyyden.

Huomioita koodista

Tiedostossa "application/models.py" määritellään abstrakti Base-luokka. Luokan perii kuitenkin vain yksi luokka - User. Monella muullakin luokalla on id, joten olisi ehkä luonnollista periä luokka. Joka tapauksessa abstrakti luokka ei säästä koodia, jos sillä on vain yksi perijä. Osassa views.py-tiedostoja importataan login_required itse toteutetusta tiedostosta, osassa taas flask_login moduulista. Helpointa lienee kaikissa importata oma versio, jos se on vain laajennettu versio toisesta dekoraattorista.

Käyttökokemuksesta

Esineen lisääminen olemassaolevaan varastoon on melko työlästä. On ensin katsottava mikä on sen varaston nimi, johon esinettä laitetaan. Sitten on erillisellä sivulla täytettävä esineen tiedot ja syötettävä varaston nimi. Miellyttävintä olisi, että olisi vaihtoehto lisätä esinettä varaston omalla sivulla. Prosessia helpottaisi helpottavasti myös se, että esineenlisäyssivulla voisi vaikka ruksia haluamansa varaston, tai että edes käyttäjän varastojen nimet näytettäisiin.

Jos esinehaussa lomakkeen validointi epäonnistuu (esim. tyhjä esineen nimi), tyhjentyy esinelista sivulla. Liittyvä metodi ei vain lisää kyseisessä tapauksessa sivun käyttöön esinekokoelmaa.

Onnistuneen esinehaun tuloksena näytetään käyttäjän varastot, joissa tuotetta löytyy, ja näiden vieressä linkki kyseisen varaston sivulle. Linkki ei kuitenkaan vie pois sivulta. Kyse saattaa olla ongelmasta form formin sisällä. Ilmeisesti lisäämällä hakukentälle form-attribuutti, olisi toiminnallisuus mahdollista: https://stackoverflow.com/questions/3430214/form-inside-a-form-is-that-alright toinen vastaus.

Dokumentaatiossa on mainittu toteutetuksi toiminto, joka estää varaston ylitäyttymisen. Tarkastusajankohtana oli kuitenkin mahdollista luoda varasto jonka tilavuus on 10 ja lisätä sinne tilavuuden 100 esine onnistuneesti.

Turvallisuus/ muokkaamattomat virheviestit

Jos kirjautumaton käyttäjä yrittää päästä esimerkiksi osoitteeseen /warehouses, saa hän vastaansa virheviestin, joka paljastaa serveristä jonkin verran tietoja. Virheen syynä on itse määritelty login_required-dekoraattori, jossa kutsutaan current_user.is_authenticated(); kun current_useria ei ole olemassa, metodin kutsu aiheuttaa virheen. Esimerkiksi sulkujen poistamisen pitäisi ratkaista asia. Toki tähän virheeseen ajautuminen vaatii jonkin verran käyttäjän aktiivista yrittämistä, sillä etusivulla ei ei-kirjautuneille vierailijoille näytetä linkkejä sivuille joissa kirjautumista vaadittaisiin.

Esineen hakeminen aiheuttaa samantyyppisen virheviestin, jos syöte ei ole täsmälleen olemassaolevan esineen nimi. Parempi olisi näyttää selkokielisempi ja vähemmin paljastava virhe.

Hyvä, että käyttäjälle näytetään vain tälle kuuluvat varastot: käyttäjillä ei ole suoria linkkejä kiellettyihin resursseihin. On tärkeää kuitenkin varmistaa myös palvelimella, että senhetkinen käyttäjä todella on varastoon littetty omistaja. Nyt käyttäjä pystyy näkemään ja ennen kaikkea muokkaamaan kenen tahansa varastoa, kunhan lähettää pyytönsä oikeisiin osoitteisiin. Koska varastosivujen osoitteet ovat muotoa /warehouses//single ja id:t numeroidaan ykkösestä kasvavasti, on helppo numeroa muuttamalla selata kaikki sivustolla olevat varastot läpi.

Lopuksi

Puutteita on aika nopea korjata, eikä syvällisempiä ongelmia näy. Jaksamista loppusuoralle!

hajame commented 6 years ago

Kiitos kovasti kattavasta palautteestasi! current_user.is_authenticated() metodista sulkeet poistamalla tosiaan saatiin tuo virhe korjattua, suurkiitos!

Korjasin myös esinehaun listabugin, kiitos tästäkin vinkistä :+1:

Sisäkkäiset formit saattavat olla syy mainitsemaasi bugiin. kuitenkin usein tilanne on, että vain ensimmäisenä listatun varaston linkki ei toimi. Yritin mainitsemaasi korjausta (kiitos muuten kun jaoit sen), mutta tämä ei näyttänyt auttavan asiaa. Opinpahan kuitenkin, miten useita formeja voi käsitellä form id:n kautta. Näyttää, että tämä bugi saattaa jäädä lopulliseen tuotokseen.