-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Delete runs #537
Delete runs #537
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.
Richtig cool, dass du dich an so eine Aufgabe ran getraut hast! 🥳
Hab auf jeden Fall einen Bug gefunden, der verhindert, dass Runs gelöscht werden, die einen Namen haben, der anders als "test" ist ;)
Persönlich fände ich es cooler, wenn delete nicht gleich viel space bekommt wie create und continue (da es doch deutlich seltener passieren soll) - aber über das Design können wir vielleicht generell noch wann anders mal sprechen
Ansonsten fände ich es noch sinnvoll eine Art check einzubauen "are you really sure you want to delete run [run_name]" oder so - aber für die Funktionalität löschen schon mal sinnvoll so :)
@@ -18,7 +18,8 @@ | |||
from django.shortcuts import render | |||
from django.urls import reverse | |||
|
|||
from protzilla.run import Run, get_available_run_names | |||
from protzilla.run import Run, get_available_run_names | |||
from protzilla.run_v2 import delete_run_folder |
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.
zur Klasse run vs. run_v2 sollten wir mal das alte BP fragen, was warum wo verwendet wird und wo dann unsere Änderungen am ehesten rein gehören (ich hätte aber auch am ehesten in run_v2 geschrieben)
|
||
delete_run_folder(run_name) | ||
|
||
return HttpResponseRedirect(reverse("runs:index")) |
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.
voll schön geschrieben und an restliche File angepasst 💯
{% endfor %} | ||
</div> | ||
<br> | ||
{% endif %} |
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.
warum ist dieser messages Teil gelöscht?
@sarahvgls Ja fand ich auch, ich bin gerade dabei. Kurzbeschreibung ist im Notion :) |
Falls du noch auf der Suche nach einem schönen Weg bist, ist das hier vielleicht einen Blick wert: |
Sehr schön, dass du dich diesem Thema angenommen hast. Meine Ideen hierzu: Tut mir leid für diese Flut an Text und ich hoffe, das entmutigt dich nicht weiter an diesem tollen Projekt zu arbeiten! Wenn du den Fehler gerne einmal live sehen möchtest, weil es dir vielleicht nicht gelingt, diesen zu reproduzieren, stehe ich dir immer gerne als Versuchskaninchen zu Verfügung. |
Vielleicht ist es möglich die beiden Workflow Auswahlfelder zu kombinieren und nu zwei verschiedene Buttons für die unterschiedlichen Butten drunter zu setzen. |
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.
Danke für die Änderungen!! :)
Description
fixes #
Added UI and functionality to delete runs
Changes
index.html got UI
views.py has new delete_ function
run_v2.py has new delete_run_folder function
Testing
PR checklist
Development
Mergeability
black
Code review