search

vbuch / node-signpdf

Simple signing of PDFs in node.
MIT License
723 stars 178 forks source link

multiple signatures (rebased from vizicsaba89 PR) #38

Closed andres-blanco closed 4 years ago

andres-blanco commented 5 years ago

It seems like @vizicsaba89 rebased the commits on develop: https://github.com/vizicsaba89/node-signpdf I've made a fork here https://github.com/SIU-Toba/node-signpdf/ rebasing the changes from vizicsaba89 onto your updated develop branch. I'm interested in this feature, so if there are any fixes to be done count me in.

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 224

Totals Coverage Status
Change from base Build 213: 0.0%
Covered Lines: 268
Relevant Lines: 268
💛 - Coveralls
vbuch commented 5 years ago

Without having looked into any of the code, I would ask for 100% coverage. The package had it so far, and I think 2.4% are feasible. Also check the discussion here: https://github.com/vbuch/node-signpdf/issues/25

andres-blanco commented 5 years ago

I've made some modifications, I am working on getting the coverage back to 100%

vbuch commented 5 years ago

Nice! I'll give it a rry tomorrow and if I don't have remarks, will merge

vizicsaba89 commented 5 years ago

Lately i didn't have much time, so thank you guys for fixing these bugs / typos. As soon as i have available time, i'll start working on adding configurable images and texts to digital signatures.

andres-blanco commented 5 years ago

I think there is only one thing that needs to be done here before it gets merged. That is: I want to see some tests of the readRefTable function. This would be mostly "in-depth code review" with some written code. Some testing. Nothing too much in detail. Maybe just snapshot testing of the current resources? Just making sure there are no regressions in future. Whatever you guys think would suffice.

Great, I'll see what I can do. I think tomorrow I will be able to sit down with this.

andres-blanco commented 4 years ago

I've added some basic tests to prevent regressions on readRefTable. I wont be able to write deeper tests on this function since I am not that familiar with the xref table of pdf files. Let me know if you think this is enough

vbuch commented 4 years ago

I just reused your test @andres-blanco and took a snapshot of the ref tables of all the pdfs available in the resources dir. I think that will save us from regressions. I will merge this in the develop branch of vbuch/node-signpdf but won't release it yet. Will do that later today or tomorrow because I need to check something first.

Thank you all for the contribution! @andres-blanco @therpobinski @vizicsaba89