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

Match default values when querying views #900

Merged
merged 2 commits into from Mar 5, 2024

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Mar 4, 2024

When gathering rows that are visible in views we need to take rows into
account that have no corresponding entry in the columns_TYPE tables. For
those we can match the filter in PHP and include row ids from a left
join where there is no entry.

Taking an example of a yes/no selection:

If the default value of a column doesn't match we end up using the same
query as before for collecting row ids

...
	`id` IN (SELECT `row_id` FROM `oc_tables_row_cells_selection` WHERE (`column_id` = 7) AND (`value` LIKE '"false"'))
...

If the default value matches, we need to include those that do not have
an entry

...
	`id` IN (SELECT `row_id` FROM `oc_tables_row_cells_selection` WHERE (`column_id` = 7) AND (`value` LIKE '"false"'))
OR
	`id` IN (select sl.id from oc_tables_row_sleeves sl LEFT JOIN oc_tables_row_cells_selection se ON sl.id = se.row_id AND se.column_id = 7 WHERE se.id IS NULL)
...

Steps to reproduce

  • Create a table with one text field
  • Add a row
  • Add a new column to it with some default value
  • Create a filter that filters for the default value

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

reasonable change, one question.

->leftJoin('sl', 'tables_row_cells_' . $column->getType(), 'v', $qb->expr()->andX(
$qb->expr()->eq('sl.id', 'v.row_id'),
$qb->expr()->eq('v.column_id', $qb->createNamedParameter($column->getId(), IQueryBuilder::PARAM_INT)),
$qb->expr()->eq('sl.table_id', $qb->createNamedParameter($column->getTableId(), IQueryBuilder::PARAM_INT))
Copy link
Member

Choose a reason for hiding this comment

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

Being a left join and since it is not establishing a condition between sl and v, this is rather to be understood as part of the general where clause, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good catch for the sl.table_id, the v.column_id we need to also include NULL values on the join where the row_id does not match

Copy link
Member

Choose a reason for hiding this comment

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

yes, my comment is only about the sl.table_id, sorry for being unprecise.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted

When gathering rows that are visible in views we need to take rows into
account that have no corresponding entry in the columns_TYPE tables. For
those we can match the filter in PHP and include row ids from a left
join where there is no entry.

Taking an example of a yes/no selection:

If the default value of a column doesn't match we end up using the same
query as before for collecting row ids

...
	`id` IN (SELECT `row_id` FROM `oc_tables_row_cells_selection` WHERE (`column_id` = 7) AND (`value` LIKE '"false"'))
...

If the default value matches, we need to include those that do not have
an entry

...
	`id` IN (SELECT `row_id` FROM `oc_tables_row_cells_selection` WHERE (`column_id` = 7) AND (`value` LIKE '"false"'))
OR
	`id` IN (select sl.id from oc_tables_row_sleeves sl LEFT JOIN oc_tables_row_cells_selection se ON sl.id = se.row_id AND se.column_id = 7 WHERE se.id IS NULL)
...

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl
Copy link
Member Author

So I would love to have comprehensive tests for this but lack time at the moment. Also still a bit unsure how to cover that. Behat tests might just be to verbose for test definition, so I might go with a phpunit tests that still writes to the DB to cover the different value conversions more easily in a dataProvider.

Nevertheless I'd follow up this if you're fine with it @blizzz

@juliushaertl juliushaertl added bug Something isn't working 3. to review Waiting for reviews labels Mar 5, 2024
@blizzz
Copy link
Member

blizzz commented Mar 5, 2024

So I would love to have comprehensive tests for this but lack time at the moment. Also still a bit unsure how to cover that. Behat tests might just be to verbose for test definition, so I might go with a phpunit tests that still writes to the DB to cover the different value conversions more easily in a dataProvider.

Not a fan of unit tests for DB logic tbh, but would be a way to tackle it.

Nevertheless I'd follow up this if you're fine with it @blizzz

Welcome to the club 😐 perhaps add an issue for tracking. You have my blessing ;)

@juliushaertl juliushaertl merged commit 33536d1 into main Mar 5, 2024
50 checks passed
@juliushaertl juliushaertl deleted the fix/match-default-values branch March 5, 2024 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants