Niepuste tytuły wydarzeń

[lubczanska]

Dla "zwykłych" wydarzeń dodaje do formularza rezerwacji wydarzenia wymaganie niepustego tytułu. Dla wydarzeń typu egzamin/kolokwium dodaje wymaganie wybrania przedmiotu z listy (dla nich w miejscu tytułu wyświetlamy nazwę przedmiotu). W efekcie nigdy nie mamy problemu niewidocznego tytułu.

Usuwa też wymaganie niepustego opisu wydarzenia i zmienia checkbox publicznego wydarzenia na domyślnie zaznaczony.

Rozwiązuje #1487

[lgpawel]

Dla większej logicznej spójności z analogiczną zmianą dla formularza (wymaga zregenerowania migracji)

Suggested change

	visible = models.BooleanField(verbose_name='Wydarzenie jest publiczne', default=False)
	visible = models.BooleanField(verbose_name='Wydarzenie jest publiczne', default=True)

[lgpawel]

Jeśli nie ma konkretnego powodu, dla którego nie wywołujemy na początku metody clean z nadklasy, to warto to zrobić.

[lgpawel]

Byłoby w sumie dobrze, gdyby przy polach wyboru przedmiotu / tytułu pokazywała się gwiazdka sugerująca użytkownikowi, że to pole jest wymagane. Tyle tylko, że nie można tego załatwić, przez required = True, bo… nie są one wymagane (ogólnie, tylko warunkowo). Jest tu jakieś eleganckie wyjście?

[AThit7]

Zmiany:

• na początku metody clean wołana jest metoda z nadklasy, bo nie widzę ku temu przeciwskazań
• pole visible wydarzenia domyślnie ustawione jest na True(spójność modelu z formularzem), zregenerowano migrację
• czerwone gwiazdki dodane poprzez dodanie ich do labeli pól wymaganych warunkowo (chyba brzydkie rozwiązanie, ale nic lepszego nie wymyśliłem)
• zmiana w pliku apps/api/rest/v1/api_wrapper/sz_api/models.py została wprowadzona zgodnie z wytycznymi lintera, ale jest ona już na master-dev d27794c

[lgpawel]

To nie jest pytanie retoryczne: czy tak byłoby przejrzyściej, czy wręcz przeciwnie? Pewne testy przestają się powtarzać, ale to wcale nie musi być krok w dobrą stronę...

Suggested change

	if cleaned_data['type'] not in (Event.TYPE_EXAM, Event.TYPE_TEST):
	cleaned_data['course'] = None
	if cleaned_data['type'] != Event.TYPE_GENERIC:
	cleaned_data['title'] = None
	if cleaned_data['type'] not in (Event.TYPE_EXAM, Event.TYPE_TEST):
	cleaned_data['course'] = None
	elif cleaned_data['course'] == None:
	raise # wyjątek
	if cleaned_data['type'] != Event.TYPE_GENERIC:
	cleaned_data['title'] = None
	elif cleaned_data['title'] in ('', None):
	raise # wyjątek

[AThit7]

Początkowo wydawało mi się, że wersja pierwotna jest trochę bardziej przejrzysta, bo bloki kodu (te oddzielone pustymi liniami) dotyczą odrębnych kwestii łatwiejszych do zauważenia. W pierwszym czyścimy niepotrzebne pola, a w drugim sprawdzamy, czy pola obowiązkowe są wypełnione. W zasugerowanej zmianie te bloki są podzielone względem tego, którym z pól cleaned_data się zajmujemy, co moim zdaniem jest trochę mniej czytelne. Przy dodawaniu kolejnych pól (np. thesis) będziemy dodawać kolejne bloki zamiast po jednym ifie do każdego z dwóch bloków. Może tak będzie lepiej a może nie, ale warto wziąć to pod uwagę. Na początku ten elif ... wydał mi się mało czytelny, w pierwotnej wersji można usunąć else a elif zamienić na if (to nie ja pisałem tamten fragment) i przejrzystość moim zdaniem jeszcze bardziej się poprawi (a liczba testów ani logika się nie zmieni). Ogólnie to im dłużej o tym myślę, tym bardziej odnoszę wrażenia, że czytelność obu wersji jest taka sama.