Reimbursement of latest bought items #424
Conversation
TheRumle
changed the title
jonasKjellerup
left a comment
jonasKjellerup
left a comment
There was a problem hiding this comment.
Generally looks fine. As for your discussion points:
- I think you should as a minimum stick to using the
MoneyTransactionentity to manage the balance updates. This should also make the balance changes visible in the admin panel. Beyond that I am not super familiar with the admin panel so I don't have much to add there. - I think this is fine.
- I am not really sure what you are getting at here, could you elaborate a bit?
| if not sale: | ||
| raise SaleNotFoundError() | ||
| product = sale.product | ||
| product.quantity = product.quantity + 1 |
There was a problem hiding this comment.
You're missing a .save() on product after updating the quantity.
Unless, the Django ORM does it as part of saving the new reimbursement,
in which case you don't need sale.member.save() on line 21.
stregsystem/business/reimburse.py
Outdated
| product = sale.product | ||
| product.quantity = product.quantity + 1 | ||
|
|
||
| sale.member.balance = sale.member.balance + sale.price |
There was a problem hiding this comment.
I think this should be done through a MoneyTransaction (possibly a new subtype ReimbursementTransaction similar to how it is done for PayTransaction) such that it is recorded in a manner consistent with other balance changes.
| ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), | ||
| ('amount', models.IntegerField()), | ||
| ('member', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='stregsystem.Member')), | ||
| ('product', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='stregsystem.Product')), |
There was a problem hiding this comment.
I don't think it is a good idea for the reimbursement to be removed when a product is deleted (however unlikely the scenario may be). It gives an incorrect view of the customers history.
SET_NULL seems more appropriate.
There was a problem hiding this comment.
With that said, it is consistent with how sales are handled, so I guess it is fine.
krestenlaust
left a comment
krestenlaust
left a comment
There was a problem hiding this comment.
Without having looked at the source code, I experience the following runtime error, whenever I try to refund. I'm not sure if it's supposed to work yet
3 participants
As seen in #418, #215 and #188 we would like to be able to re-imburse items. Here is my suggestion on how one could do it. Heavily inspired by @joandrsn #215 PR.
What makes this BL tedious is that all sale information must be 'preserved' - however, if we just put a field into the Sale entity we have create a filter on all existing logic - for instance, we don't want to make people 'coffee master' by considering reimbursed coffee. Therefore, making a 'is_reimbursed' field seems like a bad idea to me. I have instead created is_reimbursed to establish when it can occur.
I decided to create the Reimbursement class, and use it to keep track of who reimburses what, and for how much. I see a couple of critical things about my approach and would like to discuss them