-
Notifications
You must be signed in to change notification settings - Fork 9
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
MVJ-550 override_receivable_type in Rent (API) #772
MVJ-550 override_receivable_type in Rent (API) #772
Conversation
3dbb4ec
to
151fb8e
Compare
Still requires customer blessing to use this Rent version instead of the ContractRent, but this is the best version of the logic we have right now. |
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.
It looks good so I am approving, however two things could be improved if you agree:
validate_override_receivable_type_value()
could be refactored slightly to simplify itis_akv_or_kuva_service_unit_id()
might be possible to replace by adding an attribute on ServiceUnit model for enablding and disabling override receivable type on rents.
b745cae
to
9e4c041
Compare
The CI build fails due to some migration 0082 that was present at some time in my feature branch, but has since been removed. I don't think there's even a commit for that anymore here. I wonder how to fix that in this pipeline, besides creating a new PR which would lose the comment history EDIT: fixed by reverting formatting changes to service unit model help text |
It (still) seems good, we could see if the validation logic can be simplified. The logic to validate seems quite complex, and if there is room for improvement it would make it easier to understand later on |
1984213
to
d772e49
Compare
This implements the API part of override receivable type, where it is a property of Rent, instead of ContractRent like the other implementation in #771