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

Ensure all hidden form fields in test overview filter #5464

Merged
merged 1 commit into from Feb 8, 2024

Conversation

perlpunk
Copy link
Contributor

@perlpunk perlpunk commented Feb 8, 2024

Issue: https://progress.opensuse.org/issues/154537

The code was skipping elements that already existed in the form, and this resulted in elements with multiple values to only keep one value. Now the elements to be added are collected and only added to the form after the loop.

Bug was introduced in 2f6b7e0

Issue: https://progress.opensuse.org/issues/154537

The code was skipping elements that already existed in the form, and this
resulted in elements with multiple values to only keep one value.
Now the elements to be added are collected and only added to the form
after the loop.

Bug was introduced in 2f6b7e0
Copy link

codecov bot commented Feb 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (199e451) 98.38% compared to head (1471ceb) 98.38%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5464   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files         389      389           
  Lines       37829    37832    +3     
=======================================
+ Hits        37217    37220    +3     
  Misses        612      612           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +55 to +57
for (var i = 0; i < hiddenInputs.length; i++) {
$('#filter-form').append(hiddenInputs[i]);
}
Copy link
Contributor

@Martchus Martchus Feb 8, 2024

Choose a reason for hiding this comment

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

I would avoid the repeated calls to $('#filter-form') in the loop and stick to standard JavaScript (as we probably shouldn't introduce more jQuery code at this point):

Suggested change
for (var i = 0; i < hiddenInputs.length; i++) {
$('#filter-form').append(hiddenInputs[i]);
}
const filterForm = document.getElementById('filter-form');
hiddenInputs.forEach(i => filterForm.appendChild(i));

Probably the code that creates the input elements should also just use document.createElement('input') and normal DOM APIs.

It is not obvious at all why your change makes a difference but it is because of what the code passed via paramHandler does. That means you could probably also just change that code and make it skip those elements, e.g. replace if (formElement) { with if (formElement && !formElement.hidden) {. This would avoid an extra array and loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried now:

if (formElement && !formElement.hidden)

but then it is always skipping the last distri mentioned in the query parameters...
I need to debug this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So apparently in the first call of the paramHandler form[key] is undefined. In the second it''s returning HTMLInputElement (hidden). After that it returns a RadioNodeList. Possibly because there are now two elements already. Not sure yet how I should handle that.

Copy link
Contributor Author

@perlpunk perlpunk Feb 8, 2024

Choose a reason for hiding this comment

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

Also I would like to add that I didn't introduce any new jquery code. I just moved it from one loop to the other.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, then your approach might be simpler after all. And it is true that the code was just moved.

@mergify mergify bot merged commit 46cfab2 into os-autoinst:master Feb 8, 2024
41 checks passed
@perlpunk perlpunk deleted the group-overview-distri branch February 19, 2024 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants