Transition: set now as transaction date #140
Conversation
|
It seems the tests were wrong too. |
| @@ -172,7 +172,7 @@ def _date_fields2datetime(self, kwargs): | |||
| def before(self, loan, initial_loan, **kwargs): | |||
| """Validate input, evaluate conditions and raise if failed.""" | |||
| loan.update(kwargs) | |||
| loan.setdefault("transaction_date", arrow.utcnow()) | |||
| loan["transaction_date"] = arrow.utcnow() | |||
There was a problem hiding this comment.
Does this mean, we can not pass a pre-defined loan.transaction_date?
There was a problem hiding this comment.
There is a use case important here:
when a library system is down and
transactions were recorded manually on excel file or whatever
we should be able to load these transactions with the correct transaction_date when the system is back up
There was a problem hiding this comment.
OK I see your point, and you're right. I think what we are trying to fix is the fact that whenever a loan is updated from one state to another the transaction_date will not change because there is already a value there. So normally if we move the statement loan["transaction_date"] = arrow.utcnow() before loan.update(kwargs) that should solve both problems.
There was a problem hiding this comment.
@BadrAly the idea behind this change was to have the transaction_date set when it the transaction in the system really happened. I understand that the system might be down, but when it is back up, and the transactions are being loaded, they are happening from the system point of view on the "load" time (now, not in the past). So I see the transaction_date as a timestamp from the system point of view, not the librarian point of view.
There was a problem hiding this comment.
@kprzerwa @FlorianCassayre Thanks for your answers, I am checking with @pronguen and @iGormilhit on this.
There was a problem hiding this comment.
@kprzerwa Could the created field be viewed as this system timestamp? In a case of circulation backup, when a circulation transaction occurs, some dates will be calculated with regards to the circulation policy (ie a due date, or an overdue…) that should apply. When this loans will be loaded, setting the transaction date to the moment of the loads may cause issues. From my understanding, the transaction date is in fact something important from the librarian and the patron point of view.
At least if I understand correctly the conversation.
There was a problem hiding this comment.
@iGormilhit thank you for your answer.
I understand the usage of the transaction_date is a matter of interpretation, but I see your point with the created date.
When this loans will be loaded, setting the transaction date to the moment of the loads may cause issues.
Could you describe in more details what kind of issues do you mean?
We introduced this change because without it the REST API requires transaction_date to be present in the loan action payload - otherwise the transaction_date will not be updated when the transaction happens. In this case I can see that it will influence the way you use the field, so we will solve the problem in the marshmallow loaders.
Closes inveniosoftware/react-invenio-app-ils#294
Removed
test_override_transaction_datesince we are not supposed to override the transaction date.