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

This PR Implements E-CF handling and fixes an attribute error #248

Closed
wants to merge 5 commits into from

Conversation

andres-pcg
Copy link
Contributor

No description provided.

@arthurdejong
Copy link
Owner

Hi @andrp92 ,

Thanks for the contribution, sorry I didn't pick it up sooner. Can you provide some more background for this change? The change extends the lookup function but doesn't change the validation.

Can you add a quick note on the extra parameters that were introduced? They only seem to be used for an NCF that starts with an E (is it E-CF or e-NCF?).

Can you also provide translations for the other returned values?

Can you provide a test number or invocation to demonstrate this?

Thanks.

@andres-pcg
Copy link
Contributor Author

andres-pcg commented Jan 26, 2021

Hi @arthurdejong,

This change allows stdnum to evaluate not only normal fiscal sequence (NCF) but also electronic ones (E-NCF).

E-CF validation is already being performed here https://github.com/arthurdejong/python-stdnum/blob/master/stdnum/do/ncf.py#L84

Both terms are correct since they refer to two different stuff (it's confusing, right? jeje). E-CF is the named they adopted for this new way of issuing electronic invoices. E-NCF is the number with the electronic format. The extra parameters that were introduced are required when entering E-NCF on the page
Screen Shot 2021-01-26 at 7 14 08 PM

In regards of the translation, thanks for the observations, I will definitely add them.

You can use these for demonstration:
RNC: 101001577
NCF (E-NCF): E310002290395
pd: I don't currently have a sample security code or buyerRNC but I can definitely get one ASAP.

If you have any other questions, please let me know and again, thanks a lot for the feedbacks.

@andres-pcg
Copy link
Contributor Author

andres-pcg commented Jan 26, 2021

@arthurdejong something else i've realized, I see travis is yelling at me with some flake8 error. Is it something I'm doing wrong on my end?

ERROR:   flake8: commands failed
The command "tox -e "${TOXENV:-$(echo py$TRAVIS_PYTHON_VERSION | tr -d . | sed -e 's/pypypy/pypy/')}" --skip-missing-interpreters false" exited with 1.

@arthurdejong
Copy link
Owner

I would prefer to Hi @andrp92 ,

The flake8 errors can be found here: https://travis-ci.com/github/arthurdejong/python-stdnum/jobs/473198524

Most of them are related to trailing white space at the end of the line that shouldn't be there. Another thing is that the variable names should probably be buyer_rnc and security_code to conform with Python standard naming conventions.
Lastly the docstring you added needs to be indented differently: https://travis-ci.com/github/arthurdejong/python-stdnum/jobs/473198525

Anyway, those are all pretty simple fixes I can make before merging. The most important bits are

  • Some test number that I can add to the tests (I think only the buyer RNC is required for the validation to work but if you have a security code as well that would help)
  • The translation of the items in the returned structure

@andres-pcg
Copy link
Contributor Author

andres-pcg commented Mar 19, 2021

@arthurdejong Hello, sorry for the late response. I'm confused about the translation process. Based on the documentations, response will be on Spanish, should we add translation from Spanish to english?

Regards the test data, you can use the info bellow:

RNC : 131793916
E-CF: E310000000001
Buyer RNC: 101654325
security code: PkdNvp

@arthurdejong
Copy link
Owner

It seems that the numbers provided do not validate on https://dgii.gov.do/app/WebApps/ConsultasWeb2/ConsultasWeb/consultas/ncf.aspx

Regarding the translation: the normal NCF lookup returns a dict with keys name, status, type, rnc, etc. I would like to have the same (or equivalent) keys for the E-NCF. I would like translations for

  • Rnc Emisor
  • Rnc Comprador
  • Código de Seguridad
  • Monto Total
  • Total de ITBIS
  • Fecha Emisión
  • Fecha de Firma

Thanks

@frankroberto20
Copy link
Contributor

frankroberto20 commented Jun 10, 2021

Hi @arthurdejong, I'm part of the same team as @andrp92 and have been tasked with working with this fork. Sorry for the late response.
On the last commit I added the translations required for each new field and updated the check_dgii description. Also updated variable names to buyer_rnc , security_code , result_path, lbl_path, and removed trailing whitelines.

Here is some test data that you can use to test if functionality works as expected:

ENCF
rnc: 101010632
ncf: E310049533639
buyerRNC: 22400559690
securityCode: hnI63Q

NCF
rnc: 131065831
ncf: B0200078831

If there are any more issues to fix, please do let me know.

@andres-pcg
Copy link
Contributor Author

Hey @arthurdejong , I know it's been a while. Please check the latest changes submitted by @frankroberto20 and let us know if there's anything else we need to do to get this merged.

Thanks.!

@andres-pcg
Copy link
Contributor Author

@arthurdejong sorry for insisting with this one. We just have an app crashing right know 😅

@andres-pcg
Copy link
Contributor Author

Hello @arthurdejong , please check this out whenever you can.

@arthurdejong
Copy link
Owner

Hi @andrp92, @frankroberto20 and @crisog,

Thanks for providing the fixes.I have added some tests and merged the extra validation as 2b452b6. The other fixes have been merged as 48e6502 and 4c51860.

Sorry it took so long to merge.

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.

4 participants