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

feat: implement direct file upload #845

Merged
merged 3 commits into from
Feb 28, 2024
Merged

feat: implement direct file upload #845

merged 3 commits into from
Feb 28, 2024

Conversation

luka-nextcloud
Copy link
Contributor

demo.2.webm

@luka-nextcloud luka-nextcloud self-assigned this Feb 15, 2024
@luka-nextcloud luka-nextcloud added enhancement New feature or request 3. to review Waiting for reviews import/export labels Feb 15, 2024
@juliusknorr juliusknorr requested review from blizzz and enjeck February 15, 2024 09:47
blizzz
blizzz approved these changes Feb 15, 2024
View reviewed changes
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.

Looks good. Could not test fully, because of an unrelated error.

One nitpick: when I upload a beautiful (i.e. formatted cells) ods file for import with four rows, it also tries to import the empty rows, resulting in 16384 row creation errors. Might an issue of the IOFactory lib though, and unrelated to this PR.

Screenshot_20240215_190613

lib/Controller/ImportController.php
Comment on lines +106 to +115
$phpFileUploadErrors = [
UPLOAD_ERR_OK => $this->l10n->t('The file was uploaded'),
UPLOAD_ERR_INI_SIZE => $this->l10n->t('The uploaded file exceeds the upload_max_filesize directive in php.ini'),
UPLOAD_ERR_FORM_SIZE => $this->l10n->t('The uploaded file exceeds the MAX_FILE_SIZE directive that was specified in the HTML form'),
UPLOAD_ERR_PARTIAL => $this->l10n->t('The file was only partially uploaded'),
UPLOAD_ERR_NO_FILE => $this->l10n->t('No file was uploaded'),
UPLOAD_ERR_NO_TMP_DIR => $this->l10n->t('Missing a temporary folder'),
UPLOAD_ERR_CANT_WRITE => $this->l10n->t('Could not write file to disk'),
UPLOAD_ERR_EXTENSION => $this->l10n->t('A PHP extension stopped the file upload'),
];
Copy link
Member

Choose a reason for hiding this comment

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

This is used on so many places… should get into some public API imo. But out of scope here.

lib/Service/ImportService.php Outdated Show resolved Hide resolved
src/modules/modals/Import.vue
Comment on lines +205 to +211
'application/xml',
'text/html',
Copy link
Member

Choose a reason for hiding this comment

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

just to doublecheck that XML and HTML are indeed supported for imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are using phpspreasheet to parse import file. It does support xml and html file formats. (https://phpspreadsheet.readthedocs.io/en/latest/)

enjeck
enjeck reviewed Feb 15, 2024
View reviewed changes
Copy link
Contributor

@enjeck enjeck left a comment

Choose a reason for hiding this comment

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

This is a cool feature 😎

It's not immediately obvious to me how Select a file and Upload a file are different. I think we should rename the buttons to be consistent with what we have in other Nextcloud apps. For example, in collectives, we use Upload from computer and Insert from Files.
image

@juliusknorr
Copy link
Member

@nextcloud/designers Maybe you can check this one and suggest how we should layout the different upload buttons?

@nimishavijay
Copy link
Member

nimishavijay commented Feb 19, 2024 •
edited by luka-nextcloud
Loading

This is a super cool feature! :) General flow looks good! Some comments:

  • Agreed with @enjeck that the buttons can be named "Upload from device" and "Select from Files" :)
  • The placement of the buttons can also be improved. The text fields next to the buttons can be removed, the buttons can be on the same row and selected file name can be above the buttons
  • In the "Create missing columns" section, the explanation text is cut off. It should wrap around, or can even be removed.
  • The "Information" section currently has a lot of text, some of it can be removed (for example, the "The possible importing..." point seems very technical, would it cause any confusion if it was not there?) and some of it can be inserted in relevant places

So the end result will look something like this

image
image

  • The text on the loading screen can also be reduced if it is not critical
  • Instead we can show the name of the file (ellipsised if it is too long)

image

  • For the result screen, there can be an icon on the left of the heading indicating if there are any errors. If there are no errors, then it can be a checkmark and if there are errors it can be a warning
  • If there no errors, the last 2 rows can be hidden
  • If there are errors, it could be indicated by an icon or emoji
✅ Imported from n26_transactions.csv

Found columns        4
Matching columns     0
Created columns      4
Inserted rows        0

⚠️ Imported from n26_transactions.csv

Found columns          4
Matching columns       0
Created columns        4
Inserted rows          0
Value parsing errors   1 ⚠️ 
Row creation errors    1 ⚠️

What do you think? :)

@luka-nextcloud luka-nextcloud force-pushed the feature/import-table-with-upload-file branch from ea126c7 to a6389b3 Compare February 20, 2024 18:41
@luka-nextcloud
feat: implement direct file upload
4b3ea82 
Signed-off-by: Luka Trovic <luka@nextcloud.com>
@luka-nextcloud
feat: implement direct file upload
c86aeb4 
Signed-off-by: Luka Trovic <luka@nextcloud.com>
@luka-nextcloud
feat: implement direct file upload
207c0a8 
Signed-off-by: Luka Trovic <luka@nextcloud.com>
@luka-nextcloud luka-nextcloud force-pushed the feature/import-table-with-upload-file branch from a6389b3 to 207c0a8 Compare February 23, 2024 06:28
@luka-nextcloud
Copy link
Contributor Author

demo.3.webm

juliusknorr
juliusknorr approved these changes Feb 28, 2024
View reviewed changes
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Very nice 👍

@juliusknorr juliusknorr merged commit 9558791 into main Feb 28, 2024
50 checks passed
@juliusknorr juliusknorr deleted the feature/import-table-with-upload-file branch February 28, 2024 15:59
@juliusknorr
Copy link
Member

@blizzz Regarding your row errors mentioned in #845 (review) any chance you have a copy of the file you tried? I could not reproduce that in any way

@juliusknorr juliusknorr mentioned this pull request Feb 28, 2024
Open
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliusknorr juliusknorr juliusknorr approved these changes

@enjeck enjeck Awaiting requested review from enjeck enjeck is a code owner

@blizzz blizzz Awaiting requested review from blizzz blizzz is a code owner

Assignees

@luka-nextcloud luka-nextcloud

Labels
3. to review Waiting for reviews enhancement New feature or request import/export
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Import: Implement direct file upload
5 participants
@luka-nextcloud @juliusknorr @nimishavijay @blizzz @enjeck