Skip to content

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

Merged
nashby merged 1 commit into
heartcombo:mainfrom
itay-grudev:master
Nov 8, 2024
Merged

Bug Fix: Devise Checkbox TRUE VALUES not compatible with HTML specs (missing "on" value)#5514
nashby merged 1 commit into
heartcombo:mainfrom
itay-grudev:master

Conversation

@itay-grudev

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

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

@carlosantoniodasilva

@itay-grudev

Copy link
Copy Markdown
Contributor Author

Ping! Any progress on this?

@itay-grudev

Copy link
Copy Markdown
Contributor Author

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

@nashby

nashby commented Nov 7, 2024

Copy link
Copy Markdown
Collaborator

@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
Copy Markdown
Contributor Author

@nashby Done.

@itay-grudev

itay-grudev commented Nov 8, 2024

Copy link
Copy Markdown
Contributor Author

@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

nashby commented Nov 8, 2024

Copy link
Copy Markdown
Collaborator

@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
Copy Markdown
Contributor Author

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

@itay-grudev

itay-grudev commented Nov 8, 2024

Copy link
Copy Markdown
Contributor Author

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

@itay-grudev

Copy link
Copy Markdown
Contributor Author

image

@nashby

nashby commented Nov 8, 2024

Copy link
Copy Markdown
Collaborator

@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

itay-grudev commented Nov 8, 2024

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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