-
Notifications
You must be signed in to change notification settings - Fork 10
Evaluation student #422
base: staging
Are you sure you want to change the base?
Evaluation student #422
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.
Hi Nico,
thanks for the PR, I had a first glance. Are you on our slack too?
Couple of comments:
- Why did you delete docker-compose.dev?
- Please revert changes to the backup, deploy.sh and the yml configs.
- I will add more comments to the source
- Please consider adding a logger (import logging, logger = logging.getLogger(name)
tonight :) |
This concerns #226 |
migrations.CreateModel( | ||
name='StudentEvaluation', | ||
fields=[ | ||
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), |
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.
I think in this project we have never used verbose_name
before. This usage here is straight-forward, but it could create confusion with the additional translation layer later on and the separate setting of labels in the form class. I'm thinking about whether this is helping or complicating the understanding in other code files.
backend/apps/evaluation/templates/evaluation/studentevaluation_form.html
Outdated
Show resolved
Hide resolved
@maltezacharias Does one logger for each file make sense? I would add one globally like |
Am I the only one getting a |
Made base Model abstract; registered Model
|
||
operations = [ | ||
migrations.AlterField( | ||
model_name='newsletter', |
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.
Why do you add a migration for newsletter
?
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.
That was unintentional, Ill fix it in the next commits.
backend/apps/evaluation/forms.py
Outdated
'wirst?'), | ||
'contact_mail': _('Hier kannst du deine Mail da lassen, falls du uns für Rückfragen zur Verfügung stehen ' | ||
'möchtest'), | ||
'communication_with_institutions': _('Wie lief für dich die Kommunikation mit den Institutionen?'), |
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.
Aren't these questions a bit too open to get helpful answers?
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.
Those are the questions initially suggested in the typeform (see slack). I too think that it might be helpful to be more specific, but I havent really spent a lot of time to think about possible alternatives – so I'm open to any suggestions.
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.
When can query statistics like how many hospitals contacted a student on average etc. but we don't know anything about the process after they were contacted. Maybe something like "How clear were the requirements of the hospitals?", "Did you receive offers which were interesting for you?"...
Please run Please also address @bjrne's and @maltezacharias's comments and/or resolve them. |
Ja, einer pro File macht absolut Sinn, EDIT: |
The missing translations block this PR now |
Are you still at this @n-hackert ? :) |
Moin ihr Lieben,
ich hatte über Ostern ein bisschen Zeit, um mal meine Python-"Skills" wieder aufzufrischen und habe mich in Django eingelesen. Da ich mit Fabi ja mal an den Feedback-Typeforms gearbeitet habe, hab ich einfach mal versucht, damit anzufangen, die in die Codebase zu integrieren. Ich würde mich freuen, wenn sich das jemand von euch kurz anschauen und n kurzes Statement dazu abgeben könnte, ob ihr den Ansatz sinnvoll findet. (Is auch nicht viel Code :D)
Liebe Grüße und noch einen schönen Ostermontag!
Nico