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

Checkbox with 'required' parameter attribute #1014

Closed
sysadminstory opened this issue Jan 21, 2019 · 3 comments
Closed

Checkbox with 'required' parameter attribute #1014

sysadminstory opened this issue Jan 21, 2019 · 3 comments
Labels
Bug-Report Confirmed bug report

Comments

@sysadminstory
Copy link
Contributor

Since 288d4de, there is an issue with the "Pepper" bridge Family :

  • DealabsBridge
  • HotUKDealsBridge
  • MydealsBridge

Those two checkbox parameters have the attribute required :
hide_local
hide_expired

Since the change, the Bridge card set the two checkbox as required and if not checked, an HTML5 browser will refuse to submit the form for the bridge.

Is the bridge using 'required ' in a false way for checkbox, or the Bridge Card should not use "required" when checkbox are involved ?

@logmanoriginal logmanoriginal added the Bug-Report Confirmed bug report label Jan 21, 2019
@logmanoriginal
Copy link
Contributor

Good point, thanks for reporting this issue!

We absolutely failed to see this issue in the fix. Now that you mentioned it, all parameters were recognized as required => false in the past, which is why it fails after the fix. Of course, this applies to all bridges that were changed. A fast way to restore the original behavior is to set all required values to false or simply remove them (which has the same effect).

Is the bridge using 'required ' in a false way for checkbox, or the Bridge Card should not use "required" when checkbox are involved ?

Let's just say it is not well documented. Bridge maintainers simply had no chance of knowing better. For the sake of simplicity, however, the second point should be implemented before adding more complexity to bridges.

That being said, we need to be more clear on the meaning of 'required'. Currently there are at least three interpretations depending on the point of view:

  1. The parameter must be "available" (i.e. it cannot be omitted in the query)
  2. The user has to make a choice or insert data (i.e. the input field should never be empty)
  3. The user has to fill in information for all fields that are set to required (for checkboxes that means it must be checked)

Obviously it doesn't make sense to have a checkbox be required in RSS-Bridge because it essentially makes it a true constant. RSS-Bridge also already ensures that all parameters are available at all times, so there is no reason to enforce that. The only reason to require a parameter is therefore to make the user insert data (no empty field).

That means, checkboxes should simply ignore the required flag and report it in debug mode. The same probably applies to lists as well.

@logmanoriginal
Copy link
Contributor

This should be fixed in master. Please try again.

@sysadminstory
Copy link
Contributor Author

That was fast :)

Now it works !

Thanks !

infominer33 pushed a commit to web-work-tools/rss-bridge that referenced this issue Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug-Report Confirmed bug report
Projects
None yet
Development

No branches or pull requests

2 participants