feat(anrok): make api calls async in dedicated service #3002
Conversation
lovrocolic
force-pushed
the
feat-anrok-async
branch
from
298856d to
02294af
Compare
| invoice.save! | ||
| after_commit { Invoices::ProviderTaxes::PullTaxesAndApplyJob.perform_later(invoice:) } | ||
|
|
||
| return result.unknown_tax_failure!(code: 'tax_error', message: 'unknown taxes') |
There was a problem hiding this comment.
do we always return a failure if customer is on anrok? Why?
There was a problem hiding this comment.
This specific error is returned so that in calling class we can identify this case and don't raise error
which would rollback DB transaction. The goal is to store such an invoice which is pending and the background job will process taxes and move the invoice to final status.
There was a problem hiding this comment.
but then this is the expected behavior not a failure, isn't it?
There was a problem hiding this comment.
Invoice is not valid without taxes so that's why I wanted to mark this case with this failure. Probably it is just style preference. Other option could be that I check wherever I need if invoice.tax_status = 'pending' or something similar:)
| if customer_provider_taxation? | ||
| taxes_result = Integrations::Aggregator::Taxes::Invoices::CreateService.call(invoice:, fees: invoice.fees) | ||
| totals_result = Invoices::ComputeTaxesAndTotalsService.call(invoice:) | ||
| return totals_result if !totals_result.success? && totals_result.error.is_a?(BaseService::UnknownTaxFailure) |
There was a problem hiding this comment.
why we don't raise the error if it is an UnknownTaxFailure?
Maybe the naming of the failure is not the best as I would expect to raise unknown failures more than known ones...
There was a problem hiding this comment.
Maybe naming is confusing, but I couldn't find better one:) The idea is:
- if unknown error happens, we don't want to raise error since it would rollback all DB changes
- instead we just want to catch this state and leave invoice in
pendingstatus - Async job for fetching taxes will process
pendinginvoice and put it either infinalizedorfailed(if there are tax errors like invalid product mapping)
| invoice.issuing_date = issuing_date | ||
| invoice.payment_due_date = payment_due_date |
There was a problem hiding this comment.
do we expect to overwrite invoice issuing_date and payment_due_date if it its not in draft status? why?
There was a problem hiding this comment.
you are right, draft invoice can already have this informations. However, when we are in the finalize mode, we need to overwrite those informations since finalization can happen also before original issuing date (if it is triggered manually)
| @@ -15,7 +15,7 @@ def call | |||
| ActiveRecord::Base.transaction do | |||
| invoice.issuing_date = issuing_date | |||
| refresh_result = Invoices::RefreshDraftService.call(invoice:, context: :finalize) | |||
| if tax_error?(refresh_result.error) | |||
There was a problem hiding this comment.
tax_error? method is not being called and can be deleted from this service.
There was a problem hiding this comment.
Nice catch, will remove it
| @@ -34,7 +34,7 @@ def call | |||
| context: | |||
| ) | |||
|
|
|||
| set_invoice_generated_status unless invoice.failed? | |||
| set_invoice_generated_status unless invoice.pending? | |||
There was a problem hiding this comment.
why this change from pending to failed?
There was a problem hiding this comment.
Before we were fetching taxes is sync mode (in the same step when we generated invoice) so invoice could be failed (failed status is only related to tax errors).
With new changes (converting fetching taxes to be async) it cannot be failed here, only pending. However background job that fetch taxes will put invoice either in finalized (draft) or failed
| end | ||
|
|
||
| it 'returns pending invoice' do | ||
| freeze_time do |
There was a problem hiding this comment.
do you need to freeze_time? there are no time checks
There was a problem hiding this comment.
updated, left from c/p
|
|
||
| result_data = result['data']['finalizeInvoice'] | ||
|
|
||
| aggregate_failures do |
There was a problem hiding this comment.
we aggregate_failures by default, hence, this is not needed anymore.
Context
Currently Anrok calls are sync and performed during invoice generation
Description
The goal of this improvement is to make calls to Anrok in dedicated job so that we can implement throttling for rate limit issues.
This PR changes all places where Anrok in used in sync way. This is the last PR that is extension of: