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

Fix/credit invoice deletion #250

Merged
merged 14 commits into from
Aug 13, 2024
Merged

Fix/credit invoice deletion #250

merged 14 commits into from
Aug 13, 2024

Conversation

JustSamuel
Copy link
Contributor

@JustSamuel JustSamuel commented Aug 10, 2024

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

  • Bug fix (non-breaking change which fixes an issue)

@JustSamuel JustSamuel changed the title Fix/invoice deletion Fix/credit invoice deletion Aug 10, 2024
Copy link

github-actions bot commented Aug 11, 2024

SudoSOS Coverage Report

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%)
Details (changed files)
FileStatementsBranchesFunctionsLines
Details (all files)
FileStatementsBranchesFunctionsLines
/src/index.ts 83.43% 50% 33.33% 83.76%
/src/authentication/token-handler.ts 100% 100% 100% 100%
/src/controller/authentication-controller.ts 85.71% 94.73% 96% 86.14%
/src/controller/authentication-secure-controller.ts 88.23% 100% 100% 88.23%
/src/controller/balance-controller.ts 78.04% 50% 100% 78.04%
/src/controller/banner-controller.ts 82.52% 95.45% 100% 82.52%
/src/controller/base-controller.ts 100% 100% 100% 100%
/src/controller/container-controller.ts 85.96% 87.5% 100% 85.45%
/src/controller/debtor-controller.ts 84.17% 69.23% 100% 87.87%
/src/controller/event-controller.ts 82.66% 94.44% 100% 82.66%
/src/controller/event-shift-controller.ts 75.82% 93.33% 100% 75.82%
/src/controller/invoice-controller.ts 85.88% 91.66% 100% 85.71%
/src/controller/payout-request-controller.ts 71.29% 82.14% 84.61% 71.29%
/src/controller/point-of-sale-controller.ts 83.01% 84.37% 100% 83%
/src/controller/product-category-controller.ts 82.45% 100% 100% 82.45%
/src/controller/product-controller.ts 85.6% 90% 100% 86.77%
/src/controller/rbac-controller.ts 87.58% 93.75% 100% 87.32%
/src/controller/root-controller.ts 82.14% 100% 100% 82.14%
/src/controller/simple-file-controller.ts 11.62% 0% 0% 11.62%
/src/controller/stripe-controller.ts 92.59% 100% 100% 92.59%
/src/controller/stripe-webhook-controller.ts 100% 100% 100% 100%
/src/controller/test-controller.ts 33.33% 100% 0% 33.33%
/src/controller/transaction-controller.ts 85.14% 96.96% 100% 84.69%
/src/controller/transfer-controller.ts 84.21% 94.73% 100% 83.63%
/src/controller/user-controller.ts 83.19% 94.11% 100% 83.05%
/src/controller/vat-group-controller.ts 87.8% 100% 100% 87.8%
/src/controller/voucher-group-controller.ts 79.41% 83.33% 100% 79.41%
/src/controller/write-off-controller.ts 85.45% 100% 100% 85.45%
/src/controller/request/file-request.ts 0% 0% 0% 0%
/src/controller/request/validators/container-request-spec.ts 100% 100% 100% 100%
/src/controller/request/validators/duration-spec.ts 66.66% 50% 50% 64.28%
/src/controller/request/validators/general-validators.ts 92.3% 83.33% 88.88% 89.74%
/src/controller/request/validators/invoice-request-spec.ts 93.44% 85.71% 100% 97.91%
/src/controller/request/validators/point-of-sale-request-spec.ts 95% 75% 100% 94.11%
/src/controller/request/validators/product-request-spec.ts 90.32% 72.72% 100% 90%
/src/controller/request/validators/rbac-request-spec.ts 100% 100% 100% 100%
/src/controller/request/validators/string-spec.ts 100% 100% 100% 100%
/src/controller/request/validators/update-local-request-spec.ts 100% 100% 100% 100%
/src/controller/request/validators/update-nfc-request-spec.ts 100% 100% 100% 100%
/src/controller/request/validators/update-pin-request-spec.ts 100% 100% 100% 100%
/src/controller/request/validators/user-request-spec.ts 86.95% 66.66% 85.71% 88.88%
/src/controller/request/validators/validation-errors.ts 88.6% 100% 65.38% 100%
/src/controller/response/dinero.ts 0% 0% 0% 0%
/src/database/database.ts 96.29% 75% 33.33% 97.46%
/src/database/schema.ts 92% 50% 50% 95.83%
/src/entity/banner.ts 100% 100% 100% 100%
/src/entity/base-entity-without-id.ts 100% 100% 100% 100%
/src/entity/base-entity.ts 100% 100% 100% 100%
/src/entity/server-setting.ts 84.61% 50% 100% 100%
/src/entity/vat-group.ts 100% 100% 100% 100%
/src/entity/authenticator/authentication-method.ts 100% 100% 100% 100%
/src/entity/authenticator/ean-authenticator.ts 100% 100% 100% 100%
/src/entity/authenticator/hash-based-authentication-method.ts 100% 100% 100% 100%
/src/entity/authenticator/key-authenticator.ts 100% 100% 100% 100%
/src/entity/authenticator/ldap-authenticator.ts 100% 100% 100% 100%
/src/entity/authenticator/local-authenticator.ts 100% 100% 100% 100%
/src/entity/authenticator/member-authenticator.ts 100% 100% 100% 100%
/src/entity/authenticator/nfc-authenticator.ts 100% 100% 100% 100%
/src/entity/authenticator/pin-authenticator.ts 100% 100% 100% 100%
/src/entity/authenticator/reset-token.ts 100% 100% 100% 100%
/src/entity/container/container-revision.ts 95.23% 100% 83.33% 92.85%
/src/entity/container/container.ts 100% 100% 100% 100%
/src/entity/deposit/stripe-deposit-status.ts 100% 100% 100% 100%
/src/entity/deposit/stripe-deposit.ts 100% 100% 100% 100%
/src/entity/event/event-shift-answer.ts 100% 100% 100% 100%
/src/entity/event/event-shift.ts 100% 100% 100% 100%
/src/entity/event/event.ts 100% 100% 100% 100%
/src/entity/file/banner-image.ts 100% 100% 100% 100%
/src/entity/file/base-file.ts 100% 100% 100% 100%
/src/entity/file/invoice-pdf.ts 100% 100% 100% 100%
/src/entity/file/payout-request-pdf.ts 100% 100% 100% 100%
/src/entity/file/pdf-file.ts 100% 100% 100% 100%
/src/entity/file/product-image.ts 100% 100% 100% 100%
/src/entity/fine/fine.ts 100% 100% 100% 100%
/src/entity/fine/fineHandoutEvent.ts 100% 100% 100% 100%
/src/entity/fine/userFineGroup.ts 100% 100% 100% 100%
/src/entity/invoices/invoice-entry.ts 100% 100% 100% 100%
/src/entity/invoices/invoice-status.ts 100% 100% 100% 100%
/src/entity/invoices/invoice.ts 100% 100% 100% 100%
/src/entity/point-of-sale/point-of-sale-revision.ts 94.44% 100% 75% 92.3%
/src/entity/point-of-sale/point-of-sale.ts 100% 100% 100% 100%
/src/entity/point-of-sale/product-ordering.ts 100% 100% 100% 100%
/src/entity/product/product-category.ts 100% 100% 100% 100%
/src/entity/product/product-revision.ts 96.55% 100% 83.33% 95.45%
/src/entity/product/product.ts 100% 100% 100% 100%
/src/entity/rbac/assigned-role.ts 100% 100% 100% 100%
/src/entity/rbac/permission.ts 100% 100% 100% 100%
/src/entity/rbac/role-user-type.ts 100% 100% 100% 100%
/src/entity/rbac/role.ts 100% 100% 100% 100%
/src/entity/transactions/balance.ts 100% 100% 100% 100%
/src/entity/transactions/flagged-transaction.ts 100% 100% 100% 100%
/src/entity/transactions/payout-request-status.ts 100% 100% 100% 100%
/src/entity/transactions/payout-request.ts 100% 100% 100% 100%
/src/entity/transactions/sub-transaction-row.ts 100% 100% 100% 100%
/src/entity/transactions/sub-transaction.ts 100% 100% 100% 100%
/src/entity/transactions/transaction.ts 100% 100% 100% 100%
/src/entity/transactions/transfer.ts 100% 100% 100% 100%
/src/entity/transactions/write-off.ts 100% 100% 100% 100%
/src/entity/transformer/dinero-transformer.ts 100% 100% 100% 100%
/src/entity/user/invoice-user.ts 100% 100% 100% 100%
/src/entity/user/local-user.ts 100% 100% 100% 100%
/src/entity/user/user-voucher-group.ts 100% 100% 100% 100%
/src/entity/user/user.ts 97.77% 75% 100% 100%
/src/entity/user/voucher-group.ts 100% 100% 100% 100%
/src/errors/index.ts 100% 100% 0% 100%
/src/errors/not-implemented-error.ts 20% 0% 0% 20%
/src/files/initialize.ts 100% 50% 100% 100%
/src/files/response.ts 28.57% 100% 0% 28.57%
/src/files/storage/disk-storage.ts 82.14% 83.33% 60% 82.14%
/src/files/storage/file-storage.ts 100% 100% 100% 100%
/src/files/storage/index.ts 100% 100% 50% 100%
/src/files/storage/locations.ts 100% 100% 100% 100%
/src/gewis/gewis.ts 81.25% 44.44% 75% 89.65%
/src/gewis/controller/gewis-authentication-controller.ts 87.5% 75% 100% 87.5%
/src/gewis/database/seed.ts 100% 100% 100% 100%
/src/gewis/entity/gewis-user.ts 100% 100% 100% 100%
/src/gewis/helpers/gewis-helper.ts 100% 90% 100% 100%
/src/gewis/service/gewisdb-service.ts 87.67% 91.3% 80% 89.39%
/src/helpers/ad.ts 90.9% 100% 100% 90.9%
/src/helpers/bindings.ts 100% 100% 100% 100%
/src/helpers/database.ts 100% 100% 100% 100%
/src/helpers/hash.ts 100% 100% 100% 100%
/src/helpers/ordering.ts 90.9% 83.33% 100% 90%
/src/helpers/pagination.ts 100% 85.71% 100% 100%
/src/helpers/pdf.ts 100% 50% 100% 100%
/src/helpers/query-filter.ts 96.87% 95% 85.71% 96.55%
/src/helpers/raw-body.ts 100% 100% 100% 100%
/src/helpers/revision-to-response.ts 91.3% 66.66% 88.88% 95.23%
/src/helpers/specification-validation.ts 97.29% 100% 88.88% 96.87%
/src/helpers/timestamps.ts 58.82% 42.85% 66.66% 56.25%
/src/helpers/token-helper.ts 100% 100% 100% 100%
/src/helpers/transaction-mapper.ts 95.89% 100% 88% 95.58%
/src/helpers/validators.ts 94% 91.66% 100% 97.4%
/src/mailer/index.ts 100% 100% 100% 100%
/src/mailer/mail-message.ts 100% 100% 100% 100%
/src/mailer/mailer.ts 100% 100% 100% 100%
/src/mailer/transporter.ts 100% 100% 100% 100%
/src/mailer/messages/changed-pin.ts 85.71% 100% 0% 85.71%
/src/mailer/messages/forgot-event-planning.ts 76.92% 100% 57.14% 76.92%
/src/mailer/messages/hello-world.ts 100% 100% 100% 100%
/src/mailer/messages/index.ts 100% 100% 100% 100%
/src/mailer/messages/mail-content-builder.ts 81.81% 56.25% 100% 81.81%
/src/mailer/messages/membership-expiry-notification.ts 86.66% 50% 66.66% 86.66%
/src/mailer/messages/password-reset.ts 33.33% 0% 0% 33.33%
/src/mailer/messages/user-debt-notification.ts 85.71% 50% 60% 85.71%
/src/mailer/messages/user-got-fined.ts 76.92% 100% 57.14% 76.92%
/src/mailer/messages/user-will-get-fined.ts 70.37% 50% 60% 70.37%
/src/mailer/messages/welcome-to-sudosos.ts 85.71% 50% 60% 85.71%
/src/mailer/messages/welcome-with-reset.ts 85.71% 50% 60% 85.71%
/src/mailer/template/mail-body-generator.ts 91.66% 83.33% 100% 91.66%
/src/middleware/policy-middleware.ts 100% 100% 100% 100%
/src/middleware/request-validator-middleware.ts 92.3% 83.33% 100% 100%
/src/middleware/restriction-middleware.ts 93.75% 92.85% 75% 100%
/src/middleware/token-middleware.ts 100% 100% 100% 100%
/src/migrations/1707251162194-invoice-refactor.ts 5.71% 0% 0% 6.06%
/src/migrations/1720608140757-soft-deletes.ts 25% 100% 0% 25%
/src/migrations/1720610649657-payout-request-pdf.ts 12.5% 100% 0% 14.28%
/src/migrations/1720624912260-database-rbac.ts 13.72% 0% 4.76% 14.43%
/src/migrations/1721916495084-transfers-vat.ts 16.66% 0% 0% 18.18%
/src/migrations/1722004753128-write-offs.ts 23.8% 0% 0% 26.31%
/src/migrations/1722022351000-pos-cashiers.ts 33.33% 100% 0% 33.33%
/src/migrations/1722083254200-server-settings.ts 50% 100% 0% 50%
/src/migrations/1722084520361-pos-users.ts 21.05% 100% 0% 22.22%
/src/migrations/1722118077157-invoice-rework.ts 13.33% 0% 0% 14.28%
/src/rbac/default-roles.ts 100% 100% 100% 100%
/src/rbac/role-manager.ts 94% 82.35% 100% 97.61%
/src/server-settings/server-settings-store.ts 97.82% 92.85% 100% 100%
/src/server-settings/setting-defaults.ts 100% 100% 100% 100%
/src/service/ad-service.ts 86.31% 52.94% 90.32% 89.33%
/src/service/authentication-service.ts 92.98% 82.85% 100% 92.66%
/src/service/balance-service.ts 98.21% 91.04% 100% 98%
/src/service/banner-service.ts 95.23% 86.95% 100% 95.12%
/src/service/container-service.ts 97% 89.74% 100% 100%
/src/service/debtor-service.ts 98.3% 89.65% 100% 100%
/src/service/event-service.ts 100% 99% 100% 100%
/src/service/file-service.ts 96.34% 80.64% 100% 97.43%
/src/service/invoice-pdf-service.ts 100% 100% 100% 100%
/src/service/invoice-service.ts 94.97% 79.74% 96.36% 96.98%
/src/service/payout-request-pdf-service.ts 100% 100% 100% 100%
/src/service/payout-request-service.ts 98.03% 96.72% 95% 97.87%
/src/service/point-of-sale-service.ts 97.22% 88.46% 100% 100%
/src/service/product-category-service.ts 100% 100% 100% 100%
/src/service/product-service.ts 94.38% 80% 95.23% 94.8%
/src/service/rbac-service.ts 96.34% 91.66% 100% 100%
/src/service/report-pdf-service.ts 100% 100% 100% 100%
/src/service/stripe-service.ts 98.52% 100% 100% 98.33%
/src/service/transaction-service.ts 96.99% 88.07% 100% 96.71%
/src/service/transfer-service.ts 100% 97.5% 100% 100%
/src/service/user-service.ts 90.08% 61.9% 84.61% 93.51%
/src/service/vat-group-service.ts 97.29% 76.19% 92.85% 96.87%
/src/service/voucher-group-service.ts 100% 93.1% 100% 100%
/src/service/write-off-service.ts 100% 80% 100% 100%
/src/start/swagger.ts 82.5% 58.82% 66.66% 82.05%
/src/subscriber/index.ts 100% 100% 100% 100%
/src/subscriber/invoice-status-subscriber.ts 100% 50% 100% 100%
/src/subscriber/transaction-subscriber.ts 91.17% 80.76% 66.66% 92.85%
/src/subscriber/transfer-subscriber.ts 100% 87.5% 100% 100%

@Yoronex
Copy link
Member

Yoronex commented Aug 12, 2024

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:

  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.

src/controller/invoice-controller.ts Outdated Show resolved Hide resolved
src/controller/invoice-controller.ts Outdated Show resolved Hide resolved
src/controller/invoice-controller.ts Outdated Show resolved Hide resolved
src/helpers/errors.ts Outdated Show resolved Hide resolved
src/service/stripe-service.ts Outdated Show resolved Hide resolved
src/service/write-off-service.ts Show resolved Hide resolved
test/unit/controller/invoice-controller.ts Outdated Show resolved Hide resolved
test/unit/service/debtor-service.ts Outdated Show resolved Hide resolved
test/unit/service/invoice-service.ts Outdated Show resolved Hide resolved
test/unit/service/stripe-service.ts Outdated Show resolved Hide resolved
src/service/stripe-service.ts Outdated Show resolved Hide resolved
src/service/debtor-service.ts Show resolved Hide resolved
src/service/invoice-service.ts Outdated Show resolved Hide resolved
test/unit/service/invoice-service.ts Outdated Show resolved Hide resolved
test/unit/service/invoice-service.ts Outdated Show resolved Hide resolved
test/unit/service/invoice-service.ts Outdated Show resolved Hide resolved
test/unit/service/invoice-service.ts Outdated Show resolved Hide resolved
test/unit/service/invoice-service.ts Outdated Show resolved Hide resolved
test/unit/service/invoice-service.ts Outdated Show resolved Hide resolved
test/unit/service/invoice-service.ts Outdated Show resolved Hide resolved
@JustSamuel JustSamuel merged commit fe8be8a into develop Aug 13, 2024
6 checks passed
@JustSamuel JustSamuel deleted the fix/invoice-deletion branch August 13, 2024 10:29
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.

2 participants