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
OFS-133: "create contract" service (including OFS-144) #5
Conversation
This pull request introduces 1 alert and fixes 3 when merging ed7ad18 into b493cf2 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 1 alert and fixes 3 when merging c8d98ec into b493cf2 - view on LGTM.com new alerts:
fixed alerts:
|
contract/services.py
Outdated
|
||
def __services_update_from_ph_contract_valuation(self, policy_holder, contract_id): | ||
cd = ContractDetails(user=self.user) | ||
result_ph_insuree = cd.update_from_ph_insuree(contract_details={ |
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.
this "update_from_ph_insuree" should be only in create not in this function because with this implementation the user input will be deleted every time you update the value
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.
Ok, I will move this into "create".
"contract_details_id": contract_details["id"], | ||
"contribution_plan_id": str(cp.id), | ||
"policy_id": contract_details["policy_id"] | ||
} |
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.
we will need to fetch the calculation params that eligible for tha CP , I think it should be a function from the calculation module, please add a # TODO
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.
Ok, clear. I've just added # TODO in code in that place.
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.
And yes. I will use here this calculations params that eligible for that CP assigned to that insuree etc. And the function from the calculation module will be called here so as to count the valiue of the amount.
This pull request introduces 1 alert and fixes 3 when merging 5931e8e into b493cf2 - view on LGTM.com new alerts:
fixed alerts:
|
@delcroip I've changed code according to your clues from review. Please check it again. |
This pull request introduces 1 alert and fixes 3 when merging 2b8a2a7 into b493cf2 - view on LGTM.com new alerts:
fixed alerts:
|
In that pull request I want to merge service 'create contract' with including another two services from "OFS-144 - Service - update from Policy Holder Insuree and Service - Contract Valuation". (#6)
That service "create contract" will be used in [OFS-126 - create muation - create contract] as a part of mutation (wrapped service in mutation)
Moreover I added two unit tests: one for creating contract without assigned PH and the second one with assigned PH with some PHI and CP etc.
In "Service - Contract Valuation" the amount for the insuree is not counted (only hardcoded) - I assigned default temporary value until calculation module be developed (which allow to count amount for the employee based on CP etc)