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

fix: properly delete rows in views #826

Merged
merged 4 commits into from
Feb 9, 2024
Merged

fix: properly delete rows in views #826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e824226
fix: properly delete rows in views
enjeck Feb 7, 2024
7fbeab8
enh: add e2e tests for view rows
enjeck Feb 7, 2024
3cb2614
enh: remove unnecessary dependency
enjeck Feb 9, 2024
4e9ef5c
chore: fix lint
enjeck Feb 9, 2024
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
73 changes: 56 additions & 17 deletions cypress/e2e/view.cy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
let localUser
const title = 'Test view'

describe('Interact with views', () => {

Expand All @@ -11,11 +12,8 @@ describe('Interact with views', () => {
beforeEach(function() {
cy.login(localUser)
cy.visit('apps/tables')
})

it('Create view and insert rows in the view', () => {
const title = 'View for adding rows'
cy.createTable('View filtering test table')
cy.createTable('View test table')
cy.createTextLineColumn('title', null, null, true)
cy.createSelectionColumn('selection', ['sel1', 'sel2', 'sel3', 'sel4'], null, false)

Expand All @@ -37,41 +35,82 @@ describe('Interact with views', () => {
cy.fillInValueSelection('selection', 'sel2')
cy.get('button').contains('Save').click()

// create view
cy.get('[data-cy="customTableAction"] button').click()
cy.get('.v-popper__popper li button span').contains('Create view').click({ force: true })
cy.get('.modal-container #settings-section_title input').type(title)
cy.get('[data-cy="dataTableCreateViewBtn"]').contains('Create view').click({ force: true })
cy.get('[data-cy="viewSettingsDialogSection"] input').type(title)
})

it('Create view and insert rows in the view', () => {
// ## add filter
cy.get('button').contains('Add new filter group').click()
cy.get('.modal-container .filter-group .v-select.select').eq(0).click()
cy.get('[data-cy="filterFormFilterGroupBtn"]').contains('Add new filter group').click()
cy.get('[data-cy="filterEntryColumn"]').click()
cy.get('ul.vs__dropdown-menu li span[title="selection"]').click()
cy.get('.modal-container .filter-group .v-select.select').eq(1).click()
cy.get('[data-cy="filterEntryOperator"]').click()
cy.get('ul.vs__dropdown-menu li span[title="Is equal"]').click()
cy.get('.modal-container .filter-group .v-select.select').eq(2).click()
cy.get('[data-cy="filterEntrySeachValue"]').click()
cy.get('ul.vs__dropdown-menu li span[title="sel2"]').click()

// ## save view
cy.intercept({ method: 'POST', url: '**/apps/tables/view' }).as('createView')
cy.intercept({ method: 'PUT', url: '**/apps/tables/view/*' }).as('updateView')
cy.contains('button', 'Create View').click()
cy.get('[data-cy="modifyViewBtn"]').contains('Create View').click()
cy.wait('@createView')
cy.wait('@updateView')
cy.contains('.app-navigation-entry-link span', title).should('exist')
cy.get('[data-cy="navigationViewItem"]').contains(title).should('exist')

const expected = ['sevenths row', 'second row']
expected.forEach(item => {
cy.get('.custom-table table tr td div').contains(item).should('be.visible')
cy.get('[data-cy="customTableRow"] td div').contains(item).should('be.visible')
})

// Add new row in the views
cy.get('button').contains('Create row').click()
// Add new row in the view
cy.get('[data-cy="createRowBtn"]').contains('Create row').click()
cy.fillInValueTextLine('title', 'new row')
cy.fillInValueSelection('selection', 'sel2')
cy.get('button').contains('Save').click()
cy.get('[data-cy="createRowSaveButton"]').contains('Save').click()

expected.push('new row')
expected.forEach(item => {
cy.get('.custom-table table tr td div').contains(item).should('be.visible')
cy.get('[data-cy="customTableRow"] td div').contains(item).should('be.visible')
})
})

it('Create view and update rows in the view', () => {
// ## save view
cy.intercept({ method: 'POST', url: '**/apps/tables/view' }).as('createView')
cy.intercept({ method: 'PUT', url: '**/apps/tables/view/*' }).as('updateView')
cy.get('[data-cy="modifyViewBtn"]').contains('Create View').click()
cy.wait('@createView')
cy.wait('@updateView')
cy.get('[data-cy="navigationViewItem"]').contains(title).should('exist')

// Update rows in the view
cy.get('[data-cy="customTableRow"]').contains('first row').parent().parent().find('[data-cy="editRowBtn"]').click()
cy.get('[data-cy="editRowModal"] input').first().clear().type('Changed row')
cy.get('[data-cy="editRowSaveButton"]').contains('Save').click()

cy.get('[data-cy="editRowModal"]').should('not.exist')
cy.get('[data-cy="customTableRow"]').contains('first row').should('not.exist')
cy.get('[data-cy="customTableRow"]').contains('Changed row').should('exist')
})

it('Create view and delete rows in the view', () => {

// ## save view
cy.intercept({ method: 'POST', url: '**/apps/tables/view' }).as('createView')
cy.intercept({ method: 'PUT', url: '**/apps/tables/view/*' }).as('updateView')
cy.get('[data-cy="modifyViewBtn"]').contains('Create View').click()
cy.wait('@createView')
cy.wait('@updateView')
cy.get('[data-cy="navigationViewItem"]').contains(title).should('exist')

// Delete rows in the view
cy.get('[data-cy="customTableRow"]').contains('first row').parent().parent().find('[data-cy="editRowBtn"]').click()
cy.get('[data-cy="editRowModal"] [data-cy="editRowDeleteButton"]').contains('Delete').click()
cy.get('[data-cy="editRowModal"] [data-cy="editRowEditConfirmButton"]').contains('I really want to delete this row!').click()

cy.get('[data-cy="editRowModal"]').should('not.exist')
cy.get('[data-cy="customTableRow"]').contains('first row').should('not.exist')
})
})
8 changes: 2 additions & 6 deletions lib/Service/RowService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

use OCA\Tables\Db\Column;
use OCA\Tables\Db\ColumnMapper;
use OCA\Tables\Db\LegacyRowMapper;
use OCA\Tables\Db\Row2;
use OCA\Tables\Db\Row2Mapper;
use OCA\Tables\Db\Table;
Expand All @@ -28,17 +27,15 @@
* @psalm-import-type TablesRow from ResponseDefinitions
*/
class RowService extends SuperService {
private LegacyRowMapper $mapper;
private ColumnMapper $columnMapper;
private ViewMapper $viewMapper;
private TableMapper $tableMapper;
private Row2Mapper $row2Mapper;
private array $tmpRows = []; // holds already loaded rows as a small cache

public function __construct(PermissionsService $permissionsService, LoggerInterface $logger, ?string $userId,
LegacyRowMapper $mapper, ColumnMapper $columnMapper, ViewMapper $viewMapper, TableMapper $tableMapper, Row2Mapper $row2Mapper) {
ColumnMapper $columnMapper, ViewMapper $viewMapper, TableMapper $tableMapper, Row2Mapper $row2Mapper) {
parent::__construct($logger, $userId, $permissionsService);
$this->mapper = $mapper;
$this->columnMapper = $columnMapper;
$this->viewMapper = $viewMapper;
$this->tableMapper = $tableMapper;
Expand Down Expand Up @@ -438,8 +435,7 @@ public function delete(int $id, ?int $viewId, string $userId): Row2 {
$this->logger->error($e->getMessage(), ['exception' => $e]);
throw new InternalError(get_class($this) . ' - ' . __FUNCTION__ . ': '.$e->getMessage());
}
$rowIds = $this->mapper->getRowIdsOfView($view, $userId);
if(!in_array($id, $rowIds)) {
if(!$this->row2Mapper->isRowInViewPresent($id, $view, $userId)) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be the last usage of $this->mapper so we could drop it from the constructor which makes it one less dependency of the class to resolve

Copy link
Member

Choose a reason for hiding this comment

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

@enjeck Could you check that? Then we can merge this :)

$e = new \Exception('Update row is not allowed.');
$this->logger->error($e->getMessage(), ['exception' => $e]);
throw new InternalError(get_class($this) . ' - ' . __FUNCTION__ . ': '.$e->getMessage());
Expand Down
7 changes: 5 additions & 2 deletions src/modules/main/partials/editViewPartials/filter/FilterEntry.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,17 @@
class="select-field"
:options="columns"
label="title"
:placeholder="t('tables', 'Column')" />
:placeholder="t('tables', 'Column')"
data-cy="filterEntryColumn" />
</div>
<div class="fix-col-2">
<NcSelect
v-if="selectedColumn"
v-model="selectedOperator"
class="select-field"
:options="operators"
:placeholder="t('tables', 'Operator')" />
:placeholder="t('tables', 'Operator')"
data-cy="filterEntryOperator" />
</div>
<div class="fix-col-2">
<NcSelect
Expand All @@ -23,6 +25,7 @@
class="select-field"
:options="magicFields"
:placeholder="getValuePlaceholder"
data-cy="filterEntrySeachValue"
@search="v => term = v" />
</div>
<div class="fix-col-2 actions">
Expand Down
1 change: 1 addition & 0 deletions src/modules/main/partials/editViewPartials/filter/FilterForm.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
:close-after-click="true"
:aria-label="t('tables', 'Add new filter group')"
type="tertiary"
data-cy="filterFormFilterGroupBtn"
@click="addFilterGroup">
{{ t('tables', 'Add new filter group') }}
<template #icon>
Expand Down
2 changes: 1 addition & 1 deletion src/modules/main/partials/editViewPartials/filter/FilterGroup.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<template>
<div class="filter-group">
<div class="filter-group" data-cy="filterGroupSection">
<div class="group-text">
{{ t('tables', '... that meet all of the following conditions') }}
</div>
Expand Down
8 changes: 4 additions & 4 deletions src/modules/main/sections/DataTable.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,13 @@
</NcActionButton>
<NcActionButton v-if="canManageElement(table) "
:close-after-click="true"
@click="$emit('create-view')">
data-cy="dataTableCreateViewBtn" @click="$emit('create-view')">
<template #icon>
<PlaylistPlus :size="20" decorative />
</template>
{{ t('tables', 'Create view') + (isViewSettingSet ? '*' : '') }}
</NcActionButton>
<NcActionButton v-if="canManageTable(table)" :close-after-click="true" @click="$emit('create-column')">
<NcActionButton v-if="canManageTable(table)" :close-after-click="true" data-cy="dataTableCreateColumnBtn" @click="$emit('create-column')">
<template #icon>
<TableColumnPlusAfter :size="20" decorative title="" />
</template>
Expand All @@ -110,15 +110,15 @@
<NcActionCaption :title="t('tables', 'Integration')" />
<NcActionButton v-if="canCreateRowInElement(table)"
:close-after-click="true"
@click="$emit('import', table)">
data-cy="dataTableExportBtn" @click="$emit('import', table)">
<template #icon>
<Import :size="20" decorative title="Import" />
</template>
{{ t('tables', 'Import') }}
</NcActionButton>
<NcActionButton v-if="canReadData(table)" :close-after-click="true"
icon="icon-download"
@click="$emit('download-csv')">
data-cy="dataTableExportBtn" @click="$emit('download-csv')">
{{ t('tables', 'Export as CSV') }}
</NcActionButton>
<NcActionButton v-if="canShareElement(table)"
Expand Down
3 changes: 2 additions & 1 deletion src/modules/modals/EditRow.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,11 @@
<div class="row">
<div class="fix-col-4 space-T" :class="{'justify-between': showDeleteButton, 'end': !showDeleteButton}">
<div v-if="showDeleteButton">
<NcButton v-if="!prepareDeleteRow" :aria-label="t('tables', 'Delete')" type="error" @click="prepareDeleteRow = true">
<NcButton v-if="!prepareDeleteRow" :aria-label="t('tables', 'Delete')" type="error" data-cy="editRowDeleteButton" @click="prepareDeleteRow = true">
{{ t('tables', 'Delete') }}
</NcButton>
<NcButton v-if="prepareDeleteRow"
data-cy="editRowEditConfirmButton"
:wide="true"
:aria-label="t('tables', 'I really want to delete this row!')"
type="error"
Expand Down
6 changes: 3 additions & 3 deletions src/modules/modals/ViewSettings.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
<template>
<NcAppSettingsDialog :open.sync="open" :show-navigation="true" :title="createView ? t('tables', 'Create view') : t('tables', 'Edit view')">
<NcAppSettingsDialog :open.sync="open" :show-navigation="true" data-cy="viewSettingsDialog" :title="createView ? t('tables', 'Create view') : t('tables', 'Edit view')">
<NcAppSettingsSection v-if="columns === null" id="loading" title="">
<div class="icon-loading" />
</NcAppSettingsSection>
<!--title & emoji-->
<NcAppSettingsSection v-if="columns != null" id="title" :title="t('tables', 'Title')">
<NcAppSettingsSection v-if="columns != null" id="title" :title="t('tables', 'Title')" data-cy="viewSettingsDialogSection">
<div class="col-4" style="display: inline-flex;">
<NcEmojiPicker :close-on-select="true" @select="setIcon">
<NcButton type="tertiary"
Expand Down Expand Up @@ -53,7 +53,7 @@
{{ createNewViewText }}
</NcButton>
</div>
<NcButton v-if="!localLoading" type="primary" :aria-label="saveText" @click="saveView()">
<NcButton v-if="!localLoading" type="primary" :aria-label="saveText" data-cy="modifyViewBtn" @click="saveView()">
{{ saveText }}
</NcButton>
</div>
Expand Down
1 change: 1 addition & 0 deletions src/modules/navigation/partials/NavigationViewItem.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
<template>
<NcAppNavigationItem v-if="view"
data-cy="navigationViewItem"
:name="view.title"
:class="{active: activeView && view.id === activeView.id}"
:force-menu="true"
Expand Down
2 changes: 1 addition & 1 deletion src/shared/components/ncTable/partials/TableRow.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
:value="getCellValue(col)" />
</td>
<td v-if="config.showActions" :class="{sticky: config.showActions}">
<NcButton v-if="config.canEditRows || config.canDeleteRows" type="primary" :aria-label="t('tables', 'Edit row')" @click="$emit('edit-row', row.id)">
<NcButton v-if="config.canEditRows || config.canDeleteRows" type="primary" :aria-label="t('tables', 'Edit row')" data-cy="editRowBtn" @click="$emit('edit-row', row.id)">
<template #icon>
<Pencil :size="20" />
</template>
Expand Down
1 change: 1 addition & 0 deletions src/shared/components/ncTable/sections/CustomTable.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
<tbody>
<TableRow v-for="(row, index) in getSearchedAndFilteredAndSortedRows"
:key="index"
data-cy="customTableRow"
:row="row"
:columns="columns"
:selected="isRowSelected(row.id)"
Expand Down
2 changes: 2 additions & 0 deletions src/shared/components/ncTable/sections/Options.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
:aria-label="t('tables', 'Create row')"
:close-after-click="true"
type="tertiary"
data-cy="createRowBtn"
@click="$emit('create-row')">
{{ t('tables', 'Create row') }}
<template #icon>
Expand All @@ -17,6 +18,7 @@
:close-after-click="true"
:aria-label="t('tables', 'Create Row')"
type="tertiary"
data-cy="createRowBtn"
@click="$emit('create-row')">
<template #icon>
<Plus :size="25" />
Expand Down
Loading