search

iiuni / projektzapisy

System Zapisów na zajęcia w Instytucie Informatyki Uniwersytetu Wrocławskiego
https://zapisy.ii.uni.wroc.pl
32 stars 10 forks source link

Poprawki do REST API związane z #657 #658

Closed cahirwpz closed 4 years ago

cahirwpz commented 5 years ago

Ogólnie zrobiliście z grubsza to czego potrzebuję. Dobra robota! :+1:

Oczywiście nie uzgodniliśmy wszystkiego w detalach, więc potrzebuję paru poprawek:

  1. Chcę mieć możliwość wyłączenia paginacji wyników.
  2. Dla tabeli Classroom chcę mieć możliwość edycji pola building.
  3. Byłoby miło gdybym dla Semester dostał możliwość zrobienia zapytania current.
  4. Dla Semester potrzebuję pól year i type zamiast display_name.
  5. Dla Classroom potrzebuję tylko pól: id, type, number, building, capacity i usos_id.
  6. Chcę mieć dostępu do odczytu dla CourseEntity.

Co do paginacji – rozumiem, że ona jest przydatna, jeśli używa się interfejsu webowego i myślę, że to dobre rozwiązanie. Natomiast z poziomu skryptów w Pythonie ona nic mi nie daje, a tylko komplikuje kod, jeśli miałbym to obsłużyć.

Nie jestem pewien czy to dobry pomysł, żeby konstruować zagnieżdżone rekordy, tj.:

Z grubsza oczekuję, że REST API będzie działało jak prosta baza danych key-value, bez żadnych fajerwerków. Wyniki mogę sobie cache'ować, żeby nie zamulać serwera pytaniami.

jasiekmarc commented 5 years ago
  1. Odmowa. Paginacja ma sens. Służy temu, żebyś nie blokował wątku na serwerze na zbyt długo swoim zapytaniem.
  2. Odmowa. Jeśli te dane są gdzieś niepoprawne, możesz powiedzieć Lewemu — on to poprawi. Ale nie potrzebujesz prawa zapisu do danych, które zasadniczo się nie zmieniają.
  3. Ok.
  4. Ok. Dodam.
  5. Nie jesteś jedynym użytkownikiem tego api.
  6. Masz. Poprzez model Course.

Jeśli chodzi o zagnieżdżanie — jest ono intencjonalne. Dzięki temu zawsze masz dostęp do USOS-owych kluczy i nie musisz pamiętać identyfikatorów z Systemu Zapisów w swoich skryptach.

cahirwpz commented 5 years ago

Przy aktualizacji pola usos_id dla Term kod SZ robi niepotrzebnie sprawdzanie semantyczne:

"usos_id": ["Istnieje już termin z tą wartością pola Kod terminu w systemie USOS."]

W tym przypadku jest ok, że usos_id dla dwóch różnych terminów z SZ jest takie samo. W tym przypadku usos_id odnosi się do DZ_TERMINY, a nie DZ_TERMINY_GRUP.

cahirwpz commented 5 years ago
  1. Odmowa. Paginacja ma sens. Służy temu, żebyś nie blokował wątku na serwerze na zbyt długo swoim zapytaniem.

To jest rzeczywisty problem? Czy próba optymalizacji przypadku, który zdarza się bardzo rzadko. Nie próbujesz przerzucać odpowiedzialności na klienta API?

  1. Odmowa. Jeśli te dane są gdzieś niepoprawne, możesz powiedzieć Lewemu — on to poprawi. Ale nie potrzebujesz prawa zapisu do danych, które zasadniczo się nie zmieniają.

Ok. Sugeruję zatem możliwość wyboru z listy, a nie jako wpisywany z palca ciąg znaków.

  1. Masz. Poprzez model Course.

Nalegam na takie zaprojektowanie API by było jednorodne. Dzięki temu moje API dostępowe będzie miało do rozważenia mniej przypadków. Na prawdę nie rozumiem przyczyny za potraktowaniem tej tabeli w inny sposób niż pozostałe.

Dzięki temu zawsze masz dostęp do USOS-owych kluczy i nie musisz pamiętać identyfikatorów z Systemu Zapisów w swoich skryptach.

Ten argument absolutnie mnie nie boli. Co więcej zrobienie tego w sposób jaki zasugerowałem zmniejsza złożoność mojego API i prawdopodobnie zmniejsza obciążenie bazy danych pojedynczym zapytaniem.

jasiekmarc commented 5 years ago

Przy aktualizacji pola usos_id dla Term kod SZ robi niepotrzebnie sprawdzanie semantyczne:

"usos_id": ["Istnieje już termin z tą wartością pola Kod terminu w systemie USOS."]

W tym przypadku jest ok, że usos_id dla dwóch różnych terminów z SZ jest takie samo. W tym przypadku usos_id odnosi się do DZ_TERMINY, a nie DZ_TERMINY_GRUP.

Po co w ogóle jest Ci pole usos_id w modelu Term, jeśli nie jest to identyfikator?

cahirwpz commented 5 years ago

Jest to identyfikator, ale odnoszący się do terminu (w oderwaniu od zajęć), a nie terminu grupy. Nie wszystkie rzeczy mapują się 1:1 między SZ i USOS. Poza tym grzecznie proszę :)

jasiekmarc commented 5 years ago

Co to znaczy „termin w oderwaniu od zajęć”?

cahirwpz commented 5 years ago

Proszę, odpowiednio definicje DZ_TERMINY i DZ_TERMINY_GRUP:

dz_terminy dz_terminy_grup

cahirwpz commented 5 years ago

Z grubsza chodzi o to, że w SZ tabela Term jest zaprojektowana nieefektywnie. Bardzo dużo wierszy ma identyczne pola dayOfWeek, start_time, end_time. W USOS zrobiono to lepiej i takie pola wydzielono do osobnej tabeli.

EDIT: Żeby zrobić to lepiej możemy utworzyć dodatkowe pole usos_term_id.

cahirwpz commented 5 years ago

Pozwoliłem sobie dodać etykietę hot, ze względu na opóźnienie w planowanej dacie eksportu danych z SZ do USOS.

jasiekmarc commented 5 years ago

W USOS zrobiono to lepiej i takie pola wydzielono do osobnej tabeli.

Ok. Rozumiem. Będę się spierał, że rozwiązanie z USOSa wcale nie jest lepsze.

Czyli w tabeli DZ_TERMINY_GRUP pole ID jest unikalnym identyfikatorem? Jeśli tak, to umówmy się, że będziesz umieszczał tę właśnie liczbę w polu Term.usos_id.

cahirwpz commented 5 years ago

Tak DZ_TERMINY_GRUP.ID jest unikatowy. Możemy się umówić, że usos_id będzie się odnosił właśnie do niego. Zatem poproszę jeszcze o pole usos_term_id w Term.

jasiekmarc commented 5 years ago

Odmowa. Skoro DZ_TERMINY_GRUP.ID jest unikatowy, to odpowiadający mu DZ_TERMINY.ID z niego wynika. Nie będziemy duplikować całej bazy USOSa w Systemie Zapisów.

Jak wspominałem, akceptuję punkty 3 i 4 jako udogodnienie. Na razie masz wszystko, czego potrzebujesz do przeprowadzenia eksportu tego semestru (id: 341), zatem zdejmuję etykietkę hot (ona służy do zgłaszania pilnych błędów, które uniemożliwiają korzystanie z systemu). Zajmę się nimi w bliżej nieokreślonej przyszłości.

jasiekmarc commented 4 years ago

Zamykam w związku z powstaniem wrappera #812.