minvws / nl-kat-coordination

Repo nl-kat-coordination for minvws
European Union Public License 1.2
125 stars 56 forks source link

Uploading raw file without required fields gives KeyError #3641

Open madelondohmen opened 2 days ago

madelondohmen commented 2 days ago

Describe the bug When adding a raw file, it is possible to leave the required fields empty and continue, which will result in a KeyError.

To Reproduce Steps to reproduce the behavior:

  1. Go to 'Objects'
  2. Click 'Add'
  3. Click 'Add raw file'
  4. Leave all the fields empty and continue
  5. See error

Expected behavior An error message should be thrown in the form and a user should not be able to continue.

Screenshots Image

Environment:

Request Method: POST
Request URL: http://localhost:8000/en/madelon123/objects/upload/raw/

Django Version: 5.0.8
Python Version: 3.11.10
Installed Applications:
['whitenoise.runserver_nostatic',
 'django.contrib.admin',
 'django.contrib.auth',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.messages',
 'django.contrib.humanize',
 'django.forms',
 'django_components',
 'django_components.safer_staticfiles',
 'django_otp',
 'django_otp.plugins.otp_static',
 'django_otp.plugins.otp_totp',
 'two_factor',
 'account',
 'tools',
 'fmea',
 'rocky',
 'crisis_room',
 'onboarding',
 'katalogus',
 'django_password_validators',
 'django_password_validators.password_history',
 'rest_framework',
 'tagulous',
 'compressor',
 'reports',
 'knox',
 'csp']
Installed Middleware:
['django.middleware.security.SecurityMiddleware',
 'whitenoise.middleware.WhiteNoiseMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'django.middleware.locale.LocaleMiddleware',
 'django.middleware.common.CommonMiddleware',
 'django.middleware.csrf.CsrfViewMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'rocky.middleware.auth_token.AuthTokenMiddleware',
 'django_structlog.middlewares.RequestMiddleware',
 'django_otp.middleware.OTPMiddleware',
 'rocky.middleware.auth_required.AuthRequiredMiddleware',
 'django.contrib.messages.middleware.MessageMiddleware',
 'django.middleware.clickjacking.XFrameOptionsMiddleware',
 'rocky.middleware.onboarding.OnboardingMiddleware',
 'csp.middleware.CSPMiddleware']

Traceback (most recent call last):
  File "/usr/local/lib/python3.11/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django/views/generic/base.py", line 104, in view
    return self.dispatch(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django/contrib/auth/mixins.py", line 109, in dispatch
    return super().dispatch(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django/views/generic/base.py", line 143, in dispatch
    return handler(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django/views/generic/edit.py", line 150, in post
    if form.is_valid():
       ^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django/forms/forms.py", line 197, in is_valid
    return self.is_bound and not self.errors
                                 ^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django/forms/forms.py", line 192, in errors
    self.full_clean()
    ^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django/forms/forms.py", line 328, in full_clean
    self._clean_form()
    ^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django/forms/forms.py", line 349, in _clean_form
    cleaned_data = self.clean()
                   ^^^^^^^^^^^^
  File "/app/rocky/tools/forms/upload_raw.py", line 67, in clean
    ooi_id = self.cleaned_data["ooi_id"]
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^

Exception Type: KeyError at /en/madelon123/objects/upload/raw/
Exception Value: 'ooi_id'

OpenKAT version main

underdarknl commented 2 days ago

Is there a good reason why the form has novalidate? https://github.com/minvws/nl-kat-coordination/blob/main/rocky/rocky/templates/upload_raw.html#L20C23-L20C33 That would at least catch the frontend issue. the backend issue needs seperate logic / error handling.

I see we do novalidate on many forms, but I think its bad form to not use the frontend validation were possible.

noamblitz commented 2 days ago

@HeleenSG Do you know this?

underdarknl commented 2 days ago

I think we have novalidate because the client-side errors are not a11y compatible. This is a stupid problem caused by screenreaders / browsers which don't properly attribute the explanation/error messages to the input fields, and or don't show the error long enough (which should be a browser setting for those who experience problems reading these errors).

We could opt for client-side validation using javascript and or custom Error handling on the available form controls. The Javascript could then inject the 'correct' and a11y friendly error messages. We would still also need to do backend validation, and as such would need to write the same error handling logic and rules twice.