-
Notifications
You must be signed in to change notification settings - Fork 58
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
Fix error on clone settings without organization selected #997
Conversation
…istView.get() because the form was invalid.
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 it's something else that caused this not to work as a ListView object with a custom query set. But the code seems valid and ListViews without Django ORM models are always a bit tricky.
@ammar92 I added a more detailed description of the flow going wrong, perhaps that clears things up? |
@Darwinkel Good spot, fixed! |
Fix confirmed, ready to merge :) |
@@ -9,7 +9,7 @@ <h2>{% translate "Clone settings" %}</h2> | |||
This includes both the KAT-alogus settings as well as enabled and disabled plugins. | |||
{% endblocktranslate %} | |||
</p> | |||
<form action="{% url "katalogus_clone_settings" organization.code %}" | |||
<form action="{% url "katalogus_settings" organization.code %}" | |||
method="post" | |||
class="horizontal-view help" | |||
novalidate> |
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 novalidate hier nuttig op? Die skipped namelijk ook de required fields daarmee.
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.
Somehow I still think we're misusing the ListView
. It is supposed to always render a list of objects regardless of the form's validity. If our use case or flow is different in some way, then indeed we shouldn't use it as such.
* main: (21 commits) feature(octopoes): fields to finding type model (#921) Add new permissions (#950) Fix RDO workflow (#1023) Various fixes to Fierce boefje (#1001) Feature/add signing provider for raw file (#994) Only sleep when all queues are empty (#952) Upgrade (default) container Dockerfiles from Python 3.8 to 3.11 (#1021) Upgrade FastAPI and dependencies (#467) Make two-factor authentication (2fa) optional (#1002) Upgrade to Django 4.2 (#1004) Upgrade to requests v2.31.0 (#1020) Removed LXD legacy (#1016) Pin typing-extensions to 4.5.0 (#1019) Fix error on clone settings without organization selected (#997) Scheduler tests clean up (#978) Remove job model and generate migrations (#995) update/nuclei path fix, backwards compat (#962) Update debianinstall.rst (#822) Delete `plugin_repository` package (#992) Create boefjes.md (#828) ...
Changes
Remove unused endpoint and set object list field when bypassing the ListView.get() because the form was invalid. Formviews on a POST call the
form_invalid()
which callsself.render_to_response(self.get_context(form=form))
immediately. But at this time the(Base)ListView
has not setself.object_list = self.get_queryset()
since it only does this on aGET
request. Butget_context_data()' _is_ being called on a generic (list?) view that tries to set
context["object_list"] = self.object_list`, which fails because it does not have that property.Issue link
Fixes #996
Proof
Code Checklist
Communication
.env
changes files if required and changed the.env-dist
accordingly.Checklist for code reviewers:
Copy-paste the checklist from the docs/source/templates folder into your comment.
Checklist for QA:
Copy-paste the checklist from the docs/source/templates folder into your comment.