Skip to content
This repository has been archived by the owner on Mar 27, 2023. It is now read-only.

Evaluation student #422

Draft
wants to merge 41 commits into
base: staging
Choose a base branch
from

Conversation

n-hackert
Copy link

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

Copy link
Collaborator

@maltezacharias maltezacharias left a 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)

backend/apps/accounts/models.py Outdated Show resolved Hide resolved
@maltezacharias
Copy link
Collaborator

@feeds @josauder any comments?

Copy link
Collaborator

tonight :)

@maltezacharias
Copy link
Collaborator

This concerns #226

migrations.CreateModel(
name='StudentEvaluation',
fields=[
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
Copy link
Member

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.

@Baschdl
Copy link
Collaborator

Baschdl commented Apr 14, 2020

* Please consider adding a logger (import logging, logger = logging.getLogger(**name**)

@maltezacharias Does one logger for each file make sense? I would add one globally like match4healthcare

@Baschdl
Copy link
Collaborator

Baschdl commented Apr 14, 2020

Am I the only one getting a django.db.utils.OperationalError: FATAL: no pg_hba.conf entry for host [...] error when running docker-compose -f docker-compose.dev.yml -f docker-compose.prod.yml up --build?


operations = [
migrations.AlterField(
model_name='newsletter',
Copy link
Collaborator

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?

Copy link
Author

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.

'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?'),
Copy link
Collaborator

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?

Copy link
Author

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.

Copy link
Collaborator

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?"...

@Baschdl
Copy link
Collaborator

Baschdl commented Apr 29, 2020

Please run django-admin makemessages -l en --no-location and translate your new strings. More explanation in the README.

Please also address @bjrne's and @maltezacharias's comments and/or resolve them.

@maltezacharias maltezacharias linked an issue May 2, 2020 that may be closed by this pull request
@maltezacharias
Copy link
Collaborator

maltezacharias commented May 2, 2020

* Please consider adding a logger (import logging, logger = logging.getLogger(**name**)

@maltezacharias Does one logger for each file make sense? I would add one globally like match4healthcare

Ja, einer pro File macht absolut Sinn, __name__ muss verwendet werden, dann taucht im Logging auch das modul mit auf.

EDIT:
logger = logging.getLogger(__name__)

@Baschdl
Copy link
Collaborator

Baschdl commented May 2, 2020

The missing translations block this PR now

Copy link
Member

Are you still at this @n-hackert ? :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include feedback forms
5 participants