Split payments #533
Split payments #533
Conversation
bogdanpetrea
force-pushed
the
split-payments
branch
from
a47f324 to
c59294a
Compare
silver/models/documents/base.py
Outdated
|
|
||
| # Transition related document too, if needed | ||
| if document.related_document and document.state != document.related_document.state: | ||
| state_transition_map = { |
There was a problem hiding this comment.
Seems like a useful business logic block that can be extracted in BillingDocumentBase model
There was a problem hiding this comment.
It could be...
silver/models/documents/base.py
Outdated
| transition_name = state_transition_map[document.state] | ||
|
|
||
| bound_transition_method = getattr(document.related_document, transition_name) | ||
| bound_transition_method() |
There was a problem hiding this comment.
This will also save the document.related_document instance?
There was a problem hiding this comment.
Maybe we can think to move this logic into a reconcile loop. (not necessarily now)
There was a problem hiding this comment.
- Yes, it will also save the related document, because the
savemethod gets automatically called after a successful transition. - I'd rather have this happen synchronously. Having a draft proforma and an issued invoice, even for a short period of time, doesn't sound good.
silver/models/documents/base.py
Outdated
|
|
||
| document = instance | ||
|
|
||
| if hasattr(document, '.recently_transitioned'): |
There was a problem hiding this comment.
Negating this will make the entire logic a little easier to read.
silver/models/documents/base.py
Outdated
| create_transaction_for_document(document) | ||
|
|
||
| # Generate a PDF | ||
| document.set_pdf_for_generation() |
There was a problem hiding this comment.
👍
I would name it mark_for_generation (in this way you will leave some options to have more renders: pdf, html, xml. Maybe, in a near feature, each invoice will have a well know format, xml based, making them easier to import/export (I think that the Czech Republic has this system implemented)).
| return self.pdf_file.url if self.pdf_file else None | ||
|
|
||
| def generate(self, template, context, upload=True): | ||
| pdf_file_object = HttpResponse(content_type='application/pdf') |
There was a problem hiding this comment.
Why do you need to return an HttpResponse from model.
I don't think that models should be coupled with any type of http response.
There was a problem hiding this comment.
That HttpResponse will be used as a file_object for the generate_pdf_template_object method.
| '(invoice or proforma).' | ||
| ) | ||
|
|
||
| lock = redis.lock(lock_key, timeout=5) # 5 seconds should be more than enough to save |
| setattr(self, 'previous_instance', previous_instance) | ||
|
|
||
| def _clean_and_save(): | ||
| if not getattr(self, '.cleaned', False): |
There was a problem hiding this comment.
It looks like a method.
Do you use the closure, or you just want to reuse the code?
There was a problem hiding this comment.
I just want to reuse the code, but I don't want to expose it, because calling that method would evict the locking.
| lock = redis.lock(lock_key, timeout=5) # 5 seconds should be more than enough to save | ||
|
|
||
| if not lock.acquire(blocking_timeout=5): | ||
| raise ValidationError("Couldn't create a transaction for the given document.") |
There was a problem hiding this comment.
It doesn't look like a ValidationError.
Also, maybe some details regarding why the transaction can't be created will help debugging.
There was a problem hiding this comment.
Well it's a validation step that doesn't pass. How about: raise ValidationError("A transaction for the same billing document is currently being created.")?
|
|
||
| lock = redis.lock(lock_key, timeout=5) # 5 seconds should be more than enough to save | ||
|
|
||
| if not lock.acquire(blocking_timeout=5): |
bogdanpetrea
force-pushed
the
split-payments
branch
from
3d191ba to
a2eb2a9
Compare
Depends on #532.