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(import): change column format during import #944

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

feat(import): change column format during import #944

wants to merge 1 commit into from

Conversation

luka-nextcloud
Copy link
Contributor

@luka-nextcloud luka-nextcloud self-assigned this Mar 19, 2024
@luka-nextcloud luka-nextcloud added enhancement New feature or request 3. to review Waiting for reviews labels Mar 19, 2024
enjeck
enjeck reviewed Mar 22, 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.

I tested it out, and the feature works. 😃 👍

Left some thoughts and points of confusion (for me, might be debatable 😅 )

src/modules/modals/ImportPreview.vue Outdated Show resolved Hide resolved
src/modules/modals/ImportPreview.vue Outdated Show resolved Hide resolved
src/modules/modals/ImportPreview.vue Show resolved Hide resolved
src/modules/modals/ImportPreview.vue Outdated Show resolved Hide resolved
src/modules/modals/ImportPreview.vue Outdated Show resolved Hide resolved
src/modules/modals/ImportPreview.vue Show resolved Hide resolved
src/modules/modals/ImportPreview.vue Outdated Show resolved Hide resolved
src/modules/modals/Import.vue Show resolved Hide resolved
src/modules/modals/Import.vue Show resolved Hide resolved
@luka-nextcloud luka-nextcloud force-pushed the feature/change-column-format-during-import branch 3 times, most recently from 6b7cde9 to 1c24c4d Compare April 10, 2024 17:29
@luka-nextcloud luka-nextcloud force-pushed the feature/change-column-format-during-import branch from 1c24c4d to 5114238 Compare April 22, 2024 18:12
max-nextcloud
max-nextcloud requested changes Apr 23, 2024
View reviewed changes
Copy link
Contributor

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

Failing cypress tests are related and point to an issue in lib/Service/ImportService.php.

src/modules/modals/Import.vue Show resolved Hide resolved
lib/Service/ImportService.php Outdated Show resolved Hide resolved
@luka-nextcloud luka-nextcloud force-pushed the feature/change-column-format-during-import branch from 5114238 to 212f3fd Compare April 23, 2024 08:19
@max-nextcloud max-nextcloud self-assigned this Apr 23, 2024
@max-nextcloud max-nextcloud self-requested a review April 23, 2024 11:54
max-nextcloud
max-nextcloud requested changes Apr 23, 2024
View reviewed changes
Copy link
Contributor

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

When importing the same file multiple times but selecting 'New Column' it will still import the data into the old column as long as the name stays the same.

image

Imported the same data 4 times here:

  1. for seeding the columns.
  2. i selected the 'Separated' column to be created anew. It created the column but did not insert the data.
  3. I change the file to have a different title for 'Separated'. Now the data ended up in the right place.
  4. I change the title for the new column (first one this time) to be 'Comma2' - this also worked.

So the only thing that does not seem to work is using the same title twice.

@luka-nextcloud
Copy link
Contributor Author

@max-nextcloud @juliushaertl Regarding the column title issue, currently, we do not restrict the duplicate column title. I think we should not allow to create columns with the same title on same table. What do you think?

@juliushaertl
Copy link
Member

@max-nextcloud @juliushaertl Regarding the column title issue, currently, we do not restrict the duplicate column title. I think we should not allow to create columns with the same title on same table. What do you think?

I think that would make sense to restrict, maybe we can when importing just add a number to the titles that have duplicates, e.g. having three columns called "title" will result in "title", "title (2)", "title (3)" like we do it in files

@juliushaertl juliushaertl added this to the v0.8.0 milestone Apr 24, 2024
@luka-nextcloud luka-nextcloud force-pushed the feature/change-column-format-during-import branch from 212f3fd to 60fc113 Compare April 25, 2024 15:38
@luka-nextcloud luka-nextcloud force-pushed the feature/change-column-format-during-import branch from 60fc113 to c9660fd Compare April 26, 2024 12:10
@luka-nextcloud luka-nextcloud force-pushed the feature/change-column-format-during-import branch from c9660fd to 5a2d919 Compare April 30, 2024 11:35
@jancborchardt
Copy link
Member

Are there any screenshots or quick videos for easier design review, or is all good on that side? :)

@luka-nextcloud
Copy link
Contributor Author

Are there any screenshots or quick videos for easier design review, or is all good on that side? :)

@jancborchardt Please check the video.

demo.webm

@juliushaertl
Copy link
Member

@nextcloud/designers Could you have a look at this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliushaertl juliushaertl Awaiting requested review from juliushaertl

@enjeck enjeck Awaiting requested review from enjeck

@max-nextcloud max-nextcloud Awaiting requested review from max-nextcloud

@elzody elzody Awaiting requested review from elzody

Requested changes must be addressed to merge this pull request.

Assignees

@luka-nextcloud luka-nextcloud

@max-nextcloud max-nextcloud

Labels
3. to review Waiting for reviews enhancement New feature or request
Projects
📝 Office team
Status: 👀 In review
Milestone
v0.8.0
Development

Successfully merging this pull request may close these issues.

Import: Allow changing row format during import
6 participants
@luka-nextcloud @juliushaertl @jancborchardt @elzody @enjeck @max-nextcloud