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

1713: Fix MailNotification validation in CSV import #1715

Merged
merged 3 commits into from
Nov 4, 2024

Conversation

steffenkleinle
Copy link
Member

@steffenkleinle steffenkleinle commented Oct 24, 2024

Short description

Fix MailNotification validation in CSV import.

Proposed changes

  • Use default extension state if not provided
  • Show - if string is empty

Side effects

None.

Testing

  1. Go to http://localhost:3000/cards/import
  2. Import bulk data
  3. See the mail notification field error validation while cards can be created

Resolved issues

Fixes: #1713

@steffenkleinle steffenkleinle marked this pull request as ready for review October 25, 2024 11:31
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.

Looks good. tested on chrome

Copy link
Contributor

@seluianova seluianova left a comment

Choose a reason for hiding this comment

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

Could you please provide a csv example to reproduce the issue?
I tried this, MailNotification is empty, but I don't get a validation error on the main branch.

Name,Ablaufdatum,Kartentyp,MailNotification
Qwerty,,Goldkarte,

@f1sh1918
Copy link
Contributor

Could you please provide a csv example to reproduce the issue? I tried this, MailNotification is empty, but I don't get a validation error on the main branch.

Name,Ablaufdatum,Kartentyp,MailNotification
Qwerty,,Goldkarte,

you can use the one in the repo on the main branch (resources/bulk-bavaria) it should be marked red

@steffenkleinle
Copy link
Member Author

steffenkleinle commented Oct 31, 2024

Could you please provide a csv example to reproduce the issue? I tried this, MailNotification is empty, but I don't get a validation error on the main branch.

Name,Ablaufdatum,Kartentyp,MailNotification
Qwerty,,Goldkarte,

you can use the one in the repo on the main branch (resources/bulk-bavaria) it should be marked red

Exactly. The error was shown if MailNotification is not present at all in the csv.

Copy link
Contributor

@seluianova seluianova left a comment

Choose a reason for hiding this comment

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

The error was shown if MailNotification is not present at all in the csv.

💭 okay, than it works as requested, but tbh I would expect 'missing column' error in such case, because this column is specified in csv format:
image
there is no clue, that this column can be ommited.
also I think it would be nice to mark mandatory columns with an * in the table view, when not all of them are mandatory.

Copy link
Contributor

@seluianova seluianova left a comment

Choose a reason for hiding this comment

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

not sure if the thought above should block this PR, so adding my approval anyway

@steffenkleinle
Copy link
Member Author

steffenkleinle commented Nov 4, 2024

The error was shown if MailNotification is not present at all in the csv.

💭 okay, than it works as requested, but tbh I would expect 'missing column' error in such case, because this column is specified in csv format

Yes, it is specified since it can also be passed to allow for automatic email notifications.

there is no clue, that this column can be ommited. also I think it would be nice to mark mandatory columns with an * in the table view, when not all of them are mandatory.

Sounds like a good addition. I'll create a new issue for this. We could also do something like MailNotification (optional) or have a second sentence with something like optional columns are.....

Edit: New issue: #1738

# Conflicts:
#	administration/src/cards/Card.ts
@steffenkleinle steffenkleinle merged commit bc75d3f into main Nov 4, 2024
1 check passed
@steffenkleinle steffenkleinle deleted the 1713-csv-mail-validation branch November 4, 2024 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix wrong validation of empty mail notification for csv import
3 participants