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

[IMP] fields: remove support for non-string selections #29039

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@xmo-odoo
Copy link
Collaborator

commented Nov 26, 2018

@robodoo robodoo added the seen 🙂 label Nov 26, 2018

@C3POdoo C3POdoo added the RD label Nov 26, 2018

@rco-odoo rco-odoo force-pushed the odoo-dev:master-deselect branch 5 times, most recently Jan 23, 2019

@robodoo robodoo added the CI 🤖 label Jan 25, 2019

@rco-odoo rco-odoo force-pushed the odoo-dev:master-deselect branch Jan 25, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 25, 2019

@rco-odoo rco-odoo force-pushed the odoo-dev:master-deselect branch to 8ad5887 Jan 26, 2019

@robodoo robodoo removed the CI 🤖 label Jan 26, 2019

@rco-odoo rco-odoo changed the title [WIP] remove support for non-string selections [IMP] fields: remove support for non-string selections Jan 26, 2019

@robodoo robodoo added the CI 🤖 label Jan 26, 2019

@rco-odoo rco-odoo requested a review from KangOl Jan 28, 2019

@KangOl

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

Do web client handle correctly the 0/1 selection fields with the boolean widget?

@rco-odoo

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

@KangOl thanks for pointing that out, I haven't checked it.

@rco-odoo

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

@KangOl I don't understand. IMHO, there is not point using the boolean widget with a selection field. The boolean widgets are aimed a boolean fields only.

We have used selection fields in place of boolean fields in the configuration wizard. The point was to use the radio button widget. This one works as expected.

@pedrobaeza

This comment has been minimized.

Copy link
Collaborator

commented Jan 28, 2019

@gurneyalex you had several reasons for using selection with integers instead of strings, do you?

@gurneyalex

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

@pedrobaeza yes, interfacing with external systems which use enums. There are lots of these in the wild. The support was badly broken with "0" being not usable (displayed as unset, in Odoo), we ended up using stringified integers and adding int(val) and str(val) all over the place to parse / generate the expected values.

See for instance #11015

@rco-odoo

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

@pedrobaeza @gurneyalex the purpose of this task is definitively to simplify things, and remove a feature that is both fragile and error-prone. See #28877 and #28891 for fixing such a terrible programming error.

@rco-odoo

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

@KangOl are you okay with merging this?

@KangOl

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

Yes.

@rco-odoo

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

@robodoo robodoo added the r+ 👌 label Jan 28, 2019

robodoo pushed a commit that referenced this pull request Jan 28, 2019

@robodoo robodoo added merging 👷 and removed merging 👷 labels Jan 28, 2019

@robodoo robodoo added the merged 🎉 label Jan 28, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

Merged, thanks!

@robodoo robodoo closed this Jan 28, 2019

@rco-odoo rco-odoo deleted the odoo-dev:master-deselect branch Jan 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.