feat(contacts): add CSV import for APP-ADMIN and IT-ADMIN (#39)#41
feat(contacts): add CSV import for APP-ADMIN and IT-ADMIN (#39)#41Ndacyayisenga-droid wants to merge 3 commits into
Conversation
hendrikebbers
left a comment
hendrikebbers
left a comment
There was a problem hiding this comment.
some minor comments
| /** | ||
| * Response from the CSV import preview endpoint. | ||
| */ | ||
| public record ContactImportPreviewResponse( |
There was a problem hiding this comment.
Can you add minimal javadoc for each attribute pls
| /** | ||
| * Request body for CSV import preview and commit endpoints. | ||
| */ | ||
| public record ContactImportRequest( |
There was a problem hiding this comment.
Can you add minimal javadoc for each attribute pls
| this.contactService = Objects.requireNonNull(contactService, "contactService must not be null"); | ||
| } | ||
|
|
||
| @Transactional(propagation = Propagation.REQUIRES_NEW) |
There was a problem hiding this comment.
Can you explain why propagation is set to a specific value here?
There was a problem hiding this comment.
Commits a single imported contact in its own transaction for partial-success imports.
Got it, makes sense :)
There was a problem hiding this comment.
Question: do you understand it? That is an interesting part of the code and something that is more special than the rest
| static final int MAX_ROWS = 5_000; | ||
| static final long MAX_FILE_SIZE_BYTES = 20L * 1024L * 1024L; | ||
|
|
||
| private static final Set<String> SUPPORTED_ENCODINGS = Set.of("UTF-8", "WINDOWS-1252"); |
There was a problem hiding this comment.
Question: can we use java.nio.charset.StandardCharsets instead of custom constants?
| this.rowSaver = Objects.requireNonNull(rowSaver, "rowSaver must not be null"); | ||
| } | ||
|
|
||
| public ContactImportPreviewResponse preview(final MultipartFile file, final ContactImportRequest request) { |
There was a problem hiding this comment.
I would assume this method in the controller since MultipartFile file is HTTP / Rest specific.
| } | ||
|
|
||
| public ImportResult commit(final MultipartFile file, final ContactImportRequest request) { | ||
| final ParsedCsv parsed = parseFile(file, request); |
There was a problem hiding this comment.
I beliefe the methods here should have ParsedCsv as param instead of MultipartFile. MultipartFile is more controller / rest specific
| chooseFile: "Datei wählen", | ||
| noFileChosen: "Keine Datei ausgewählt", | ||
| encoding: "Zeichenkodierung", | ||
| encodingUtf8: "UTF-8", |
There was a problem hiding this comment.
The 2 encodings are fixed values and do not need a translation fro m my point of view
| forbidden: "Sie haben keine Berechtigung, Kontakte zu importieren.", | ||
| generic: "Import fehlgeschlagen. Bitte erneut versuchen.", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Your German is really excellent :D
2 participants
cc @hendrikebbers