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

1492: Add bulk import from freinet #1541

Merged
merged 8 commits into from
Aug 26, 2024
Merged

Conversation

ztefanie
Copy link
Member

@ztefanie ztefanie commented Jul 23, 2024

Short description

Adding bulk import csv option from freinet csv format

Proposed changes

  • Added new route "import/?freinet=true"
  • Added project config setting to enable freinet import
  • Add function that adjusted freinet csvs to expected csv input format
  • Add button to import from freinet
  • Tests are missing, I will create a ticket for them if i will not manage to add some this week before my vacation

Side effects

  • Maybe standart csv import is somehow affected

Resolved issues

Fixes: #1492

@ztefanie ztefanie force-pushed the 1492-add-bulk-import-from-freinet branch from 3584167 to 9c3b8f6 Compare July 23, 2024 09:06
Copy link
Contributor

@f1sh1918 f1sh1918 left a comment

Choose a reason for hiding this comment

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

Added some comments.
In general it think we should have an error handling if the column names do not exist
If the column names in the header (f.e. vorname, nachname) do not match our format, i just get undefined name but can create cards.

image

csvHeader[csvHeader.length] = nameCoulmnName
}

const indexVorname = csvHeader.indexOf(FREINET_FIRSTNAME_COLUMN_HEADER)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should add some error handling if columns do not exists

# Conflicts:
#	administration/src/project-configs/bayern/config.ts
#	administration/src/project-configs/getProjectConfig.ts
#	administration/src/project-configs/nuernberg/config.ts
#	administration/src/project-configs/showcase/config.ts
@ztefanie ztefanie requested a review from f1sh1918 August 19, 2024 08:05
@f1sh1918
Copy link
Contributor

f1sh1918 commented Aug 19, 2024

@ztefanie
i used the import files in the mentioned issue. But i get all these empty lines, caused by ;

A second (optional) thing could be to set a null for the columnNames mail extension (my issue)
extensionColumnNames: ['Kartentyp', null, null], to not show the mail notification column
We could think about providing this as a feature that they could export the mail address of the user and send the mail notification also for bulk imports. But i think we didn't make a decision on this

image

@ztefanie
Copy link
Member Author

@ztefanie i used the import files in the mentioned issue. But i get all these empty lines, caused by ;

A second (optional) thing could be to set a null for the columnNames mail extension (my issue) extensionColumnNames: ['Kartentyp', null, null], to not show the mail notification column We could think about providing this as a feature that they could export the mail address of the user and send the mail notification also for bulk imports. But i think we didn't make a decision on this

image

Yes both is as it should be.
In the example input are empty lines and they should be displayed like this.
About the email notification, we can do this if they request it at a later point.

Copy link
Contributor

@f1sh1918 f1sh1918 left a comment

Choose a reason for hiding this comment

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

Alright works as expected 👍

For later testing i think it could help to also provide a working csv example.
Sorry i was a bit confused while testing it ^^

administration/src/Router.tsx Show resolved Hide resolved
const FREINET_LASTNAME_COLUMN_HEADER = 'nachname'
const FREINET_CARDTYPE_COLUMN_HEADER = 'inhaber_ehrenamtskarte'

const mergeFirstAndLastnameIntoNewColumn = (line: string[], csvHeader: string[], nameColumnName: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

🙃 Is all of this done in place as no copies or anything are returned? Wouldn't it be a lot safer to work with copies? Looks kindof error prone to me.
E.g. if the header is adjusted in an early iteration, doesn't that have implications for later iterations?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it does impact later iterations, but this is handled and works as expected. You are right it may be not the best implementation, but we are not planning to have this feature for a ever or a long time, it is just a workaround, we will remove this as soon as Freinet and entitlemtcard are connected.

Copy link
Member

@steffenkleinle steffenkleinle left a comment

Choose a reason for hiding this comment

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

Tested on firefox. I can open the CSV you linked in the issue (#1492 (comment)), but it looks like this:
image

So it checked that there are invalid values and therefore blocks creating the QR codes, but I am not able to change the names or anything? Am I doing something wrong?

If this is supposed to happen in another issue/PR, it works as expected!

Tests are missing, I will create a ticket for them if i will not manage to add some this week before my vacation

Did you manage to do this?

@steffenkleinle
Copy link
Member

And perhaps you could add the csv here too? https://github.com/digitalfabrik/entitlementcard/tree/main/administration/resources/cards

@f1sh1918
Copy link
Contributor

Tested on firefox. I can open the CSV you linked in the issue (#1492 (comment)), but it looks like this: image

So it checked that there are invalid values and therefore blocks creating the QR codes, but I am not able to change the names or anything? Am I doing something wrong?

If this is supposed to happen in another issue/PR, it works as expected!

Tests are missing, I will create a ticket for them if i will not manage to add some this week before my vacation

Did you manage to do this?

this is expected. just remove the ";" from the csv and create a card :) @steffenkleinle

…from-freinet

# Conflicts:
#	administration/src/bp-modules/cards/ImportCardsInput.tsx
@f1sh1918
Copy link
Contributor

resolved merge conflicts and merge @ztefanie to put it in the next release

@f1sh1918 f1sh1918 merged commit 0e9791f into main Aug 26, 2024
1 check passed
@f1sh1918 f1sh1918 deleted the 1492-add-bulk-import-from-freinet branch August 26, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add option to import from freinet
3 participants