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

implemented Import csv from filemanger #52

Closed
wants to merge 44 commits into from
Closed

Conversation

MaxIsV
Copy link
Contributor

@MaxIsV MaxIsV commented Feb 13, 2023

No description provided.

k3b and others added 30 commits January 28, 2023 12:23
* incremented java to version 11 to allow java stream api
* added interface NameWithIdProvider to entities to allow generic translation between id and name
* created empty CsvExporter with a failing junit4-test
SecUSo#42: implemented createCsvHeader; extracted CSV_FIELD_DELIMITER
…ose; added a Writer argument to CsvExporter constructor; extracted ICSVWriter variable
SecUSo#42: merged openCsv implementation
SecUSo#42: re-added accidently deleted unittest
SecUSo#51: created branch ImportCsv. Created CsvImporter, CsvDefinitions, C…
Copy link
Contributor

@udenr udenr left a comment

Choose a reason for hiding this comment

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

As far as I can see, I can only do the import if I open the CSV file from another app. I think many users will not discover the feature this way. Would it be possible to add an Import button to the Transactions menu, like the Export button?

Apart from a few minor issues, it looks good to me. 👍

<string name="nav_title_export_as_csv">Als CSV exportieren</string>

<!-- parameter 1: specific text parameter 2 exception messasage -->
<string name="err_cannot_read_import_file">Kann CSVDatei nicht lesen aus %s : %s</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be Kann CSV-Datei nicht lesen aus %s : %s

<!-- parameter 1: specific text parameter 2 exception messasage -->
<string name="err_cannot_read_import_file">Kann CSVDatei nicht lesen aus %s : %s</string>
<string name="err_cannot_save_to_db">Kann %s nicht in Datenbank speichern\n\t %s</string>
<string name="info_import_success">Ergebnis: (%s/%s) Transactionen importiert</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: Ergebnis: (%s/%s) Transaktionen importiert

} catch (Exception ex) {
addError(ex.getMessage());
}
System.out.println(line);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this debug output.

@udenr
Copy link
Contributor

udenr commented Mar 27, 2023

@MaxIsV @k3b Will you continue to work on this?

@k3b
Copy link
Contributor

k3b commented Mar 27, 2023

I (k3b) currently have no contact to my former student (MaxIsV)

I try to fix this

@k3b
Copy link
Contributor

k3b commented Mar 27, 2023

I have no write permission for MaxIsV:ImportCsv.

I will try tomorrow .....

k3b added a commit to k3b/privacy-friendly-finance-manager that referenced this pull request Mar 28, 2023
@k3b
Copy link
Contributor

k3b commented Mar 28, 2023

I created a new pullrequest #56 with the fixes that will replace this request.


@udenr :

As far as I can see, I can only do the import if I open the CSV file from another app.
I think many users will not discover the feature this way.
Would it be possible to add an Import button to the Transactions menu, like the Export button?

I totally agree.

I have added a new issue #57 because handling this would require additional read-from-file-permissions

@k3b k3b mentioned this pull request Mar 28, 2023
@k3b
Copy link
Contributor

k3b commented Mar 28, 2023

@udenr is it ok to merge import without the proposed menu item?

@udenr
Copy link
Contributor

udenr commented Mar 29, 2023

I will close this because it was replaced by #56

@udenr udenr closed this Mar 29, 2023
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.

3 participants