Refactoring/Class extraction and general cleanup.

martinsabo commented 8 years ago

Trochu som Uvobota upratal (ako prípravu pre Discourse posting). A mierne zmenil texty (odstránený counter). Pripomienky sú vítané.

jsuchal commented 8 years ago

Ok, ked ideme refactor, tak poriadne. :+1: Dopisal som nejake komenty. Inak, pomaly a isto vznika potreba pre nejake testy. Trufnes si? Ja preferujem rspec. Inak diky, celkom dobre.

martinsabo commented 8 years ago

Dík za code review. RSpec nie je problém. Skúsim nieco draftnúť.

martinsabo commented 8 years ago

Zapracoval som pripomienky. Counter je späť. Parsuje to teraz len prvú stránku. Po stránkach sa nedá presúvať napriamo pomocou url. Ak budeme chcieť spracovať všetky stránky (napr. ak by mal Uvobot fungovať pre iné skupiny ako IT) tak bude pravdepodobne treba schopnejšiu knižnicu napr. Mechanize.

Specs su prázdne, pozriem sa na ne v ďalšom kole.

Btw Hound je naschvál nastavený, aby upozorňoval na single quotes? Alebo je to len nejaký default.

jsuchal commented 8 years ago

Ad Hound. Ja som tiez zvyknuty na singe quotes ale predpokladam, ze to vychadza zo stylu https://github.com/thoughtbot/guides/tree/master/style/ruby v klude sa ho mozeme drzat.

martinsabo commented 8 years ago

Zhrniem to:

WebMock - ja som za každú simplifikáciu, skúsim to prerobiť s Curl doublom.
Timecop je tam kvôli tomu aby sedel dátum fixtúry s Date.today, toho by sa tiež dalo zbaviť keby tá metóda berie dátum ako parameter.
Z názvu metódy som naschvál vynechal "found", lebo ona podľa implementácie môže aj nenájsť, vtedy je 0 oznamov. A found mi znelo ako úspešné dokonanie. Celkovo tá metóda robí dve veci a skôr by som ju mal rozdeliť na matching_announcements_found a na no_announcements_found.
Pravdupovediac neviem čo by som do Uvobot specu dal.

jsuchal commented 8 years ago

Kukal som este na to, podla mna UvoScraper robi vela. Jasny znak toho je privatna metoda #parse_page. Schovava sa tam Parser. (Inak privatna metoda je podla mna smell, ktory treba cim skor vytiahnut do separe objektu a injektnut zhora.)

Cize podla mna

class UvoScraper
  SEARCH_URL = "https://www2.uvo.gov.sk/evestnik?p_p_id=evestnik_WAR_eVestnikPortlets&p_p_lifecycle=1&p_p_state=normal&p_p_mode=view&p_p_col_id=column-2&p_p_col_pos=1&p_p_col_count=2".freeze
  NEW_ISSUE_URL = "https://www2.uvo.gov.sk/evestnik/-/vestnik/aktual".freeze
  IT_CONTACTS_CODE = 72

  def initialize(parser, http_client: Curl)
    @parser = parser
    @http_client = http_client
  end

  def issue_ready?(time)
    html = @http_client.get(NEW_ISSUE_URL).body
    @parser.issue_for_date?(html, time)
  end

  def get_announcements(time)
    date = time.strftime("%d.%m.%Y")
    search_query = { cpv: IT_CONTACTS_CODE, datumZverejneniaOd: date, datumZverejneniaDo: date }
    html = @http_client.post(SEARCH_URL, search_query).body
    @parser.parse_announcements(html)
  end
end

UvoParser nasledne vies pekne otestovat.

UvoScraper testy sa stanu trivialne, lebo len testujes rozhodovaciu logiku.

Moze byt?

martinsabo commented 8 years ago

O tom viem, že Scraper nie je "solid" :). Samozrejme úvaha je správna a treba ho rozbiť. IT_CONTACTS_CODE by nemal prísť tiež zhora, aby sa dal uvobot okrem dátumu konfigurovať aj pre inú oblasť ako IT?

jsuchal commented 8 years ago

V principe ten kode moze ist zhora, ale kedze to nepotrebujeme, tak som to nechal tam. Ma to byt IT_CONTRACTS_CODE

martinsabo commented 8 years ago

Co by si testoval v uvobot specu, aby to teda nebolo "smelly" ako moj posledny pokus?

jsuchal commented 8 years ago

Ad ten Uvobot test. Zodpovednost toho objektu je posielat notifikacie, cize treba testovat to. Asi si velmi nepomozeme a bude to treba nechat tak.

martinsabo commented 8 years ago

Ad Uvobot test. Presne to mi z toho vyslo nech som sa na to pozeral z ktorejkolvek strany.

jsuchal commented 8 years ago

Este jedna prosba, presun vsetky .rb subory okrem toho uvobot.rb co sa spusta do lib/

martinsabo commented 8 years ago

Nespravim z toho rovno gem? Ci az ked sa dohodneme, ze uz je to good enough?

jsuchal commented 8 years ago

Gem je podla mna zbytocny, ked bude treba tak to spravime. Dalsi krok je #2. To by nam velmi pomohlo.

martinsabo commented 8 years ago

Na to sa chystam. Idealne vo zvlast pull requeste.

jsuchal commented 8 years ago

@martinsabo jasne. Toto cim skor uzavrieme, nahodime travis - ci alebo codeship. A potom.

martinsabo commented 8 years ago

Este daco?

jsuchal commented 8 years ago

Zdvihnime toho hounda na 120 znakov lebo 80 je dnes dost krute.

jsuchal commented 8 years ago

Potrebujem este rebase na mastra, tam som menil nejake male veci.

slovensko-digital / uvobot

Refactoring/Class extraction and general cleanup. #5