arthurdejong / python-stdnum

A Python library to provide functions to handle, parse and validate standard numbers.
https://arthurdejong.org/python-stdnum/
GNU Lesser General Public License v2.1
495 stars 205 forks source link

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

Closed andrp92 closed 3 years ago

arthurdejong commented 3 years ago

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.

andrp92 commented 3 years ago

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.

andrp92 commented 3 years ago

@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 commented 3 years ago

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

andrp92 commented 3 years ago

@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 commented 3 years ago

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

Thanks

frankroberto20 commented 3 years ago

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.

andrp92 commented 3 years ago

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.!

andrp92 commented 3 years ago

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

andrp92 commented 3 years ago

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

arthurdejong commented 3 years ago

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.