-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had a quick look and it seems to be fine so far. But I was not that deeply involved when schedules were introduced, so there might be some issues spotted by the others.
The only thing I noticed: You implemented a redirect for the legacy schedule pages and I'm not sure if this is really necessary. Were these links distributed somewhere? Otherwise it might be clean to avoid legacy code chunks. But head of Ophase should decide this for now.
@@ -152,6 +152,8 @@ class Meta: | |||
name = models.CharField(max_length=100, verbose_name=_('Name')) | |||
description_template = models.CharField(max_length=50, verbose_name=_('Beschreibung in Template')) | |||
lang = models.CharField(max_length=5, verbose_name=_('Sprachcode'), default="de") | |||
details_template = models.CharField(max_length=50, verbose_name=_('Template für Stundenplan View')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could replace these two fields with a slug field containing an url name which could also be used as a template name. There should also be a method getSlug()
, which uses the name of the categorie if the slug field is empty. This avoids actions requiered by the user when upgrading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The slug would then be used to lookup the corresponding category in the website model (enforce unique).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would introduce manual interactions as well as additional fields that are obsolete as soon as #143 is done. Please consider implementing it directly.
All relevant functionality was introduced with #144 -- thus this pull request will be closed without being merged. |
See #141
This is not the cleanest solution, this would need mor changes on existing models. But it fixes the mentioned bug and it is important that pyophase can be used for the SommerOphase as soon as possible.