search

plone / plone.volto

Plone add-on to configure Plone with Volto, the new default frontend for Plone 6.
https://6.demo.plone.org/
3 stars 3 forks source link

Fix unchecked checkboxes in migrate_to_volto #125

Closed pbauer closed 12 months ago

pbauer commented 12 months ago

Fix #124

mister-roboto commented 12 months ago

@pbauer thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

pbauer commented 12 months ago

@jenkins-plone-org please run jobs

davisagli commented 12 months ago

@pbauer Wouldn't it make more sense to change the default values on the server side at https://github.com/plone/plone.volto/blob/7c0ba3336907288895c52028b7a87ffea81d06cc/src/plone/volto/browser/migrate_to_volto.py#L40-L42 ?

pbauer commented 12 months ago

@pbauer Wouldn't it make more sense to change the default values on the server side at

https://github.com/plone/plone.volto/blob/7c0ba3336907288895c52028b7a87ffea81d06cc/src/plone/volto/browser/migrate_to_volto.py#L40-L42 ?

Done. I would still keep the additional checkboxes to make them fully actually functional when kept unchecked. I was unaware that If a checkbox is unchecked when its form is submitted, neither the name nor the value is submitted to the server. It was actually the first time that hit me :)

davisagli commented 12 months ago

I was unaware that If a checkbox is unchecked when its form is submitted, neither the name nor the value is submitted to the server. It was actually the first time that hit me :)

Right, now I remember this. But it is still okay for no value to be sent, as long as the server side does not default to True in that case.

Mm, looks like the tests fail now.

davisagli commented 12 months ago

@pbauer The change to the tests made me realize that we changed the default state of the checkboxes, which was not the original intent here. Sorry that I was confused and asked for additional work. I think we should reset the branch to this commit (before my first review) and merge it: https://github.com/plone/plone.volto/pull/125/commits/c93f85ffe5b2244cc02965b13e754ee62fb2676b

pbauer commented 12 months ago

@jenkins-plone-org please run jobs