-
Notifications
You must be signed in to change notification settings - Fork 4
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
Updating dependencies #305
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.
Looks good apart from two things.
src/feedback/admin.py
Outdated
@@ -52,7 +52,7 @@ def assign_fachgebiet_action(self, request, queryset): | |||
|
|||
if not form: | |||
form = self.FachgebietZuweisenForm(initial={ | |||
'_selected_action': request.POST.getlist(admin.ACTION_CHECKBOX_NAME) | |||
'_selected_action': queryset.values_list('id', flat=True) |
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.
Nitpick: extra whitespace before queryset
, also in other lines in this PR
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.
Extra spaces are removed
src/feedback/views/veranstalter.py
Outdated
cleaned_data = self.get_cleaned_basisdaten() | ||
cleaned_data = {} | ||
if perform_evalution(self) : | ||
cleaned_data = self.get_cleaned_basisdaten() |
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.
Is the if
necessary? I think it is a good thing when lecturers get notified of a successful submission, even if they don't want to evaluate.
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 if
was added because of django-formtools update. get_cleaned_basisdaten
is using get_cleaned_data_for_step
which uses get_form
which uses get_form_list
.
Starting from django-fromtools 2.4, "WizardView is using get_form_list() instead of directly accessing form_list" as in chanegelog. get_form_list()
is using functions in condition_dict
to determine whether to add or not to add steps.
As perform_evalution
is assigned to basisdaten
in condition_dict
, it returns False
, because we don't want to evaluate, which also causes basisdaten
not to get added to steps in get_form_list
.
In get_form
there is self.get_form_list()[step]
, which throws Error, as basisdaten
is not in output of get_form_list
.
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.
Ok, thanks for the explanation! Would you mind adding a short comment in the source code on why this is necessary?
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 added the comment, I think this is as short as it gets, without leaving anything important out
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.
Looks good to me, thanks!
Updated dependencies to newer versions. Following updates have been made :
django 3.0.14 -> 5.1
django-debug-toolbar 3.2.4 -> 4.4.6
docutils 0.20.1 -> 0.21.2
freezegun 1.2.2 -> 1.5.1
django-formtools 2.3 -> 2.5.1