Added initial support for reimbursement #215
Conversation
|
I like the idea, though I think it would need the approval/blessting of the TREO/Forstyrrelse. For modelling the data, I think it would be better to have the reference between the Payment and the (now refunded) Sale, rather than having two boolean flags. This way, if you want to query for refunded sales, you can simply check for non-null values, and the same for the negative Payments, which is the refund. |
This should definitely be possible given a sensible implementation, in my opinion. I've begun the discussion for the relevant parties. |
| return | ||
| sale.is_reimbursed = True | ||
| sale.save() | ||
| Payment.objects.create(amount=sale.price, member=sale.member, is_reimbursement=True) |
There was a problem hiding this comment.
We should add a LogEntry to the Payment object to note that it is a result of a refund.
There was a problem hiding this comment.
While I think this is a great idea, during implementation, I found out that LogEntry has a NOT NULL constraint on the user_id. So that pretty much rules it out.
https://github.com/django/django/blob/b9fe7f9294b1b4fc974c008adeb96e1375cdb0c6/django/contrib/admin/models.py#L45
An idea would be to make a "reimbursement user" and set that user as the person who did it. Buut that is kinda hacky.
Any ideas on how to overcome this?
There was a problem hiding this comment.
I would actually prefer if the person who did the reimbursement was referenced on the LogEntry. That allows for tracability of reimbursements. The Payment object to be reimbursed should also be referenced.
There was a problem hiding this comment.
Completely agree, but since stregsystemet has modeled it's own members seperate from django users, it does not have users and thus it seems impossible to use the LogEntry table since LogEntry has a NOT NULL constraint to users.
|
@VirtualSatai I thought about doing a foreign key to the refunded sale, but I believe that would make the list of sales slow. The double boolean solution may be simple, but since the information is highly local, I think this is still the best solution at the moment. If you still think otherwise, then please enlighten me. I cannot think of the usecase where it would be better to have a foreign key over the two booleans. Anyway, I have made 3 tests, testing the reimbursement feature and some changes. |
Codecov Report
@@ Coverage Diff @@
## next #215 +/- ##
==========================================
+ Coverage 82.42% 82.62% +0.20%
==========================================
Files 30 30
Lines 2247 2285 +38
Branches 157 158 +1
==========================================
+ Hits 1852 1888 +36
- Misses 370 371 +1
- Partials 25 26 +1
Continue to review full report at Codecov.
|
|
With the implementation of inventory management, it is key that we add +1 to the quantity of the reimbursed product, if the product quantity has a start date. |
I'm fairly certain that deleting a sale object makes quantity of given product increment, furthermore, the purchase price is returned to the purchaser. If that's the case, we don't need to do very much, except allow that action for the users within the grace period. |
If the deletion increments quantity, then there should be no conflict with the inventory system, assuming this also affects the |
This is an initial fix for #188.
This PR allows the user to refund his/her purchase by doing the following:
Modifies the sale to show
is_reimbursed.Creates a new payment for the amount that the sale was on. This payment is tagged with
is_reimbursement.The button for reimbursing the user is on the user information. The user only sees sales that have not been reimbursed.
Example:
Example of confirmation dialog
The button is only visible to the user if the sale was within the last 12 hours.
Unfortunately I had to allow the user to edit a sale. This may be unwanted - but I could not think of another way to do this.
The reason for modifying the sale is to allow the values to be filtered out of statistics.
Questions I had during making this:
Should the "Undo" button be an actual button or rather an anchor?
Should I have instead of creating a
Payment, instead saveRefundin a separate table?I hope for feedback.
Also I need tests. Lots of tests...