-
Notifications
You must be signed in to change notification settings - Fork 21
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
Handle different payments with same transaction id #124
base: master
Are you sure you want to change the base?
Conversation
Any idea why tests fail? I couldn't see the obvious reason why the helper wouldn't work in this test but works in others. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least the test needs some fixes. I'd like to hear some reasoning for the unique_together set.
@@ -179,7 +179,10 @@ def _get_row(self, row): | |||
|
|||
def row_to_payment(row): | |||
try: | |||
p = Payment.objects.get(transaction_id__exact=row['transaction']) | |||
# Search first for from already imported rows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Search first
forfrom already imported rows
@@ -179,7 +179,10 @@ def _get_row(self, row): | |||
|
|||
def row_to_payment(row): | |||
try: | |||
p = Payment.objects.get(transaction_id__exact=row['transaction']) | |||
# Search first for from already imported rows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a comment here csvbills#L112-L122 BillDictReader.next() that row_to_payment is sensitive to changes in the data cleaning done in the next function?
Meaning if something was imported with old version, this would fail in later version as the normalization will be different.
membership/models.py
Outdated
@@ -933,7 +933,7 @@ class Meta: | |||
permissions = ( | |||
("can_import_payments", "Can import payment data"), | |||
) | |||
|
|||
unique_together = ('reference_number', 'transaction_id', 'message') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the original issue description, I'd have thought it'd be better to unique transaction_id and payment_day, not reference number and message?
I don't know why we have payment_day is a DateTimeField, and not just a date, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is true, fixed in newest commit.
error = "The sample file should have contained only one payment" | ||
self.assertEqual(Payment.objects.count(), 1, error) | ||
with open(data_file("procountor-csv-single.csv"), 'r') as f: | ||
process_procountor_csv(f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for doing this twice on the same input file? Should this be two tests?
335ac6b
to
39a0690
Compare
Codecov Report
@@ Coverage Diff @@
## master #124 +/- ##
==========================================
+ Coverage 74.67% 74.83% +0.15%
==========================================
Files 53 54 +1
Lines 5173 5205 +32
==========================================
+ Hits 3863 3895 +32
Misses 1310 1310
Continue to review full report at Codecov.
|
Fix situation where two different payments from different years have same transaction id but different reference numbers and/or messages.
CSV exports from Procountor have transaction id without year thus making duplicate transaction ids from different years possible.
Also fixes transaction id field length (ref. finanssiala.fi )