Skip to content

Digital Signature #167

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 25 commits into
base: develop
Choose a base branch
from
Open

Digital Signature #167

wants to merge 25 commits into from

Conversation

stankut
Copy link
Collaborator

@stankut stankut commented Apr 23, 2025

No description provided.

stankut added 14 commits October 4, 2024 14:55
@stankut
DIgital signature first version
ae97710
@stankut
OS-110 code refactoring, security
987b6ac
@stankut
OS-110 upload file for signature
53fb77e
@stankut
Upload digital document
ff96214
@stankut
OS-110 Signing service: Support for adding an extra page with signing…
3be3b70 
… info
@stankut
Merge commit '841076066c102e5236808e7a9c020ca6092bc5cd' into f/OS-110
f60bfbc 
* commit '841076066c102e5236808e7a9c020ca6092bc5cd': (37 commits)
  Release 3.20.0
  Applied coding standards
  Added audit logging to FBS handler
  Added audit logging to Fasit
  Added webform id to digital post audit log messages
  Release 3.19.0
  Adjusted audit logging message
  Added audit logging to os2forms_nemid
  Release 3.18.0
  Added error handling upon saving XML
  Updated CHANGELOG
  Audit logged digital post
  Ensured audit log is installed
  Release 3.17.0
  Cleanup
  Updated os2web_audit and os2web_datalookup versions
  Added further documentation
  Release 3.16.2
  Applied coding standards
  Applied coding standards
  ...
@stankut
OS-144 - adding return URL
d06c862
@stankut
OS-145 adding websubmissions automatic cleaning
5d36f4a
@stankut
OS-161 Disabling annotation page by default
8d3b2a7
@stankut
OS-144 Fixing cancel digital signature
43e5a04
@stankut
Merge commit '7ace38332357344358eca1935f43ae2a30c23fa4' into f/OS-110
2b4a5a1 
* commit '7ace38332357344358eca1935f43ae2a30c23fa4': (31 commits)
  Release 3.22.1
  Preparing 3.22.0 release
  OS-143 - Removed image_widget_crop
  Simplified validation
  Changed validation to check for empty string instead of null
  Conditional inclusion of patron phone number, since field is not required
  Updated changelog
  renamed fbs class
  Coding standards
  Add barn_tlf as an incoming field in FbsWebformHandler
  Handle phoneNumber assignment in advanced queue job
  Make phoneNumber a writeable property in Patron model
  Add support for phone numbers in FBS payloads
  Updating os2web_simplesaml to D10 compatible version
  Prepare release 3.21.2
  Add missing iterable type specifications
  Run code analysis on fbs handler
  Add and use http-message-util
  Release 3.21.1
  ITKDEV: Used configured format in maestro notification
  ...
@stankut
OS-167 adding Digital signature validation text
0db0714
@stankut
Merge commit 'eecb47a101ea651f749c7f1bce1ea7313ae3cdce' into f/OS-110
592ddd1 
* commit 'eecb47a101ea651f749c7f1bce1ea7313ae3cdce':
  OS-175 package version on status page
  Releasing 4.0.0
  Updated CHANGELOG
  Update os2web_audit
  OS-115 Skipping address with empty matrikelnummer phpcs fix
  OS-115 Skipping address with empty matrikelnummer
@stankut
Adding module description
6d6af9f
@rimi-itk
Copy link
Collaborator

@stankut, what is this pull request about and what is it trying to solve? Please provide a description and maybe a reference to a task with additional details.

@rimi-itk
Copy link
Collaborator

@stankut, please address the failing code checks (https://github.com/OS2Forms/os2forms/pull/167/checks). Most of them may be resolved by merging #168 (after review).

@ds-bellcom
Copy link

@stankut, what is this pull request about and what is it trying to solve? Please provide a description and maybe a reference to a task with additional details.

Hi @rimi-itk.

This task adds the ability to check/uncheck "Digital Signature" on the "OS2Forms Attachment" element and after submitting the form, sign the PDF file via "The Danish Digital Agency's signing component" (in danish: “Digitaliseringsstyrelsens signeringskomponent”).

You can see more about the task in our documentation here: https://github.com/OS2Forms/os2forms_docs/blob/master/docs/digital_signature/documentation-on-digital-signature-module.md

@ds-bellcom
Copy link

@stankut, please address the failing code checks (https://github.com/OS2Forms/os2forms/pull/167/checks). Most of them may be resolved by merging #168 (after review).

Hi @rimi-itk.

We will review that on Wednesday, May 28th.

@ds-bellcom ds-bellcom mentioned this pull request May 26, 2025
Open
rimi-itk
rimi-itk requested changes May 26, 2025
View reviewed changes
Copy link
Collaborator

@rimi-itk rimi-itk left a comment

Choose a reason for hiding this comment

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

Generally a lot of good stuff. However, there are some inconsistencies in how things are done, e.g. when dependency injection is used and when static functions in \Drupal are used. I have a feeling that some of the code has been written by a non-human programmer …

@stankut
Copy link
Collaborator Author

stankut commented May 30, 2025

@rimi-itk
thanks for the review. fixes are now mad.

As now phpcs is working again, there pop-up some warning from the other modules, not related with this PR. Namely os2forms_attachment
i don't think we should include it into this PR

@stankut stankut requested a review from rimi-itk May 30, 2025 13:47
rimi-itk
rimi-itk requested changes Jun 2, 2025
View reviewed changes
@rimi-itk
Copy link
Collaborator

rimi-itk commented Jun 2, 2025

@rimi-itk thanks for the review. fixes are now mad.

As now phpcs is working again, there pop-up some warning from the other modules, not related with this PR. Namely os2forms_attachment i don't think we should include it into this PR

Your "branch is 18 commits ahead of, 9 commits behind develop." You probably need a merge/rebase with develop.

stankut added 2 commits June 2, 2025 16:11
@stankut
Merge branch 'develop' into f/OS-110
1599ed0 
* develop:
  Added docker compose setup
  Update .github/workflows/pr.yml
  Cleaned up code
  Cleaned up code
  Cleaned up changelog
  Fixed digital post command. Updated GitHub Actions steps.
@stankut
OS-110 injecting dependency
9ae977f
@stankut
Copy link
Collaborator Author

stankut commented Jun 2, 2025

Last issues fixed, @rimi-itk please have a look now

@stankut stankut requested a review from rimi-itk June 2, 2025 13:49
@rimi-itk
Copy link
Collaborator

rimi-itk commented Jun 2, 2025

Last issues fixed, @rimi-itk please have a look now

@stankut, I know that GitHub is good at hiding comments ("conversations") on code and it's easy to loose sight of what has been adressed and what hasn't, but please address all comments, e.g. #167 (comment), and use "Resolve conversation" when done.

We should use dependency injection whenever possible – and in general be consistent on how we do things.

stankut added 5 commits June 9, 2025 12:36
@stankut
OS-110 refactoring
e01f0c1
@stankut
OS-110 refactoring, service injections
23fa488
@stankut
OS-110 refactoring, service injections
4681c85
@stankut
OS-110 refactoring, phpcs
2482846
@stankut
Merge branch 'develop' into f/OS-110
7346ebc 
* develop:
  Release 4.1.0
  #4232: Normalized composer
  Updated CHANGELOG
  Added patch allowing attachments in coc_forms_auto_export
  Removed old patches
  Removed old patches
  Updated composer
@stankut
Copy link
Collaborator Author

stankut commented Jun 10, 2025

@rimi-itk think i checked all the comments, please have a look

@rimi-itk
Copy link
Collaborator

@rimi-itk think i checked all the comments, please have a look

Looks good, @stankut. Approved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rimi-itk rimi-itk rimi-itk approved these changes

@cableman cableman Awaiting requested review from cableman

@martinyde martinyde Awaiting requested review from martinyde

@jekuaitk jekuaitk Awaiting requested review from jekuaitk

@ChatBotBerg ChatBotBerg Awaiting requested review from ChatBotBerg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stankut @rimi-itk @ds-bellcom