Fixed #1682 -- alert user when using file field without proper encoding

[bkdekoning]

Description

Added functionality where the toolbar inspects the loaded HTML content for a form that includes a file input but does not have the encoding type set to multipart/form-data, and warns the user if so. This is implemented in a separate alerts panel, as discussed. The alerts panel is placed at the top of the toolbar, to ensure that it receives appropriate attention when issues arise. My one concern with this implementation is that panel may cause people to expect that the toolbar checks for a wide range of issues, whereas it currently just checks for a single one.

What I did specifically is subclass Python's HTML parser and added a function called check_invalid_file_form_configuration in panels/templates/panel.py. The function checks all forms for a file input type and, if it exists, checks if the form has encoding type multipart/form-data. If it does not, the error message is shown in the alerts panel by passing it to record_stats. It also amends the nav_subtitle property of the panel to notify the user that there is an issue. Added four tests and checked that the test suite passes for all compatible versions of Django and Python through Tox.

Fixes #1682

Checklist:

• I have added the relevant tests for this change.
• I have added an item to the Pending section of docs/changes.rst.

[tim-schilling]

By the way, you don't have to recreate the PRs. I think for 95% of cases, we're happy with re-using the same PR for a change.

[tim-schilling]

I wonder if it'd be better to set this in enable_instrumentation instead

[bkdekoning]

I trust your call on this, but have to admit that I do not fully understand the enable_instrumentation method. What would the advantage be of setting it there? Looking at the other panels, I commonly see lists being initialized in __init__.

[tim-schilling]

I've got a few opinionated takes on the approach. Let me know if I'm overreaching with any of them. I really like what you have here. I think changing the nav_subtitle is a great first version for notifying the user of a problem! Nice solution there.

Well done with the tests, documentation and arrangement of the code too!

[bkdekoning]

While working on the above, I realized that I am implicitly assuming that the form is not nested. While forms should not be nested, I tested such a construction with Chrome, Firefox, and Edge, and they handle it by simply ignoring the "child" form.

As such, we might end up in a situation where we are not alerting the developer to an issue that may exist for some users and vice versa -- when the encoding is properly set in the "child" form but not the "parent" form. Is this something we want to issue an additional alert for or would that be overly cautious?

To illustrate; the below code would not cause an alert, while it would cause an encoding issue on the above mentioned browsers:

<form>
  <form enctype="multipart/form-data">
    <input type ="file">
  </form>
</form>

[tim-schilling]

As a heads up, I probably won't get to this towards the end of June. Things are ramping up elsewhere and the toolbar is taking a back seat. Thank you for making the changes so far!

[bkdekoning]

Alright, thanks for the heads-up. For future reference, I think the remaining items to settle on are:

• Whether to use enable_instrumentation or __init__ to initialize alerts
• Whether we should handle direct form references in file inputs
• Whether we should handle nested forms

Good luck with your other activities!

[tim-schilling]

Whether we should handle nested forms

Nested forms are against the HTML spec, so I think what we have is fine. Most people aren't relying on this to be perfect, so let's get something "good" out.

Whether we should handle direct form references in file inputs

While I'd like to see us do that, I don't think we have to worry too much about it. It would take a fair amount of work to get a Django form renderer to handle that. Let's lean into what's the common flow for Django devs.

[tim-schilling]

Whether to use enable_instrumentation or init to initialize alerts

Looking at this again, it's not a big deal. Let's go with what you have.

[tim-schilling]

Code looks great. I'm going to pull it down and check the UI to see what it looks like. If that's good, I'll merge it.

[bkdekoning]

Whether we should handle direct form references in file inputs

While I'd like to see us do that, I don't think we have to worry too much about it. It would take a fair amount of work to get a Django form renderer to handle that. Let's lean into what's the common flow for Django devs.

Just to be sure: I did implement this. When the parser sees a file input field that directly references a form, it adds it to the referenced_file_inputs list. The check_invalid_file_form_configuration function then fetches the referenced form's encoding type and checks if it is set to multipart/form-data.

[tim-schilling]

Oh, perfect!

Take a look at the latest commits and let me know if you have any issues with them. The biggest changes were moving the panel and rewording the messages a bit.

[bkdekoning]

All looks good to me!

[tim-schilling]

@matthiask I'd like to get your approval on the new panel before we merge this.

[matthiask]

Yes sure, let's do it!

[tim-schilling]

Thank you @bkdekoning! I've wanted this for so long!

[bkdekoning]

Thank you, @tim-schilling and @matthiask! Very excited to have made my first contribution. I really appreciated the thorough review and guidance.

[matthiask]

I haven't dont much here, the thank you belongs to you and Tim!