Skip to content
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

OPL-22: added services for Bill/BillItem, signal to save Bill created from calcrule #13

Merged
merged 9 commits into from Nov 26, 2021

Conversation

sniedzielski
Copy link
Contributor

@sniedzielski sniedzielski commented Nov 18, 2021

part of ticket: https://openimis.atlassian.net/browse/OPL-22

related to the repo (branch): https://github.com/openimis/openimis-be-calcrule-third_party_payment_py/tree/feature/OPL-22

Changes:

  • added base services for Bill/BillItem and validators
  • pre/post method to process creating bill in calculation rules with using added services in that PR

@sniedzielski
Copy link
Contributor Author

I will add PR also for this https://github.com/openimis/openimis-be-calcrule-third_party_payment_py/tree/feature/OPL-22 when I write tests for calcrule converter (from Claims Queryset to Bill) etc.

@sniedzielski sniedzielski marked this pull request as ready for review November 18, 2021 14:22
Copy link
Contributor

@dborowiecki dborowiecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I left some comments.

return self._evaluate_generic_types(bill_data)

def bill_validate_items(self, bill: Bill):
# TODO: Implement after calculation rules available
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it still to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think when this 1st rule regarding creating Bill is ready, then we can implement this validation.

pass

def bill_match_items(self, bill: Bill) \
-> Dict[str, Union[BillItemStatus, Dict[BillItem, List[BillItemStatus]]]]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just a note, but Union is for X | Y. In docstring you're more specific; we're always returning ''subject' with BillItemStatus and 'line_items' with bill items and statuses. It'd might be better idea to use TypedDict for this hint so it'll match docstring information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I will check this. Thx for this note.

if instance.__class__.__name__ == "QuerySet":
queryset_model = instance.model
if queryset_model.__name__ == "Claim":
claim = instance.first()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that query will return >1 entry? Maybe it'll be better to use .get() and raise exception if more than one entry is returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Could be more than 1. But this is about to check if claim is in BillItem. I can explain this details what I intended to do tomorrow

@sniedzielski
Copy link
Contributor Author

Looks good, I left some comments.

Ok. Anyway I need to fix what Dragos mentioned to me on Skype. I will inform @dborowiecki you about this changes what I need to do.

@sniedzielski sniedzielski marked this pull request as ready for review November 24, 2021 09:15
@dragos-dobre dragos-dobre merged commit adcc1c5 into develop Nov 26, 2021
@dragos-dobre dragos-dobre deleted the feature/OPL-22 branch November 26, 2021 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants