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

Stp transfer #394

Merged
merged 23 commits into from
Dec 29, 2022
Merged

Stp transfer #394

merged 23 commits into from
Dec 29, 2022

Conversation

mike-one
Copy link
Contributor

closes #393

@mike-one mike-one added the bug Something isn't working label Dec 29, 2022
@mike-one mike-one self-assigned this Dec 29, 2022
Copy link

@andreshndz andreshndz left a comment

Choose a reason for hiding this comment

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

Por ahora mejorar el test para que probemos que no buscamos en CEP las transacciones de STP

@@ -244,6 +244,37 @@ def test_send_transaction_restricted_accounts_curp_from_cep(
)


@patch('celery.Celery.send_task')
@pytest.mark.vcr
def test_send_transaction_restricted_accounts_stp_to_stp(

Choose a reason for hiding this comment

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

Este test valida que se envía el status, pero es una transferencia de STP a STP. Debemos validar que no se mandó a buscar el CEP

Puedes hacer un assert que no se llamó a la task del CEP o que no hay ningún mensaje enviado a esa task

@codecov
Copy link

codecov bot commented Dec 29, 2022

Codecov Report

Merging #394 (46faa0b) into master (59673da) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #394   +/-   ##
=======================================
  Coverage   99.61%   99.61%           
=======================================
  Files          26       26           
  Lines        1041     1043    +2     
  Branches       94       93    -1     
=======================================
+ Hits         1037     1039    +2     
  Partials        4        4           
Flag Coverage Δ
unittests 99.61% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
speid/tasks/transactions.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59673da...46faa0b. Read the comment docs.

Copy link
Member

@pachCode pachCode left a comment

Choose a reason for hiding this comment

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

Me parece que en ocasiones no solo los de stp a stp falla, también por ejemplo asp para solucionarlo hay que hacer que se mande el status pero agregar una nueva queue para mandar los datos una vez actualizado el cep, una opción sería separar las queues y crear también en core o manejar una lógica para que se mande el status y posteriormente se valide el cep
En preferencia creo que la primera es una opción más limpia

@mike-one
Copy link
Contributor Author

Ya no le puse el try-excpet MaxRetriesExceededError porque en el código hacemos un if antes de hacer el retry y en el momento que excede el número de intentos, ya no se hace el retry.
Los test ya contemplan el flujo, por eso no agregué más.

@@ -93,7 +95,7 @@ def send_transaction_status(self, transaction_id: str, state: str) -> None:
except MaxRequestError:
rfc_curp = 'max retries'
except CepError:
self.retry(countdown=GET_RFC_TASK_DELAY)
pass
Copy link
Member

Choose a reason for hiding this comment

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

puedes juntar las excepciones MaxRequestError y CepError para en los dos casos setear rfc_curp = None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

En MaxRequestError se define el valor de 'max retries' para que a la hora de ejecutar el if, no vuelva a hacer el retry

        if (
            not rfc_curp or rfc_curp == 'ND'
        ) and self.request.retries < GET_RFC_TASK_MAX_RETRIES:
            self.retry(countdown=GET_RFC_TASK_DELAY * self.request.retries)

Podría juntar CepError y AssertionError o si quieres después del if, le cambio de 'max retries' a None

Copy link
Member

Choose a reason for hiding this comment

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

Ah si, me confundí es juntar Assertion y Cep Error
y poner en try el de celery MaxRetriesExceededError

if not rfc_curp or rfc_curp == 'ND':
        try:
                self.retry(countdown=GET_RFC_TASK_DELAY * self.request.retries)
        except MaxRetriesExceededError:
                pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Va

Copy link
Member

@pachCode pachCode left a comment

Choose a reason for hiding this comment

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

También queda más limpio si dejas el retry de la taareaa en un try con la exxcepcion MaxRetriesExceededError y dejas terminar la tarea

Copy link

@andreshndz andreshndz left a comment

Choose a reason for hiding this comment

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

Solo un cambio y ya

@@ -93,7 +95,7 @@ def send_transaction_status(self, transaction_id: str, state: str) -> None:
except MaxRequestError:
rfc_curp = 'max retries'
except CepError:
self.retry(countdown=GET_RFC_TASK_DELAY)
pass

Choose a reason for hiding this comment

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

... en vez de pass

Copy link

@andreshndz andreshndz left a comment

Choose a reason for hiding this comment

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

Yo o veo bien, @pachCode que dices?

@andreshndz andreshndz merged commit c4197a6 into master Dec 29, 2022
@andreshndz andreshndz deleted the stp_transfer branch December 29, 2022 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When cep validation fails, the transaction status never send the update
3 participants