Fixed Issues #4 #5 #6 #7 #8
Conversation
AlexanderKaschta
left a comment
AlexanderKaschta
left a comment
There was a problem hiding this comment.
Ich hab es mal durchgesehen und ein paar Anmerkungen notiert.
| flash('User successfully registered. Go to the login form to log in.', category='success') | ||
| return redirect(url_for('auth.login')) | ||
| # Log the user in immediately | ||
| login_user(user, remember=True) |
There was a problem hiding this comment.
Das sollte nicht standardmäßig eine angemeldet gebliebene Session sein
| # Register the user | ||
| user = User(name=username, isop=False, pin=calc_hash(form.pin.data)) | ||
| card_hash = None | ||
| if form.card_number.data: |
There was a problem hiding this comment.
Hier könnte man ggf. noch checken, ob die Eingabe passt (Passende Länge, nur Ziffern, etc.)
| </div> | ||
|
|
||
| <div class="d-grid gap-2"> | ||
| {{ form.submit(class="btn btn-primary btn-login text-uppercase fw-bold") }} |
There was a problem hiding this comment.
Der Button, sowie die beiden unteren auch, haben ein anderes Aussehen, wie alle anderen Buttons. Diese sollten angepasst werden, dass sie einheitlich aussehen.
| {{ form.repeat_pin(class='form-control') }} | ||
| {{ form.repeat_pin.label }} | ||
| <div class="form-floating mb-3"> | ||
| {{ form.repeat_pin(class="form-control", id="floatingRepeatPin", placeholder="1234") }} |
There was a problem hiding this comment.
Das placeholder-Argument wirkt redundant. Da das Label schwebt, wird der Placeholder-Text nicht angezeigt und sobald man drauf klickt, wird das Feld active, wodurch der Text standardmäßig verschwindet.
Dies ist auch keine Verbesserung in Richtung Accessibility, da Screenreader dies standardmäßig nicht vorlesen.
Gleiches gilt für die Stelle dadrunter.
| @@ -1,54 +1,39 @@ | |||
| {% extends 'base.html' %} | |||
|
|
|||
| {% block pagetitle %}Verify Purchase{% endblock %} | |||
There was a problem hiding this comment.
Der Titelblock sollte beibehalten werden, um dem Nutzer eine klare und eindeutige Navigation zu präsentieren.
|
|
||
| {% block pagetitle %}Verify Purchase{% endblock %} | ||
|
|
||
| {% block utilbar %} |
There was a problem hiding this comment.
User-Impersonating ist durchaus ein wichtiges Admin-Feature. Es muss daher klar angezeigt werden, wenn man sich gerade als ein anderer Account ausgibt, um Verwirrung/Fehler zu vermeiden. Dies könnte z.B. sein, dass man es für den falschen Account abbucht.
Der Block utilbar soll erhalten bleiben.
| <div class="row justify-content-center mt-5"> | ||
| <div class="col-md-6 text-center"> | ||
| <h1>Verify Purchase</h1> | ||
| <h3 class="mt-4">Purchase the item "Getränkeliste" at the card-terminal to the left.</h3> |
There was a problem hiding this comment.
Es sollten auf keinen Fall Dinge hardgecoded werden, wie in diesem Fall hier Getränkeliste, da dies dynamische Inhalte aus der Datenbank sind und es mehr als Produkt (mit unterschiedlichen Namen) geben kann, die für das Aufladen verwendet werden.
Zusätzlich ist die Position des Terminals nicht fix. Es passiert regelmäßig, dass dies nicht links davon steht.
Der Bindestrich aus card-termial sollte entfernt werden.
|
Stell bitte deinen Code als Merge-Request auf dem Hauptrepo (https://gitlab.kit.edu/kit/fs-physik/projekte/nanposweb) bereit, damit dies gemerged werden kann. Das Hauptrepo hat seit April bereits Commits bekommen, die hier auf dem Mirror bislang nicht gelandet sind. Würde man dies hier mergen, würde dies nur zu divergierenden Zweigen der beiden Kopien führen. |
2 participants
Hallo Alex,
Grüße von der Hütte. Die Issues #4 #5 #6 #7 sind jetzt crispy clean gelöst.
Viele Grüße
Ben Dover