Review TODOs in code

[ReneB]

Review TODOs in code

Voor MVP

• settings/production.py:12-13
  fixen bij deployment. Alles via https, s'il vous plaît, no good reason not to. Redirect ook alles naar SSL. Moved to Deploy MVP on server #88
• apps/registrations/services.py:36
# TODO: This produces a fairly complex query, which should be checked for performance
  Moet zeker gebeuren, want critical path voor inschrijven. Kunnen we daar een benchmark-run voor doen, die een zooi events en users en registrations aanmaakt, en dan in scenario 1) optimistisch gewoon de registration aanmaakt of in scenario 2) de boel checkt? Dan kunnen we het verschil meten. Natuurlijk is de versie uit scenario 1 niet bruikbaar voor registrations, maar als we het verschil weten kunnen we kijken of we moeten refactoren en/of (pre-)cachen oid.
• - apps/people/models/medical_details.py:7
# TODO: dit afmaken nadat we een knoop hebben doorgehakt over wat we precies willen vragen en opslaan. zie ook issue 42
  Knoop is volgens mij gehakt, en volgens mij is wat er nu is gemaakt redelijk voldoende?
• - apps/events/templates/events/snippets/event_detail.html:14
TODO: prijsbepaling met allerlei opties enzo
  De 'registrations' hebben al een price, maar in de events zit die nog niet meegenomen. Moet dat met allerlei QExprs e.d. worden uitgebreid?
• - apps/events/templates/events/snippets/registered_event.html:14
{% trans "Registration number" %}: TODO<br/>
  De 'registrations' hebben al een id, maar in de events zit die nog niet meegenomen. Moet dat met allerlei QExprs e.d. worden uitgebreid?
• apps/registrations/views.py:73: # TODO: if there are other active registrations, this page should probably indicate that these will also be Moved to Revise explanations offered by the system #64
• apps/registrations/views.py:159: # TODO: Check registration status? More generally, the registration procedure should verify that the registration is actually complete at the end (you can now just skip a skep by modifying the url), and this might be part of that.
• apps/registrations/forms.py:135: # TODO: Handle depends en apps/registrations/forms.py:139: # TODO: Handle depends zijn eigenlijk alleen relevant voor crew-inschrijvingen op dit moment, dus leuk maar niet essentieel. Moved to Handle registration option dependencies in frontend #100

Fixed

• apps/events/views.py:14
# TODO: really needed?
  Volgens mij niet. Ik kan de events/-url niet eens bezoeken, dan krijg ik een TemplateDoesNotExist dus ik heb hem maar weggegooid.
• arta/settings/common.py:158
# Normal password authentication. TODO: Needed?
'django.contrib.auth.backends.ModelBackend',
  Not needed. Als ik hem weghaal kan ik nog steeds inloggen in zowel de normale app als de admin-app, dus volgens mij kunnen we deze missen - en sterker nog, beter kwijt dan rijk aangezien Django iedere login-strategie gebruikt tot er één slaagt.
• apps/core/models/consent_log.py:34
# TODO: Enforce read-only at model level (e.g. only allow creating new instances, not saving existing ones)?
  Model is nu readonly - delete werkt niet meer en save alleen als er nog geen id is gegeven voor het object.
• apps/registrations/forms.py:90: # TODO: is this an object, or freeform Form? Link to conditions should be provided. Here (helptext) or in template?
• apps/registrations/views.py:23: # TODO: This should be a POST request

---

For later

• arta/settings/common.py - # TODO: social accounts weer aanzetten als we ze handig hebben gemaakt
• templates/base.html - {# TODO: deze pagina weer beschikbaar maken zodra we ook een edit-interface en 'bewaar info ja/nee' optie hebben uitgedacht #}
• app/core/forms.py - # TODO: When implementing social accounts, we should probably pre-fill these fields with data from user, as populated by the social provider if available.
• - apps/core/models/consent_log.py - # TODO: __str__? (lijkt me momenteel niet strikt noodzakelijk)
• arta/urls.py:14
# TODO: This does not work when a user is logged in, but does not have admin site permissions. admin.site.login = login_required(admin.site.login)
• apps/events/templates/events/snippets/registered_event.html:12
{# TODO: betaalstatus weergeven in kleur en tekst #}
  Moet waarschijnlijk nog wel gedaan worden.
• apps/registrations/forms.py:136: # TODO: Handle allow_change_until
• apps/registrations/forms.py:190: # TODO: Handle allow_change_until
• apps/registrations/views.py:68: # TODO: send verification e-mail for e-address after figuring out what to use
• apps/registrations/views.py:186:# TODO: Where should this live? Template snippet? Database? How about translations?
• apps/registrations/views.py:225: # TODO: Redirect elsewhere? Or Maybe remove this except clause when status is checked above?

[ReneB]

Nog te checken:

[matthijskooijman]

TODO: This does not work when a user is logged in, but does not have admin site permissions. admin.site.login = login_required(admin.site.login)
Deze snap ik niet goed genoeg om er iets zinnigs over te zeggen. Het lijkt voor mij, als ik met een non-privileged user inlog, precies te doen wat ik wil dat het doet - er zit geen "admin" link in de dropdown, en als ik de pagina gewoon via de url bezoek word ik er niet ingelaten. En als ik diezelfde dingen check als privileged user, is alles zoals je het zou verwachten.

DIt betekent dat als je als niet-admin user ingelogd bent en dan met de hand naar /admin gaat, dat het dan niet werkt (dan krijg je een error, de verkeerder loginprompt, zoiets). Niet echt relevant nu dus.

• apps/events/templates/events/snippets/registered_event.html:12
  {# TODO: betaalstatus weergeven in kleur en tekst #}
  Moet waarschijnlijk nog wel gedaan worden.

Betalingen houden we nog niet bij, dus voor later.

• apps/events/templates/events/snippets/registered_event.html:14
{% trans "Registration number" %}: TODO<br/>
  De 'registrations' hebben al een id, maar in de events zit die nog niet meegenomen. Moet dat met allerlei QExprs e.d. worden uitgebreid?

Hier had ik al even naar gekeken, maar annotaten met een model instance lijkt ingewikkeld tot onmogelijk, dus dat zou via een methode moeten denk ik. Ga ik nog even verder naar kijken. En als dit werkt, dan is meteen het volgende puntje (prijs bij een registration weergeven) ook makkelijk.

[matthijskooijman]

Kunnen we daar een benchmark-run voor doen, die een zooi events en users en registrations aanmaakt, en dan in scenario 1) optimistisch gewoon de registration aanmaakt of in scenario 2) de boel checkt? Dan kunnen we het verschil meten. Natuurlijk is de versie uit scenario 1 niet bruikbaar voor registrations, maar als we het verschil weten kunnen we kijken of we moeten refactoren en/of (pre-)cachen oid.

Ik heb de queries die het relevants zijn bekeken met SQL EXPLAIN en waar nodig wat indexen toegevoegd, dus dat aspect zou nu ok moeten zijn. Volgens mij is de query waar dit om ging nu ook niet heel spannend (wel complex, maar nu goed met indexen te doen). Voor verdere performance testen heb ik #93 gemaakt, dus dat is hier nu klaar.

Verder heb ik alle TODOs nagelopen en ze onderverdeeld in voor nu en voor later. De meeste en belangrijkste zijn inmiddels gefixt.