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.

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

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
* 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
  ...
* 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
  ...
* 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
@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.

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
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
* 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
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
* 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants