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

Bug Fix: Devise Checkbox TRUE VALUES not compatible with HTML specs (missing "on" value) #5514

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

itay-grudev
Copy link
Contributor

@itay-grudev itay-grudev commented Aug 29, 2022

See:

This issue causes the remember me functionality not to work correctly, especially when overriding the default styles as check boxes by default in all major browsers send the value on as required by the HTML specifications.

See also:

https://stackoverflow.com/questions/5987075/devise-remember-me-not-working-localhost-issue

@itay-grudev
Copy link
Contributor Author

@carlosantoniodasilva

@itay-grudev
Copy link
Contributor Author

Ping! Any progress on this?

@itay-grudev
Copy link
Contributor Author

@carlosantoniodasilva I think this project needs new contributors or it's of risk of dying. Do you need help?

@nashby
Copy link
Collaborator

nashby commented Nov 7, 2024

@itay-grudev thank you! Could you please add 'ON' as well to this list?

Looks like Rails decided to use FALSE_VALUES (instead of TRUE_VALUES) at some point (https://github.com/rails/rails/blob/c7de35a41bd7255249c9a5750e6a6edf75e61c82/activemodel/lib/active_model/type/boolean.rb#L15) but I think we can keep it as it is in Devise for now.

@itay-grudev
Copy link
Contributor Author

@nashby Done.

@itay-grudev
Copy link
Contributor Author

itay-grudev commented Nov 8, 2024

@nashby I rebased against main. This probably will fix the test failures, as they cannot possibly be caused by my change. Could you approve the workflow?

@nashby
Copy link
Collaborator

nashby commented Nov 8, 2024

@itay-grudev thanks, failing specs are not related to this change so this is fine. Could you please squash your commits?

@itay-grudev
Copy link
Contributor Author

GitHub does that when you click merge and create one merge commit on the entire PR diff.

@itay-grudev
Copy link
Contributor Author

itay-grudev commented Nov 8, 2024

If not - consider enabling it. It keeps your repo clean without using extra human time.

@itay-grudev
Copy link
Contributor Author

image

@nashby
Copy link
Collaborator

nashby commented Nov 8, 2024

@itay-grudev the thing is that I want to have a merge commit but I don't want to have 3 commits on top (it's fine to have multiple commits in one PR but not in this case). So the only way to do it if you squash them, sorry for the trouble

…missing "on" value)

See:
* https://html.spec.whatwg.org/multipage/input.html#checkbox-state-(type%3Dcheckbox)
* https://html.spec.whatwg.org/multipage/input.html#dom-input-value-default-on

This issue causes the remember me functionality not to work correctly, especially when overriding the default styles as check boxes by default in all major browsers send the value `on` as required by the HTML specifications.

See also:

https://stackoverflow.com/questions/5987075/devise-remember-me-not-working-localhost-issue
@itay-grudev
Copy link
Contributor Author

itay-grudev commented Nov 8, 2024

I wish to inform you that because of the non-linear commits applied out of order over the time-span of over 2 years - it took me 10min to understand / resolve the situation and squash them. And I am git skilled. I knew what I was doing.

This is as opposed to the UI-based squash merge that would have taken less than 30s if enabled.

As a true software engineer I despise this bureaucratic inefficiency, hence this complaint.

@itay-grudev
Copy link
Contributor Author

@nashby Done.

@nashby nashby merged commit 0f514f1 into heartcombo:main Nov 8, 2024
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.

3 participants