Skip to content
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 wrong id_for_label usage in rendered evaluation edit form #1784

Merged
merged 4 commits into from
Aug 1, 2022

Conversation

richardebeling
Copy link
Member

@richardebeling richardebeling commented Jul 19, 2022

Fixes #1769.

Our old code was broken and just happened to work accidentally before django 4: The default widget for a choice field is a Select widget. With that, the rendering:

<select id="test" name="test">
   <option>1</option>
   <option>2</option>
</select>

wouldn't require any id_for_label to be set for the individual values. However, before django 4, this happened to be set, and our code relied on that with no documented guarantee.

This PR changes the button groups to use RadioSelect widgets, which would be rendered as:

<input type="radio" id="id_1" name="test" value="1"><label for="id_1">1</label>
<input type="radio" id="id_2" name="test" value="2"><label for="id_2">2</label>

i.e. here, every choice gets a seperate ID. This is also guaranteed by the docs:

To get more granular, you can use each radio button’s tag, choice_label and id_for_label attributes

So this time, if anything changes, we should see an entry in the release notes.

Edit: We can probably make a front end test for this one, it should be detectable just from the rendered html

@janno42
Copy link
Member

janno42 commented Jul 21, 2022

Do you want to add a test? (:

Copy link
Member

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems all correct, blocking until test though (as discussed in person)

@niklasmohrin
Copy link
Member

Take this:

const formData = await page.evaluate(() => {
    return Object.fromEntries(new FormData(document.getElementById("evaluation-form") as HTMLFormElement));
});

@richardebeling
Copy link
Member Author

Take this:

const formData = await page.evaluate(() => {
    return Object.fromEntries(new FormData(document.getElementById("evaluation-form") as HTMLFormElement));
});

Nice, thanks. Added that, seems to work now.

I've removed the TomSelectedHtmlSelectElement interface, because I figured it doesn't really help if we add "This has a .tomselect attribute" to the type system, but then go on to do stuff inside there (-> .tomselect.options, access its keys, ...) while only having it declared as any. Or, to say it differently: That only hid the any behind another interface, but didn't make the code type-safe. I think if we want proper typing, we should just use the proper tomselect types.


Codacy complains about magic numbers (in a test, I think they are fine: We do want to click the first button that says "xyz") and any / non-null-assertions (Also fine for me: Its a test, I'm fine if it breaks at runtime, I don't want to write additional boilerplate code just to make that explicit.)

Co-authored-by: Niklas Mohrin <dev@niklasmohrin.de>
@richardebeling richardebeling merged commit 61bb13f into e-valuation:main Aug 1, 2022
@richardebeling richardebeling deleted the evaluation-edit-fix branch August 1, 2022 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Evaluation Edit: Button Groups don't work, submitting fails with "This field is required".
4 participants