Princeton-LSI-ResearchComputing / tracebase

Mouse Metabolite Tracing Data Repository for the Rabinowitz Lab
MIT License
4 stars 1 forks source link

Implement browser-supported form validation in the advanced search #132

Open hepcat72 opened 3 years ago

hepcat72 commented 3 years ago

FEATURE DESCRIPTION

Inspiration

Javascript added to make dynamic hierarchical forms on the advanced search page ~inadvertently circumvents Django's~ Does not take advantage of browser-builtin automatic form validation that makes a nice tooltip. ~It's described in this stack post.~ Manually implemented form validation was implemented as a quick fix.

Description

Remove the quick fix code referenced here when implemented:

https://github.com/Princeton-LSI-ResearchComputing/tracebase/pull/127#discussion_r662424813

Steps to reproduce

  1. Run the server and go to http://127.0.0.1:8000/DataRepo/search_peakgroups/
  2. Leave a field empty and click search

Current behavior

The form submits and you see the quick fix form validation error, such as the red message here:

custom_validation

Desired behavior

Before the dynamic form was changed to manipulate the forms to work with Django's formset_factory, a nice tooltip was presented and the form would not submit:

form_validation_tooltip_works_in_f9c2cac151f9909380022cea8b7a40a5f0e72a4e

Suggested Change

~As suggested in the comments in the stack issue, use onsubmit instead of onclick to manipulate the forms. Might need to also use onclick to change the form element IDs.~

See the comments below, but to briefly describe one way to implement this:

  1. Dynamically add/remove a required attribute to non-hidden form elements of the currently selected output format when the fmt select list changes.
  2. Dynamically update the user-facing val form element based on the currently selected output format and the selected fld value (string/number versus enumeration).

Current work is in branch django_form_validation. See the commit log for where I left off. - Rob

Comment

This is based on this review issue:

NB. This breaks Django's automatic form validation

_Originally posted by @hepcat72 in https://github.com/Princeton-LSI-ResearchComputing/tracebase/pull/127#discussion_r662390345_

https://github.com/Princeton-LSI-ResearchComputing/tracebase/pull/127#discussion_r662424813


ISSUE OWNER SECTION

Assumptions

Requirements

Limitations

Affected/Changed Components

DESIGN

GUI Change description

Describe changes the user will see.

Code Change Description (Pseudocode optional)

Describe code changes planned for the fix.

Tests

A test should be planned for each requirement (above), where possible.

hepcat72 commented 3 years ago

During my work on #141, I realized that the tooltip is likely generated via the CSS. I'm not entirely sure what triggers it yet or how the submission is prevented - that might be different - but suffice it to say that the form the display takes is via CSS. The solution to this may or may not also involve CSS in addition to javascript changes.

lparsons commented 2 years ago

This appears to be fixed, can we close this?

hepcat72 commented 2 years ago

This has not been fixed. This issue has only been mentioned in 2 PRs, but not resolved.

hepcat72 commented 3 months ago

I learned a good bit about this yesterday. The tooltip "Fill out this field" over the form field (like in the screenshot), actually has nothing to do with Django. It's a feature of each individual browser in how if treats form fields rendered in html5. The wording of the tooltip can actually be different from browser to browser. The thing which makes it appear is the click of the submit button and the input element attribute "required". The browser, upon form submission (via click - [this is important]), checks for values in all input fields containing the required attribute (no value, BTW - true is assumed). If any required fields do not have values, the browser prevents form submission and renders the tooltip associated with the required fields that do not have values.

Interestingly, even though Django is rendering the form's input elements (albeit outside of the form tags (which I will explain below^)), and those elements are defined to be required in the form class, is not adding the required attributes, so submission always proceeds.

^ The form elements are rendered outside of the form in a hiudden div tag as a part of the dynamic form replication strategy. Javascript uses that template to add search forms (in a hierarchical fashion to add "and" and "or" groups). And even in the "active"/rendered forms, there are hidden form elements (because it stores all of the searches - one for each format, so that users can switch back and forth between the formats without having to re-enter their search terms).

So the solution cannot simply be to add the required attributes to each form (because you only want to apply them to the active format search forms.

I'm not entirely sure this is all worth it - to make the browser-supplied functionality work. It would involve a lot of code. I think it might be better/easier to instead, allow the form to submit (as it does), and instead of render the red messages, figure out how to present the currently red messages in a nicer way.

hepcat72 commented 3 months ago

Going to change the label from bug to enhancement and remove this issue from the release 3.0 milestone.

From what I learned (described in the above comment), the hierarchical formsets didn't "break" django form validation - it's not even a django feature - it's a browser feature. Rendering the form templates outside of a form apparently doesn't include the required attributes defined in the form class, which means that when those inputs are cloned, there are no required attributes copied - nor would we want them copied, because determining which inputs are required, given the various hidden forms, is something that would have to be determined in the javascript, thus it's an "enhancement".

I got a decent ways into implementing that feature in a branch named django_form_validation, but given Michael doesn't think it's level of effort isn't worth it for the release 3.0 milestone, I'm going to go ahead and also remove it from the milestone. I saved my work on that branch and commented where I left off in the commit log if/when we want to revisit.