Skip to content

feat(import): Provide extended import report #3071

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

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

feat(import): Provide extended import report #3071

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
15 changes: 15 additions & 0 deletions cypress.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* SPDX-FileCopyrightText: 2023 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/
import { exec } from 'node:child_process'
import { defineConfig } from 'cypress'
import vitePreprocessor from 'cypress-vite'
import { nodePolyfills } from 'vite-plugin-node-polyfills'
Expand All @@ -24,6 +25,20 @@ export default defineConfig({
configFile: false,
}))

on('task', {
log(message) {
console.log(message)
return null
},
exec({ command, timeout = 60000 }) {
return new Promise((resolve) => {
exec(command, { timeout, cwd: config.projectRoot }, (error, stdout, stderr) => {
resolve({ code: error ? (error.code ?? 1) : 0, stdout, stderr })
})
})
},
})

// Disable spell checking to prevent rendering differences
on('before:browser:launch', (browser, launchOptions) => {
if (browser.family === 'chromium' && browser.name !== 'electron') {
Expand Down
41 changes: 39 additions & 2 deletions cypress/e2e/tables-import.cy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,38 @@ describe('Import csv', () => {
cy.intercept({ method: 'POST', url: '**/apps/tables/v2/import/table/*' }).as('importUploadReq')
cy.get('.modal__content button').contains('Import').click()
cy.wait('@importUploadReq').its('response.statusCode').should('equal', 200)

// The import runs as a queued background job — execute it via cron.php CLI
let cronResult
cy.task('exec', { command: 'php ../../cron.php -v "OCA\\Tables\\BackgroundJob\\ImportTableJob"', timeout: 120000 }, { timeout: 130000 })
.then(result => {
cronResult = result
cy.task('log', `[cron.php] code=${result.code}\nstdout: ${result.stdout}\nstderr: ${result.stderr}`)
})

const waitForProblemsFile = (retries = 15) => {
cy.task('exec', { command: 'find ../../data -name "*import-problems*" -print -quit' })
.then(result => {
if (result.stdout.trim() !== '') {
cy.visit('apps/files/files?dir=/Tables/Imports')
cy.get('[data-cy-files-list-row-name*="import-problems"]', { timeout: 20000 })
.should('be.visible')
return
}
if (retries <= 0) {
cy.task('exec', { command: 'tail -500 ../../data/nextcloud.log | grep -i "import\\|report" | tail -40' })
.then(r => {
throw new Error('Import problems file was not created. '
+ `cron.php: code=${cronResult?.code} stderr=${JSON.stringify(cronResult?.stderr)}\n`
+ `log tail: ${r.stdout}`)
})
return
}
cy.wait(2000)
waitForProblemsFile(retries - 1)
})
}
waitForProblemsFile()
})

it('Import small csv from device', () => {
Expand Down Expand Up @@ -100,7 +132,7 @@ describe('Import csv', () => {
cy.wait('@importUploadReq').its('response.statusCode').should('equal', 200)
})

it('Import csv from device with updating of existent files', () => {
it('Import csv from device with updating of existent files and a broken row', () => {
cy.intercept({ method: 'GET', url: '**/apps/tables/row/table/*' }).as('rowsReq')

cy.loadTable('Welcome to Nextcloud Tables!')
Expand All @@ -110,6 +142,7 @@ describe('Import csv', () => {
const csv = [
['id', 'What', 'How to do'],
[firstRow.id, 'What (Updated)', 'How to do (Updated)'],
['not-a-number', 'Skipped row', 'Skipped row'],
]

cy.writeFile('cypress/fixtures/test-import-update.csv', csv.map(row => row.join(',')).join('\n'))
Expand All @@ -133,7 +166,11 @@ describe('Import csv', () => {
cy.get('[data-cy="importResultRowsInserted"]').should('contain.text', '0')
cy.get('[data-cy="importResultRowsUpdated"]').should('contain.text', '1')
cy.get('[data-cy="importResultParsingErrors"]').should('contain.text', '0')
cy.get('[data-cy="importResultRowErrors"]').should('contain.text', '0')
cy.get('[data-cy="importResultRowErrors"]').should('contain.text', '1')
cy.get('[data-cy="importResultProblems"] tbody tr').should('have.length', 1)
cy.get('[data-cy="importResultProblems"] tbody tr').first()
.should('contain.text', '3')
.and('contain.text', 'not numeric')
})

it('Import small csv from device with an extra ignored column', () => {
Expand Down
22 changes: 11 additions & 11 deletions cypress/fixtures/test-import-large.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ Q1,Q2,4,True
R1,R2,5,False
S1,S2,0,True
T1,T2,1,False
U1,U2,2,True
U1,U2,99,True
V1,V2,3,False
W1,W2,4,True
X1,X2,5,False
Expand Down Expand Up @@ -197,7 +197,7 @@ M1,M2,2,True
N1,N2,3,False
O1,O2,4,True
P1,P2,5,False
Q1,Q2,0,True
Q1,Q2,99,True
R1,R2,1,False
S1,S2,2,True
T1,T2,3,False
Expand Down Expand Up @@ -297,7 +297,7 @@ I1,I2,0,True
J1,J2,1,False
K1,K2,2,True
L1,L2,3,False
M1,M2,4,True
M1,M2,99,True
N1,N2,5,False
O1,O2,0,True
P1,P2,1,False
Expand Down Expand Up @@ -397,7 +397,7 @@ E1,E2,4,True
F1,F2,5,False
G1,G2,0,True
H1,H2,1,False
I1,I2,2,True
I1,I2,99,True
J1,J2,3,False
K1,K2,4,True
L1,L2,5,False
Expand Down Expand Up @@ -497,7 +497,7 @@ A1,A2,2,True
B1,B2,3,False
C1,C2,4,True
D1,D2,5,False
E1,E2,0,True
E1,E2,99,True
F1,F2,1,False
G1,G2,2,True
H1,H2,3,False
Expand Down Expand Up @@ -597,7 +597,7 @@ W1,W2,0,True
X1,X2,1,False
Y1,Y2,2,True
Z1,Z2,3,False
A1,A2,4,True
A1,A2,99,True
B1,B2,5,False
C1,C2,0,True
D1,D2,1,False
Expand Down Expand Up @@ -697,7 +697,7 @@ S1,S2,4,True
T1,T2,5,False
U1,U2,0,True
V1,V2,1,False
W1,W2,2,True
W1,W2,99,True
X1,X2,3,False
Y1,Y2,4,True
Z1,Z2,5,False
Expand Down Expand Up @@ -797,7 +797,7 @@ O1,O2,2,True
P1,P2,3,False
Q1,Q2,4,True
R1,R2,5,False
S1,S2,0,True
S1,S2,99,True
T1,T2,1,False
U1,U2,2,True
V1,V2,3,False
Expand Down Expand Up @@ -897,7 +897,7 @@ K1,K2,0,True
L1,L2,1,False
M1,M2,2,True
N1,N2,3,False
O1,O2,4,True
O1,O2,99,True
P1,P2,5,False
Q1,Q2,0,True
R1,R2,1,False
Expand Down Expand Up @@ -997,7 +997,7 @@ G1,G2,4,True
H1,H2,5,False
I1,I2,0,True
J1,J2,1,False
K1,K2,2,True
K1,K2,99,True
L1,L2,3,False
M1,M2,4,True
N1,N2,5,False
Expand Down Expand Up @@ -1097,5 +1097,5 @@ C1,C2,2,True
D1,D2,3,False
E1,E2,4,True
F1,F2,5,False
G1,G2,0,True
G1,G2,99,True
H1,H2,1,False
45 changes: 43 additions & 2 deletions lib/Activity/ActivityManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,10 @@ private function createEvent(string $objectType, Row2|Table|View|Column $object,
break;
case self::SUBJECT_IMPORT_FINISHED:
$subjectParams['importStats'] = $additionalParams['importStats'] ?? null;
$subjectParams['importProblemsFile'] = $additionalParams['importProblemsFile'] ?? null;
break;
case self::SUBJECT_IMPORT_FAILED:
$subjectParams['errorMessage'] = $additionalParams['errorMessage'] ?? null;
break;
default:
throw new \Exception(sprintf('Unknown subject "%s" for activity.', $subject));
Expand Down Expand Up @@ -322,6 +326,14 @@ private function sendToUsers(IEvent $event, Row2|Table|View|Column $object) {
return;
}

if ($subject === self::SUBJECT_IMPORT_FAILED) {
if ($event->getAuthor() !== '') {
$event->setAffectedUser($event->getAuthor());
$this->manager->publish($event);
}
return;
}

if ($object instanceof Table || $object instanceof View) {
$uniqueAffectedUser = $this->findRecipientsByElement($object);
} elseif ($object instanceof Row2 || $object instanceof Column) {
Expand All @@ -333,7 +345,15 @@ private function sendToUsers(IEvent $event, Row2|Table|View|Column $object) {
return;
}

$subjectParams = $event->getSubjectParameters();
foreach ($uniqueAffectedUser as $userId) {
if ($subject === self::SUBJECT_IMPORT_FINISHED) {
$params = $subjectParams;
if ($userId !== $event->getAuthor()) {
unset($params['importStats'], $params['importProblemsFile']);
}
$event->setSubject($subject, $params);
}
$event->setAffectedUser($userId);

/** @noinspection DisconnectedForeachInstructionInspection */
Expand Down Expand Up @@ -592,7 +612,17 @@ public function getActivitySubject(string $language, $subjectIdentifier, array $
}

if ($subjectIdentifier === self::SUBJECT_IMPORT_FINISHED) {
return $ownActivity ? $l->t('You have imported file to table {table}') : $l->t('{user} has imported file to table {table}');
$hasProblemsFile = isset($subjectParams['importProblemsFile']['id'], $subjectParams['importProblemsFile']['name'], $subjectParams['importProblemsFile']['path']);
if ($ownActivity) {
return $hasProblemsFile
? $l->t('You have imported file to table {table} — problems report: {problemsFile}')
: $l->t('You have imported file to table {table}');
}
return $l->t('{user} has imported file to table {table}');
}

if ($subjectIdentifier === self::SUBJECT_IMPORT_FAILED) {
return $ownActivity ? $l->t('Your import to table {table} has failed') : $l->t('{user}\'s import to table {table} has failed');
}

return '';
Expand Down Expand Up @@ -787,11 +817,17 @@ private function isLinkShare(mixed $sharedWith): bool {
return is_array($sharedWith) && ($sharedWith['type'] ?? null) === ShareReceiverType::LINK;
}

public function getActivityMessage(string $language, $subjectIdentifier) {
/**
* @param array<string, mixed> $subjectParams
*/
public function getActivityMessage(string $language, $subjectIdentifier, array $subjectParams = []) {
$l = $this->l10nFactory->get(Application::APP_ID, $language);

switch ($subjectIdentifier) {
case self::SUBJECT_IMPORT_FINISHED:
if (!is_array($subjectParams['importStats'] ?? null)) {
return null;
}
$lines = [
$l->t('Found columns: {foundColumnsCount}'),
$l->t('Matching columns: {matchingColumnsCount}'),
Expand All @@ -802,6 +838,11 @@ public function getActivityMessage(string $language, $subjectIdentifier) {
$l->t('Row creation errors: {errorsCount}'),
];
return implode("\n", $lines);
case self::SUBJECT_IMPORT_FAILED:
$errorMessage = $subjectParams['errorMessage'] ?? null;
return is_string($errorMessage) && $errorMessage !== ''
? $l->t('Error: %s', [$errorMessage])
: null;
Comment thread
Koc marked this conversation as resolved.
Resolved
default:
return null;
}
Expand Down
31 changes: 22 additions & 9 deletions lib/Activity/TablesProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ public function parse($language, IEvent $event, ?IEvent $previousEvent = null):
$subjectIdentifier = $event->getSubject();
$subjectParams = $event->getSubjectParameters();
$ownActivity = ($event->getAuthor() === $this->userId);
if (!$ownActivity && in_array($subjectIdentifier, [ActivityManager::SUBJECT_IMPORT_FINISHED, ActivityManager::SUBJECT_IMPORT_FAILED], true)) {
unset($subjectParams['importStats'], $subjectParams['importProblemsFile'], $subjectParams['errorMessage']);
}

/**
* Map stored parameter objects to rich string types
Expand Down Expand Up @@ -206,19 +209,29 @@ public function parse($language, IEvent $event, ?IEvent $previousEvent = null):
}

$messageParameters = [];
if ($event->getSubject() === ActivityManager::SUBJECT_IMPORT_FINISHED) {
$messageParameters['{foundColumnsCount}'] = $subjectParams['importStats']['foundColumnsCount'];
$messageParameters['{matchingColumnsCount}'] = $subjectParams['importStats']['matchingColumnsCount'];
$messageParameters['{createdColumnsCount}'] = $subjectParams['importStats']['createdColumnsCount'];
$messageParameters['{insertedRowsCount}'] = $subjectParams['importStats']['insertedRowsCount'];
$messageParameters['{updatedRowsCount}'] = $subjectParams['importStats']['updatedRowsCount'];
$messageParameters['{errorsParsingCount}'] = $subjectParams['importStats']['errorsParsingCount'];
$messageParameters['{errorsCount}'] = $subjectParams['importStats']['errorsCount'];
if ($event->getSubject() === ActivityManager::SUBJECT_IMPORT_FINISHED && is_array($subjectParams['importStats'] ?? null)) {
$messageParameters['{foundColumnsCount}'] = (string)$subjectParams['importStats']['foundColumnsCount'];
$messageParameters['{matchingColumnsCount}'] = (string)$subjectParams['importStats']['matchingColumnsCount'];
$messageParameters['{createdColumnsCount}'] = (string)$subjectParams['importStats']['createdColumnsCount'];
$messageParameters['{insertedRowsCount}'] = (string)$subjectParams['importStats']['insertedRowsCount'];
$messageParameters['{updatedRowsCount}'] = (string)$subjectParams['importStats']['updatedRowsCount'];
$messageParameters['{errorsParsingCount}'] = (string)$subjectParams['importStats']['errorsParsingCount'];
$messageParameters['{errorsCount}'] = (string)$subjectParams['importStats']['errorsCount'];

if (isset($subjectParams['importProblemsFile']['id'], $subjectParams['importProblemsFile']['name'], $subjectParams['importProblemsFile']['path'])) {
$subjectParameters['problemsFile'] = [
'type' => 'file',
'id' => (string)$subjectParams['importProblemsFile']['id'],
'name' => (string)$subjectParams['importProblemsFile']['name'],
'path' => (string)$subjectParams['importProblemsFile']['path'],
'link' => $this->urlGenerator->linkToRouteAbsolute('files.viewcontroller.showFile', ['fileid' => $subjectParams['importProblemsFile']['id']]),
];
Comment thread
Koc marked this conversation as resolved.
Resolved
}
}

try {
$subject = $this->activityManager->getActivitySubject($language, $subjectIdentifier, $subjectParams, $ownActivity);
$message = $this->activityManager->getActivityMessage($language, $subjectIdentifier);
$message = $this->activityManager->getActivityMessage($language, $subjectIdentifier, $subjectParams);
$this->parseEvent($event, $subject, $subjectParameters, $message, $messageParameters);
} catch (\Exception $e) {
$this->logger->warning('Could not parse activity: ' . $e->getMessage(), ['exception' => $e]);
Expand Down
Loading
Loading