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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

minor fix: bool should not be empty when there is not null constraint #1711

Merged
merged 5 commits into from Sep 29, 2023

Conversation

azmy60
Copy link
Contributor

@azmy60 azmy60 commented Sep 28, 2023

Prior to this, users were allowed to input empty value (empty string) to non-nullable boolean. Steps to reproduce:

  1. In sqlite, create a table containing nullable bool and non nullable bool
CREATE TABLE
  `table_with_bool` (
    `id` integer not null primary key autoincrement,
    `flag1` bool not null,
    `flag2` bool null
  );
insert into `table_with_bool` (`flag1`, `flag2`, `id`) values ('0', NULL, '1');
  1. Open the table, click non nullable bool cell, and then click away to cancel the edit.

  2. Now you can see the cell is (EMPTY) and can be applied (Does not give an error!).

(EMPTY) here is not a null value, which is why there is no constraint error. Also it's sqlite... yknow...

This PR will:

  • fix that
  • add nullable option to nullable bool column

BEFORE (unable to assign null and get back to original value 馃槨):
Animation4

AFTER (:innocent:):
Animation4

Prior to this, users are allowed to input empty value (string) to
nullable boolean.

This commit will:
- fix that
- add nullable option to nullable bool column
if (/^(bool|boolean)$/i.test(column.dataType)) {
// BigInt to match the value from backend
const values = [
{ label: '0', value: BigInt(0) },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't labels here be true and false? For non-Sqlite boolean columns especially.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, for non-sqlite, shouldn't the values be 'true' and 'false' also?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry I missed the dropdown will show false/true section below.

Question though -- do 0 and 1 map to boolean data types in all the other databases? Maybe that needs a test to confirm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I thought boolean is only in sqlite. 0 and 1 do not map to boolean types in postgres. With that I think it's simpler to use false / true like before. It's easy to switch. Give me a second.

Copy link
Contributor Author

@azmy60 azmy60 Sep 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it needs some generic mapping like mysqlDialect.booleanValues: { "true": BigInt(1), "false": "BigInt(0)" }, and then do this to each dialect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing that changes here is in sqlite, we're showing 0 and 1 in dropdown, instead of false and true. That's probably the simplest solution we could have. Is that ok?

The problem with sqlite is that boolean is represented as 0 and 1, and when using false and true as the dropdown options, it doesn't match up, which then shows that (empty) value.

We still can use the false and true as values for sqlite though if you want. I just need some time to wire up the good solution to that.

Copy link
Collaborator

@rathboma rathboma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@rathboma rathboma merged commit 5dc95ad into master Sep 29, 2023
4 checks passed
@rathboma rathboma deleted the fix/tabulator-bool-option branch September 29, 2023 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants