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.

Sign up for GitHub

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

Jump to bottom

fix: Properly create new rows in filtered views #1409

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix: Properly create new rows in filtered views #1409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 13 additions & 53 deletions lib/Db/LegacyRowMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use OCA\Tables\Db\ColumnTypes\SuperColumnQB;
use OCA\Tables\Db\ColumnTypes\TextColumnQB;
use OCA\Tables\Errors\InternalError;
use OCA\Tables\Helper\ColumnsHelper;
use OCA\Tables\Helper\UserHelper;
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\AppFramework\Db\MultipleObjectsReturnedException;
Expand All @@ -30,39 +31,23 @@
/** @template-extends QBMapper<LegacyRow> */
class LegacyRowMapper extends QBMapper {
protected string $table = 'tables_rows';
protected TextColumnQB $textColumnQB;
protected SelectionColumnQB $selectionColumnQB;
protected NumberColumnQB $numberColumnQB;
protected DatetimeColumnQB $datetimeColumnQB;
protected SuperColumnQB $genericColumnQB;
protected ColumnMapper $columnMapper;
protected LoggerInterface $logger;
protected UserHelper $userHelper;
protected Row2Mapper $rowMapper;

protected int $platform;

public function __construct(
IDBConnection $db,
LoggerInterface $logger,
TextColumnQB $textColumnQB,
SelectionColumnQB $selectionColumnQB,
NumberColumnQB $numberColumnQB,
DatetimeColumnQB $datetimeColumnQB,
SuperColumnQB $columnQB,
ColumnMapper $columnMapper,
UserHelper $userHelper,
Row2Mapper $rowMapper) {
private LoggerInterface $logger,
private TextColumnQB $textColumnQB,
private SelectionColumnQB $selectionColumnQB,
private NumberColumnQB $numberColumnQB,
private DatetimeColumnQB $datetimeColumnQB,
private SuperColumnQB $genericColumnQB,
private ColumnMapper $columnMapper,
private ColumnsHelper $columnsHelper,
private UserHelper $userHelper,
private Row2Mapper $rowMapper,
) {
parent::__construct($db, $this->table, LegacyRow::class);
$this->logger = $logger;
$this->textColumnQB = $textColumnQB;
$this->numberColumnQB = $numberColumnQB;
$this->selectionColumnQB = $selectionColumnQB;
$this->datetimeColumnQB = $datetimeColumnQB;
$this->genericColumnQB = $columnQB;
$this->columnMapper = $columnMapper;
$this->userHelper = $userHelper;
$this->rowMapper = $rowMapper;
$this->setPlatform();
}

Expand Down Expand Up @@ -134,31 +119,6 @@ private function getFilterGroups(IQueryBuilder $qb, array $filters): array {
return $filterGroups;
}

private function resolveSearchValue(string $unresolvedSearchValue, string $userId): string {
switch (ltrim($unresolvedSearchValue, '@')) {
case 'me': return $userId;
case 'my-name': return $this->userHelper->getUserDisplayName($userId);
case 'checked': return 'true';
case 'unchecked': return 'false';
case 'stars-0': return '0';
case 'stars-1': return '1';
case 'stars-2': return '2';
case 'stars-3': return '3';
case 'stars-4': return '4';
case 'stars-5': return '5';
case 'datetime-date-today': return date('Y-m-d') ? date('Y-m-d') : '';
case 'datetime-date-start-of-year': return date('Y-01-01') ? date('Y-01-01') : '';
case 'datetime-date-start-of-month': return date('Y-m-01') ? date('Y-m-01') : '';
case 'datetime-date-start-of-week':
$day = date('w');
$result = date('m-d-Y', strtotime('-'.$day.' days'));
return $result ?: '';
case 'datetime-time-now': return date('H:i');
case 'datetime-now': return date('Y-m-d H:i') ? date('Y-m-d H:i') : '';
default: return $unresolvedSearchValue;
}
}

/**
* @param (int|string)[][] $sortArray
*
Expand Down Expand Up @@ -262,7 +222,7 @@ private function addFilterToQuery(IQueryBuilder $qb, View $view, array $neededCo
$filter['columnType'] = $neededColumnTypes[$filter['columnId']];
// TODO move resolution for magic fields to service layer
if(str_starts_with((string) $filter['value'], '@')) {
$filter['value'] = $this->resolveSearchValue((string) $filter['value'], $userId);
$filter['value'] = $this->columnsHelper->resolveSearchValue((string) $filter['value'], $userId);
}
}
}
Expand Down
5 changes: 3 additions & 2 deletions lib/Db/Row2.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

use JsonSerializable;
use OCA\Tables\Model\Public\Row;
use OCA\Tables\Model\RowDataInput;
use OCA\Tables\ResponseDefinitions;

/**
Expand Down Expand Up @@ -73,10 +74,10 @@ public function getData(): ?array {
}

/**
* @param list<array{columnId: int, value: mixed}> $data
* @param RowDataInput|list<array{columnId: int, value: mixed}> $data
* @return void
*/
public function setData(array $data): void {
public function setData(array|RowDataInput $data): void {
foreach ($data as $cell) {
$this->insertOrUpdateCell($cell);
}
Expand Down
31 changes: 1 addition & 30 deletions lib/Db/Row2Mapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ private function replacePlaceholderValues(array &$filters, string $userId): void
foreach ($filters as &$filterGroup) {
foreach ($filterGroup as &$filter) {
if (substr($filter['value'], 0, 1) === '@') {
$filter['value'] = $this->resolveSearchValue($filter['value'], $userId);
$filter['value'] = $this->columnsHelper->resolveSearchValue($filter['value'], $userId);
}
}
}
Expand Down Expand Up @@ -476,35 +476,6 @@ private function getSqlOperator(string $operator, IQueryBuilder $qb, string $col
}
}

/** @noinspection DuplicatedCode */
private function resolveSearchValue(string $placeholder, string $userId): string {
if (substr($placeholder, 0, 14) === '@selection-id-') {
return substr($placeholder, 14);
}
switch (ltrim($placeholder, '@')) {
case 'me': return $userId;
case 'my-name': return $this->userHelper->getUserDisplayName($userId);
case 'checked': return 'true';
case 'unchecked': return 'false';
case 'stars-0': return '0';
case 'stars-1': return '1';
case 'stars-2': return '2';
case 'stars-3': return '3';
case 'stars-4': return '4';
case 'stars-5': return '5';
case 'datetime-date-today': return date('Y-m-d') ? date('Y-m-d') : '';
case 'datetime-date-start-of-year': return date('Y-01-01') ? date('Y-01-01') : '';
case 'datetime-date-start-of-month': return date('Y-m-01') ? date('Y-m-01') : '';
case 'datetime-date-start-of-week':
$day = date('w');
$result = date('m-d-Y', strtotime('-'.$day.' days'));
return $result ?: '';
case 'datetime-time-now': return date('H:i');
case 'datetime-now': return date('Y-m-d H:i') ? date('Y-m-d H:i') : '';
default: return $placeholder;
}
}

/**
* @param IResult $result
* @param RowSleeve[] $sleeves
Expand Down
45 changes: 40 additions & 5 deletions lib/Helper/ColumnsHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,48 @@

namespace OCA\Tables\Helper;

use OCA\Tables\Db\Column;

class ColumnsHelper {

public array $columns = [
'text',
'number',
'datetime',
'selection',
'usergroup'
Column::TYPE_TEXT,
Column::TYPE_NUMBER,
Column::TYPE_DATETIME,
Column::TYPE_SELECTION,
Column::TYPE_USERGROUP,
];

public function __construct(
private UserHelper $userHelper,
) {
}

public function resolveSearchValue(string $placeholder, string $userId): string {
if (str_starts_with($placeholder, '@selection-id-')) {
return substr($placeholder, 14);
}
switch (ltrim($placeholder, '@')) {
case 'me': return $userId;
case 'my-name': return $this->userHelper->getUserDisplayName($userId);
case 'checked': return 'true';
case 'unchecked': return 'false';
case 'stars-0': return '0';
case 'stars-1': return '1';
case 'stars-2': return '2';
case 'stars-3': return '3';
case 'stars-4': return '4';
case 'stars-5': return '5';
case 'datetime-date-today': return date('Y-m-d') ? date('Y-m-d') : '';
case 'datetime-date-start-of-year': return date('Y-01-01') ? date('Y-01-01') : '';
case 'datetime-date-start-of-month': return date('Y-m-01') ? date('Y-m-01') : '';
case 'datetime-date-start-of-week':
$day = date('w');
$result = date('m-d-Y', strtotime('-'.$day.' days'));
return $result ?: '';
case 'datetime-time-now': return date('H:i');
case 'datetime-now': return date('Y-m-d H:i') ? date('Y-m-d H:i') : '';
default: return $placeholder;
}
}
}
8 changes: 8 additions & 0 deletions lib/Model/RowDataInput.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,12 @@ public function valid(): bool {
public function rewind(): void {
reset($this->data);
}

public static function fromArray(array $data): self {
$newRowData = new RowDataInput();
foreach ($data as $value) {
$newRowData->add((int)$value['columnId'], $value['value']);
}
return $newRowData;
}
}
76 changes: 45 additions & 31 deletions lib/Service/RowService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
use OCA\Tables\Event\RowAddedEvent;
use OCA\Tables\Event\RowDeletedEvent;
use OCA\Tables\Event\RowUpdatedEvent;
use OCA\Tables\Helper\ColumnsHelper;
use OCA\Tables\Model\RowDataInput;
use OCA\Tables\ResponseDefinitions;
use OCA\Tables\Service\ColumnTypes\IColumnTypeBusiness;
Expand All @@ -36,30 +37,21 @@
* @psalm-import-type TablesRow from ResponseDefinitions
*/
class RowService extends SuperService {
private ColumnMapper $columnMapper;
private ViewMapper $viewMapper;
private TableMapper $tableMapper;
private Row2Mapper $row2Mapper;
private array $tmpRows = []; // holds already loaded rows as a small cache

protected IEventDispatcher $eventDispatcher;

public function __construct(
PermissionsService $permissionsService,
LoggerInterface $logger,
?string $userId,
ColumnMapper $columnMapper,
ViewMapper $viewMapper,
TableMapper $tableMapper,
Row2Mapper $row2Mapper,
IEventDispatcher $eventDispatcher
PermissionsService $permissionsService,
private ColumnMapper $columnMapper,
private ViewMapper $viewMapper,
private TableMapper $tableMapper,
private Row2Mapper $row2Mapper,
private IEventDispatcher $eventDispatcher,
private ColumnsHelper $columnsHelper,
) {
parent::__construct($logger, $userId, $permissionsService);
$this->columnMapper = $columnMapper;
$this->viewMapper = $viewMapper;
$this->tableMapper = $tableMapper;
$this->row2Mapper = $row2Mapper;
$this->eventDispatcher = $eventDispatcher;

}

/**
Expand Down Expand Up @@ -174,6 +166,8 @@
throw new InternalError(get_class($this) . ' - ' . __FUNCTION__ . ': '.$e->getMessage());
}

$view = null;

if ($viewId) {
try {
$view = $this->viewMapper->find($viewId);
Expand Down Expand Up @@ -215,9 +209,10 @@
throw new InternalError('Cannot create row without table or view in context');
}

$data = $data instanceof RowDataInput ? $data : RowDataInput::fromArray($data);
$data = $this->cleanupData($data, $columns, $tableId, $viewId);
$data = $this->enhanceWithViewDefaults($view, $data);

// perf
$tableId = $tableId ?? $view->getTableId();
$row2 = new Row2();
$row2->setTableId($tableId);
Expand All @@ -227,24 +222,45 @@

$this->eventDispatcher->dispatchTyped(new RowAddedEvent($insertedRow));

return $insertedRow;
return $this->filterRowResult($view, $insertedRow);
} catch (InternalError|Exception $e) {
$this->logger->error($e->getMessage(), ['exception' => $e]);
throw new InternalError(get_class($this) . ' - ' . __FUNCTION__ . ': '.$e->getMessage());
}
}

/**
* @param RowDataInput|list<array{columnId: string|int, value: mixed}> $data
* @param Column[] $columns
* @param int|null $tableId
* @param int|null $viewId
* @return list<array{columnId: int, value: float|int|string|null}>
* When inserting rows into views we try to prefill columns that are not accessible by reasonable defaults
*
* This might not work in all cases, but for single filter rules this is the sanest to ensure the row is actually part of the view
*/
private function enhanceWithViewDefaults(?View $view, RowDataInput $data): RowDataInput {
if ($view === null) {
return $data;
}

$filters = $view->getFilterArray();
if ($filterRule = array_shift($filters)) {
foreach ($filterRule as $filter) {
if (in_array($filter['columnId'], $view->getColumnsArray())) {
continue;
}

if (!in_array($filter['operator'], ['is-equal'])) {
continue;
}

$data->add($filter['columnId'], $this->columnsHelper->resolveSearchValue((string)$filter['value'], $this->userId));
}
}
return $data;
}

/**
* @throws InternalError
*/
private function cleanupData(RowDataInput|array $data, array $columns, ?int $tableId, ?int $viewId): array {
$out = [];
private function cleanupData(RowDataInput $data, array $columns, ?int $tableId, ?int $viewId): RowDataInput {
$out = new RowDataInput();
foreach ($data as $entry) {
$column = $this->getColumnFromColumnsArray((int) $entry['columnId'], $columns);

Expand All @@ -262,18 +278,15 @@
}

// parse given value to respect the column type value format
$out[] = [
'columnId' => (int) $entry['columnId'],
'value' => $this->parseValueByColumnType($column, $entry['value'])
];
$out->add((int) $entry['columnId'], $this->parseValueByColumnType($column, $entry['value']));

Check failure on line 281 in lib/Service/RowService.php

View workflow job for this annotation

GitHub Actions / static-psalm-analysis dev-master

InvalidScalarArgument 

lib/Service/RowService.php:281:40: InvalidScalarArgument: Argument 2 of OCA\Tables\Model\RowDataInput::add expects array<array-key, mixed>|string, but array<array-key, mixed>|float|int|null|string provided (see https://psalm.dev/012)

Check failure on line 281 in lib/Service/RowService.php

View workflow job for this annotation

GitHub Actions / static-psalm-analysis dev-stable28

InvalidScalarArgument 

lib/Service/RowService.php:281:40: InvalidScalarArgument: Argument 2 of OCA\Tables\Model\RowDataInput::add expects array<array-key, mixed>|string, but array<array-key, mixed>|float|int|null|string provided (see https://psalm.dev/012)
}
return $out;
}

/**
* @param Column $column
* @param string|array|int|float|bool|null $value
* @return string|int|float|null
* @return array|string|int|float|null
*/
private function parseValueByColumnType(Column $column, $value = null) {
try {
Expand Down Expand Up @@ -413,6 +426,7 @@
}

$previousData = $item->getData();
$data = RowDataInput::fromArray($data);
$data = $this->cleanupData($data, $columns, $item->getTableId(), $viewId);

foreach ($data as $entry) {
Expand Down
Loading
Loading