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

Feat: File action to import from Files to Tables #890

Merged
merged 3 commits into from
Mar 8, 2024
Merged

Conversation

elzody
Copy link
Contributor

@elzody elzody commented Mar 1, 2024

Related Issue

This feature is in response to issue #365

Explanation

This feature adds a file action to files in the Files app, which allows you to import the file into Tables, provided it is a supported file format. Upon clicking the "Import into Tables" button in the file action menu, a dialog will be displayed which lets you choose to import the data as a new table, or if you want to import the data into an existing table.

Importing the data as a new table allows you to choose an icon and a name for the table, and if none are provided, will use the 🔧 icon and the file name for the table information. If you choose to import the file into an already existing table, it allows you to select one of your tables and gives you the choice of creating missing columns or not. *

* Possible bug with this, will look into it

Screenshots

File action

grafik

Import dialog

grafik
grafik

Import results dialog

grafik

TODO

  • Look into better way of including the Tables icon in the file action than hardcoding the SVG string
  • Because 2 new components were added, look into maybe the re-usability of these, particularly the import results dialog?
  • No testing yet, so this would probably be a good thing to include at some point
  • Instead of passing t function for translation to the modal, can probably just import it itself (just realized this was a thing)
  • Implement better error handling / error feedback for the user
  • There seems to be a residual commit that is unrelated to this PR; it should be dropped

Follow up

  • Fix potential issue with importing into an existing table when the "Create missing columns" toggle is disabled
  • Upon mounting the import dialog component, the user's tables are retrieved from the backend. Look into ways to cache this and reduce number of network requests.

@elzody elzody added enhancement New feature or request 2. developing Work in progress labels Mar 1, 2024
@elzody elzody linked an issue Mar 1, 2024 that may be closed by this pull request
Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

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

A few comments, only read the code so far, haven't tried it yet

lib/Listener/LoadAdditionalListener.php Show resolved Hide resolved
lib/Listener/LoadAdditionalListener.php Outdated Show resolved Hide resolved
src/file-actions.js Outdated Show resolved Hide resolved
src/modules/modals/ImportResults.vue Outdated Show resolved Hide resolved
@juliushaertl
Copy link
Member

When wrapping up, would also be nice to squash commits in a meaningful way a bit if possible or just all in one feature commit which makes the history once merged a bit clearer. Also I haven't mentioned before and we don't enforce this, but most of us try to somewhat stick to conventional commits https://www.conventionalcommits.org/en/v1.0.0/ Might be nice to adapt that as well ;)

src/file-actions.js Outdated Show resolved Hide resolved
Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

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

Just two comments, otherwise this looks good 👍

@juliushaertl
Copy link
Member

Just pushed two small fixups when trying out locally. First one for cypress, second adding the missing dependencies

@juliushaertl
Copy link
Member

The new files api only works with 28 so we just just skip the cypress tests on earlier versions.

@juliushaertl
Copy link
Member

We can just pass in the server version as collectives does in the cypress.yml github workflow:

CYPRESS_ncVersion: ${{ matrix.server-versions }}

Then we can skip the tests in the spec files

102:			if (['stable26', 'stable27'].includes(Cypress.env('ncVersion'))) {

@juliushaertl
Copy link
Member

Rebased, squashed and pushed a small follow up fix. I noticed that the tests seemed to fail only sometimes where the file action was not showing up. This might happen if the files app code initializes before the action was registered, so I moved the script loading to be earlier

@juliushaertl juliushaertl added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Mar 8, 2024
elzody and others added 3 commits March 8, 2024 12:28
Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

basic implementation of import to tables button

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

work on api request to import table

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

addition of import modal and import logic

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

update variable names for clarity and consistency

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

added error handling and feedback to import modal

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

updated valid importable file formats

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

added error handling for unimportable file type

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

removed unused import

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

modal organization/code cleanup

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

added error messages and modal visibility

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

reworked error handling

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

added import results dialog, renamed files

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

only render button when valid file type

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

fixed enabled for valid file types

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

fix lint errors

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

added info to aid static analysis

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

made ImportResults into reusable component

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

refactored to use ImportResults component

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

change icon from hardcoded svg string to icon import

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

added cypress tests for file action import

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

fix: dynamic import for FileActionImport component

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>

test: refactored tests for file importing

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>
…n import test for v26-27

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl juliushaertl merged commit 6ff2f87 into main Mar 8, 2024
39 of 41 checks passed
@juliushaertl juliushaertl deleted the feat/365 branch March 8, 2024 12:01
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement New feature or request feedback-requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add import link to spreadsheets in files
2 participants