CSV bills import (cospend compatible) #951

youegraillot · 2021-11-28T15:20:47Z

Hi, bigger pr this time to implement CSV project import

CSV compatible import, same form input as JSON, we sort that out server-side
Cospend compatible import
Fix errors logs/flash in edit page
Updated localization (best effort)
Optimized Person fetching (get_by_ids and get_by_names)

This is a draft for now, I'm planning to update and add some more tests for this feature

Glandos

I am glad to see more Cospend compat code 👍

However, I have mixed feelings about the new __init__ for Bill. In some case, it is helpful, but I feel that it is sometimes less readable than previous code with explicit affectations.

Great move though for replacing fake_form by an explicit export.

As a last comment, you can remove all translations from this, Weblate will handle this for us.

ihatemoney/forms.py

ihatemoney/translations/bn_BD/LC_MESSAGES/messages.po

Glandos · 2021-11-28T20:58:40Z

ihatemoney/models.py

+                Bill(
+                    amount,
+                    None,
+                    None,
+                    "XXX",
+                    [members[name] for name in owers],
+                    members[payer].id,
+                    project.default_currency,
+                    what,
+                )


Using the __init__ is somewhat confusing here.

Modified init with named parameters, if it's still a problem I can revert it

almet · 2021-11-28T22:28:49Z

That's cool! Thanks for taking the time to implement this :-)

I wonder if this will break importing previously exported project data?

youegraillot · 2021-11-28T22:31:59Z

That's cool! Thanks for taking the time to implement this :-)

I wonder if this will break importing previously exported project data?

As far as I tested nope, same behavior as for json : only add missing transactions and persons

almet · 2021-12-10T12:59:56Z

Hey, let us know if you need some help on this, we might be able to find some time to take over if you want!

youegraillot · 2021-12-11T11:56:52Z

Hi, I'm planning to update this in the following days when I get some time, but for this PR or any future one feel completely free to update it !

youegraillot · 2021-12-13T03:34:27Z

Now with CSV tests I believe this PR is ready for review :)

I'm not really satisfied with the CSV/JSON tests implementation but I could not find a better way without converting to pytest, btw I'm wondering why this project uses unittest instead ?

almet

Thanks for doing this and tidying up the tests during the process :-) I believe we're not using py.test because we are not familiar with it, but if you want and know how to improve these tests with py.test it will be useful and appreciated, don't hesitate to open a PR (or an issue) about it.

I've not reviewed all the part with the tests due to lack of time right now, but I've left a few comments here and there.

almet · 2021-12-13T22:10:49Z

ihatemoney/models.py

+        db.session.commit()
+
+        # Import bills not already in the project
+        bills_project = self.get_pretty_bills()


I'm not sure I follow here: this is named bills_project but it seems to contain bills instead, right? If that's the case, we should probably rename the variable here.

I think I understand. It seems to be the bills of the existing project. If this is the case, we could name it project_bills :-)

almet · 2021-12-13T22:13:19Z

ihatemoney/models.py

+                # Create bills
+                db.session.add(
+                    Bill(
+                        amount=b["amount"],


We probably should check that the data contains this key before using it, otherwise it may fail.

Or we might want to try/catch this entire block and display an error message with the format of the required data.

import_bills is already called from a try/catch block in web.import_project
but I agree we should add another one in import_bills as a safeguard in case we use the method elsewhere
done !

almet · 2021-12-13T22:21:19Z

ihatemoney/utils.py

+    reader = csv.DictReader(csv_file)
+    result = []
+    for r in reader:
+        # cospend filtering


Could you please expand here on why this is required and what's it's doing? Thanks :-)

Yup
by the way I thought about also exporting data in cospend format (with lines for users and a table for categories)
it would simplify our csv import code AND render IHM exports compatible with cospend

youegraillot · 2021-12-13T23:40:12Z

pytest would indeed simplify code and avoid some ugly duplication, but I honestly don't have the motivation to refactor all tests. I might write new ones with it though if there is no reason not to do so

Thanks for your feedbacks I'll check these later this week

Glandos · 2021-12-14T22:08:32Z

ihatemoney/forms.py

+        return Bill(
+            float(self.amount.data),
+            self.date.data,
+            self.external_link.data,
+            str(self.original_currency.data),
+            Person.query.get_by_ids(self.payed_for.data, project),
+            self.payer.data,
+            project.default_currency,
+            self.what.data,
+        )


It could be good to also use named parameters here for consistency

Oops, forgot to include them here, fixed !

Glandos · 2021-12-14T22:12:15Z

@Youe Graillot commented on 13 déc. 2021, 04:34 UTC+1:

Now with CSV tests I believe this PR is ready for review :)

I'm not really satisfied with the CSV/JSON tests implementation but I could not find a better way without converting to pytest, btw I'm wondering why this project uses unittest instead ?

But… this project uses pytest: https://github.com/spiral-project/ihatemoney/blob/master/setup.cfg#L57

And I'm running all tests locally with .venv/bin/pytest --pyargs ihatemoney.tests.main_test -k weight, it all run fine.

youegraillot · 2021-12-14T22:21:55Z

@Youe Graillot commented on 13 déc. 2021, 04:34 UTC+1:

Now with CSV tests I believe this PR is ready for review :)
I'm not really satisfied with the CSV/JSON tests implementation but I could not find a better way without converting to pytest, btw I'm wondering why this project uses unittest instead ?

But… this project uses pytest: https://github.com/spiral-project/ihatemoney/blob/master/setup.cfg#L57

And I'm running all tests locally with .venv/bin/pytest --pyargs ihatemoney.tests.main_test -k weight, it all run fine.

Running tests with pytest does not mean they are written using pytest, which can run unittest but with caveats.
Fixtures, parametrization and hooks are not supported with this method.

almet · 2021-12-21T22:15:23Z

Is this ready for review?

almet · 2021-12-21T22:17:55Z

I believe it's alright from my point of view. As soon as we get the green light from @youegraillot, I believe we should merge :-)

youegraillot · 2021-12-21T22:55:10Z

Oups, I didn't notified you last week

I think I have answered all comments, light is green !

almet · 2021-12-21T23:00:44Z

Thanks a bunch!

youegraillot · 2021-12-21T23:03:46Z

My pleasure, I'll move on to #152 then

* proper import form (fix messy errors) * csv compatible import * cospend compatible import * localization (best effort) * refactoring * revert localization (best effort) * import return 400 on error * fix Person.query.get_by_ids calls * Bill explicit init parameters * fix tests * refacto tests with self.get_project * separate import tests * fix tests * csv import test case * fix import csv parsing * revert DestructiveActionProjectForm renaming * fix csv import test * fix error redirection on import * fix lint * import file input type hint * various fixes from review Co-authored-by: Youe Graillot <youe.graillot@gmail.com>

Youe Graillot added 5 commits November 28, 2021 11:29

proper import form (fix messy errors)

d24efac

csv compatible import

e7d991e

cospend compatible import

61df8aa

localization (best effort)

12f59f8

refactoring

ef3b9aa

youegraillot changed the title ~~CSV bills import~~ CSV bills import (cospend compatible) Nov 28, 2021

Glandos requested changes Nov 28, 2021

View reviewed changes

youegraillot added 7 commits December 12, 2021 17:12

revert localization (best effort)

1765c1b

import return 400 on error

97729a7

fix Person.query.get_by_ids calls

8d9cb82

Bill explicit init parameters

7a7d0a2

fix tests

186d42c

refacto tests with self.get_project

9352ad6

separate import tests

ef92181

youegraillot force-pushed the feature/import_csv branch 6 times, most recently from 00ac1b9 to 4be27b1 Compare December 13, 2021 00:47

fix tests

7d3d669

youegraillot force-pushed the feature/import_csv branch from 4be27b1 to 7d3d669 Compare December 13, 2021 00:48

youegraillot added 4 commits December 13, 2021 03:27

csv import test case

b38a1c1

fix import csv parsing

030fb37

revert DestructiveActionProjectForm renaming

64ce4cd

fix csv import test

99a78f8

youegraillot added 2 commits December 13, 2021 04:31

fix error redirection on import

946fa3e

fix lint

edbddab

youegraillot marked this pull request as ready for review December 13, 2021 10:09

import file input type hint

07f23ce

almet reviewed Dec 13, 2021

View reviewed changes

Glandos reviewed Dec 14, 2021

View reviewed changes

various fixes from review

fbd8c69

almet merged commit 747824a into spiral-project:master Dec 21, 2021

youegraillot deleted the feature/import_csv branch January 13, 2022 16:44

almet mentioned this pull request Jan 14, 2022

Docker -e ADMIN_PASSWORD hash is not generated (and queried) correctly #977

Closed

AlexanderSch90 mentioned this pull request Jan 14, 2022

Cospend CSV bills import not working anymore #980

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSV bills import (cospend compatible) #951

CSV bills import (cospend compatible) #951

youegraillot commented Nov 28, 2021 •

edited

Glandos left a comment

Glandos Nov 28, 2021

youegraillot Dec 13, 2021

almet commented Nov 28, 2021

youegraillot commented Nov 28, 2021 •

edited

almet commented Dec 10, 2021

youegraillot commented Dec 11, 2021

youegraillot commented Dec 13, 2021

almet left a comment

almet Dec 13, 2021

almet Dec 13, 2021

youegraillot Dec 14, 2021

almet Dec 13, 2021

almet Dec 13, 2021

youegraillot Dec 14, 2021

almet Dec 13, 2021

youegraillot Dec 14, 2021

youegraillot commented Dec 13, 2021

Glandos Dec 14, 2021

youegraillot Dec 14, 2021

Glandos commented Dec 14, 2021

youegraillot commented Dec 14, 2021

almet commented Dec 21, 2021

almet commented Dec 21, 2021

youegraillot commented Dec 21, 2021

almet commented Dec 21, 2021

youegraillot commented Dec 21, 2021

CSV bills import (cospend compatible) #951

CSV bills import (cospend compatible) #951

Conversation

youegraillot commented Nov 28, 2021 • edited

Glandos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

almet commented Nov 28, 2021

youegraillot commented Nov 28, 2021 • edited

almet commented Dec 10, 2021

youegraillot commented Dec 11, 2021

youegraillot commented Dec 13, 2021

almet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youegraillot commented Dec 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Glandos commented Dec 14, 2021

youegraillot commented Dec 14, 2021

almet commented Dec 21, 2021

almet commented Dec 21, 2021

youegraillot commented Dec 21, 2021

almet commented Dec 21, 2021

youegraillot commented Dec 21, 2021

youegraillot commented Nov 28, 2021 •

edited

youegraillot commented Nov 28, 2021 •

edited