jrief / django-formset

The missing widgets and form manipulation library for Django
https://django-formset.fly.dev/
MIT License
318 stars 30 forks source link

Fix assertUniqueName throwing exception when using MultiWidget #74

Closed Actionb closed 1 year ago

Actionb commented 1 year ago

assertUniqueName was not checking whether all names in a field group were unique. Instead, it was checking whether all elements in the field group had the same name as the first element of that group.

This led to problems when using field groups with multiple elements, f.ex. when using MultiWidget widgets.

Fixes #61

Actionb commented 1 year ago

Seems like the tests are busted? Not even the clean/untouched upstream main branch passes the tests for me.

jrief commented 1 year ago

I will try to merge this directly into my branch and run the E2E tests locally.

In 1.0 there is a check to prevent two <django-formset>s interfering with their id="…". In case use auto_id on forms. In the docs I do this all the time, because I have multiple <django-formset>s on each page.

Currently I'm working on #54.

Actionb commented 1 year ago

I tweaked the error message slightly: mentioning name (which is just the name of the first element encountered) probably would have been confusing.

jrief commented 1 year ago

Many thanks for this fix. I integrated it into PR #76

jrief commented 1 year ago

I reverted this fix.

It caused some E2E tests to fail, namely test_radiochoice_field and test_multichoice_field.

Did you run all tests before submitting this PR? Imo, it doesn't seem so.

Actionb commented 1 year ago

No I mentioned that:

Seems like the tests are busted? Not even the clean/untouched upstream main branch passes the tests for me.

I figured there must be something wrong with the tests, considering how minor the change is. And to be honest, since the tests take a long LONG time to run I didn't pursuit this further.


Now, looking at test_radiochoice_field the test fails here:

assert placeholder_field.inner_text() == "This field is required."

The test includes a RadioSelect widget, which renders as multiple checkbox inputs all with the same name. Running the test in headed mode revealed that assertUniqueName throws an error (because all the inputs of the RadioSelect widget have the same name), which breaks the scripts, which stops the "required" text from being attached to the element, which fails the test.

When I rewrote the function according to its name, I didn't think about RadioSelects sharing the same name being something that is totally valid. Absolutely my bad. But now I am confused why assertUniqueName should exist in the first place; in its original state it didn't assert that the name was unique, and in its modified state it doesn't allow RadioSelects (and maybe other widgets). Also, how will #61 get fixed if the function is not changed?

Can you clarify what assertUniqueName is supposed to be doing?

By the way: how do you run the tests in headed mode? I had to modify conftest (basically removing all the stuff related to the Connector) to get into headed mode for debugging.

jrief commented 1 year ago

Can you clarify what assertUniqueName is supposed to be doing?

In case two fields have the same name, catch this before weird stuff occurs. You're right, this check might be over-engineered. Maybe I'll ditch it. Anyway checking for unique ids turned out to be really useful. In the docs I often have two or more formsets on the same page.

By the way: how do you run the tests in headed mode?

Good question. I add a breakpoint into one of the functions to be tested. Then I look at the port where the server runs – it unfortunately changes every time. Then I open the browser and point it to that port on localhost and use one of the routed URLs. There I have the same view on HTML and JS as Playwright would have.

jrief commented 1 year ago

Can you clarify what assertUniqueName is supposed to be doing?

I now remember what that function is intended to do. Multiple checkboxes and radios must use the same name if inside a group. This is the reduce the checked value(s) onto one submitted entity.