Add helper to finalise interrupted validations

[WhyNotHugo]

Find receipts that seem to have failed a previous submission and call revalidate for them.

[npazosmendez]

Buenas! Cómo te imaginabas la implementación de esto? Una que se me ocurre es agregarle un tercer "estado" RESULT_PENDING a ReceiptValidation, y que el procedimiento para validar un Receipt sea:

1. Crear ReceiptValidation con result=RESULT_PENDING
2. Validar el Receipt
3. Setear result=RESULT_APPROVED o result=RESULT_REJECTED al ReceiptValidation existente, según corresponda.

Luego la app al arrancar busca todos los ReceiptValidation en pending y llama revalidate para ellos.

Otra opción más barata podría ser un write ahead log. Te ahorras las llamadas extra a la DB, pero es más laburo de implementación y probablemente requiera persistent storage.

PD: excelente laburo este proyecto

[npazosmendez]

Ah, estuve mirando más en detalle el código y veo que sería más simple, por las garantías que ya da la implementación actual: buscar los Receipt sin un ReceiptValidation pero con un receipt_number. Esos son los candidatos a validaciones interrumpidas.

[WhyNotHugo]

Creo que esto podría funcionar:

for receipt in Receipt.objects.filter(
    receipt_number__isnull=False,
    validation__isnull=True,
):
    receipt.revalidate()

[WhyNotHugo]

Corrección: ese ejemplo saltearía comprobantes con alguna validación fallida.

[WhyNotHugo]

Capaz así?

for receipt in Receipt.objects.filter(
    receipt_number__isnull=False,
).exclude(
    validation__result=ReceiptValidation.RESULT_APPROVED,
):
    receipt.revalidate()

[npazosmendez]

Suena bien :+1: . Salvo que me esté perdiendo algo, el código actual nunca crea un ReceiptValidation con ReceiptValidation.RESULT_REJECTED ante errores, solamente ReceiptValidation.RESULT_APPROVED:

https://github.com/WhyNotHugo/django-afip/blob/7d08ed4bd28dc3e7302a040d7fee06435586aaf2/django_afip/models.py#L956-L985

En ese caso, tu versión anterior también debería funcionar.

Pero no sé si en algún momento se usó el RESULT_REJECTED, y la DB podría tener alguno con result = RESULT_REJECTED.

[npazosmendez]

Tema relacionado: que opinás de que el revalidate setee receipt_number=None en caso de que no haya sido validado? Así como hace validate para los fallidos:

https://github.com/WhyNotHugo/django-afip/blob/7d08ed4bd28dc3e7302a040d7fee06435586aaf2/django_afip/models.py#L981

Si un Receipt no validado queda con receipt_number != None, al llamar Receipt.validate() se va a intentar usar ese receipt_number ya seteado, que probablemente sea viejo.

Edit: y ahora que lo pienso, no setearlo en None hace que se intente revalidar para siempre.

[WhyNotHugo]

Puede que valga la pena eliminar ReceiptValidation.RESULT_REJECTED completamente. Por ahora dejo un issue como recordatorio: https://github.com/WhyNotHugo/django-afip/issues/214

que opinás de que el revalidate setee receipt_number=None en caso de que no haya sido validado?

Puede haber problemas de concurrencia:

• [thread1] validate setea el receipt_number y empieza a comunicarse con el AFIP.
• [thread2] revalidate encuentra el comprobante a medio validar, consulta al AFIP el estado.
• [thread2] revalidate recibe su respuesta antes que validate. revalidate elimina el receipt_number.
• [thread1] guarda los datos de validación, pero el receipt_number queda perdido.

Es poco probable pero posible.

Creo que habría que lockear las fila durante la validación para evitar este problema.

[npazosmendez]

Buen catch. Estuve pensando en esto de setear el receipt_number en null, acá lo que pensé:

Opcion 1: Lockear las filas

Te dejo el walltext oculto, pero TLDR: creo que se pueden lockear para asegurar la integridad de datos, pero me surgen algunos caveats que hacen medio mala la UX.

walltext

Estuve viendo de cambiar `validate` para que lockee las filas, algo así (se poco de Django, perdón de antemano): ```python def validate(...): ... qs = self.filter(validation__isnull=True).check_groupable() # Return early if queryset is empty: first = qs.first() if first is None: return [] qs.order_by("issued_date", "id")._assign_numbers() with transaction.atomic(): qs = qs.filter(receipt_number__isnull=False).select_for_update() ... check_response(response) errs = [] for cae_data in response.FeDetResp.FECAEDetResponse: validation = ReceiptValidation.objects.create( ... # Remove the number from ones that failed to validate: qs.filter(validation__isnull=True).update(receipt_number=None) ``` De yapa, esto debería hacer al `validate()` thread safe (en cuanto a la integridad de datos, no en cuanto a excepciones de AFIP), por poner el `qs.filter(validation__isnull=True).update(receipt_number=None)` adentro de la transacción. Por otro lado `revalidate()` haría algo como: ```python def revalidate(self) -> ReceiptValidation | None: ... if receipt_data.Resultado == ReceiptValidation.RESULT_APPROVED: ... return validation # Update atomically, skip if it was just validated Receipt.objects.filter(pk=self.pk, validation__isnull=True).update( receipt_number=None, ) return None ``` Tendría que pensar un poco más para ver si me estoy periendo algún caso, pero hay algo de esto que no me gusta: si bien (creo) que esto hace que la race condition de tu ejemplo no pierda el `receipt_number` de una validación, lo que sí ocurre es que no se valida un Receipt al que el usuario llamó `.validate()`, se skipea. Se mantiene integridad, pero es un comportamiento medio inesperado. Una que se me ocurre es, luego del `qs = qs.filter(receipt_number__isnull=False).select_for_update()`, checkear si el set de receipts que quedó es el mismo, y si no, agregar un error a `errs` (o throwear?). No haría a `revalidate` y `validate` thread safe "entre sí", pero al menos el comportamiento es más intuitivo. Otra opción es empezar a jugar con locks con timeouts.

Opcion 2: comparar con último comprobante

Esta se me ocurrió después, mucho más simple: que el revalidate compare el Receipt.receipt_number con FECompUltimoAutorizado, probablemente dentro de una transacción por si cambia el point of sales y receipt type. Si ya fue usado, borrarlo.

Si no fue usado y se lo deja seteado, todavía podría haber problemas: podría asignarsele el mismo receipt_number a otro Receipt, y si se intentan validar juntos, qs.validate() va a throwear. Lo que podemos hacer es que el validate haga un except <El numero o fecha del comprobante no se corresponde con el proximo a autorizar. ..>, y setee todos los receipt_number = None si tal cosa pasa. Así, el próximo intento debería funcionar. Esto podría hacerse siempre de hecho, más allá del contexto de este problema.

---

PD: tal vez esté dandole mucha vuelta a un caso bastante anómalo :sweat_smile: . La verdad es que la implementación actual no tiene ningún problema de integridad de datos. Para clarificar, lo que estoy queriendo resolver sería: (1) que no se apilen los Receipts no validados por crashes, y (2) que Receipts.validate() para un receipt no validado por crashes no throwee.

[WhyNotHugo]

Me parece que la opción 1 va bien :+1:

[npazosmendez]

Acá hay un primer draft con la opción 1: https://github.com/WhyNotHugo/django-afip/pull/217