Closed JustSamuel closed 3 months ago
github-actions[bot]
commented
3 months ago Commit: c9fa5f1 Base: develop@8e8fdb0
| Type | Base | This PR |
|---|---|---|
| Total Statements Coverage |  88.06% |
88.02% (-0.04%) |
| Total Branches Coverage | 85.77% |
85.51% (-0.26%) |
| Total Functions Coverage | 89.52% |
89.56% (+0.04%) |
| Total Lines Coverage | 88.04% |
88% (-0.04%) |
| File | Statements | Branches | Functions | Lines |
|---|
Yoronex
commented
3 months ago I am not really a fan of this implementation... You have added the manager parameter to every single function and you now have to wrap every invoice function you call in a EntityManager helper function. In some methods, like TransactionService line 533, you even create such a manager manually if it is not given. Why not just use the fact that the (invoice) service is a class and add this code snippet:
50%
33.33% private manager: EntityManager;
constructor(manager?: EntityManager) {
this.manager = manager ? manager : AppDataSource.manager;
}If you then make all service functions no longer static, you only have to initialize the service (optionally with a manager) and no longer need to wrap everything.
What do you think? This is also one of the reasons why I wished to implement the AppDataSource correctly, because now stuff like this is possible.
Description
Fixes a bug where deleting a credit invoice would cause two transfers to be created. Also wraps most InvoiceService methods in a entity manager to make sure that errors are dealt with more gracefully.
Also, I found that the setup where you create a single Invoice for a user who has bought stuff at multiple sellers is super untransparent to the user and to the sellers and to the BAC. I have now made it throw a 501 Not Implemented until I fix the real issue.
The solution will be that we take @CodeNamedRobin his refactor of the InvoiceService which can "preview" and highligh that you are trying to invoice something for multiple sellers. The solution will be to make an invoice per seller, a bit like how bol.com does it.
Following my comment here I have decided to throw a 501 in all "edge" cases that will later be fixed but to make sure no more errors occur.
Related issues/external references
249
Types of changes