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

Add Consent Information to EnhancedDocV 📸 #292

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

jumaallan
Copy link
Member

Story: https://app.shortcut.com/smileid/story/14976/add-consent-screen-support-to-enhanced-doc-v-on-all-sdks

Summary

A few sentences/bullet points about the changes

Known Issues

Any shortcomings in your work. This may include corner cases not correctly handled or issues related
to but not within the scope of your PR. Design compromises should be discussed here if they were not
already discussed above.

Test Instructions

Concise test instructions on how to verify that your feature works as intended. This should include
changes to the development environment (if applicable) and all commands needed to run your work.

Screenshot

If applicable (e.g. UI changes), add screenshots to help explain your work.

@prfectionist
Copy link

prfectionist bot commented Feb 5, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 Security concerns

Data Privacy:
The implementation automatically sets all consent flags to true (personalDetailsConsentGranted, contactInformationConsentGranted, documentInformationConsentGranted) without actual user confirmation. This could violate data protection regulations that require explicit user consent before processing personal information.

⚡ Recommended focus areas for review

Hardcoded Values
The consent information values are hardcoded to true without any user input or validation. This should be configurable based on actual user consent.

Null Consent
Setting consentInformation to nil in multiple places could cause issues if consent tracking is required. Consider making this parameter required or handle the nil case explicitly.

Copy link

github-actions bot commented Feb 5, 2025

Warnings
⚠️ The source files were changed, but the tests remain unmodified. Consider updating or adding to the tests to match the source changes.
⚠️

Sources/SmileID/Classes/DocumentVerification/Model/OrchestratedDocumentVerificationViewModel.swift#L118 - Function should have complexity 10 or less: currently complexity equals 15 (cyclomatic_complexity)

⚠️

Sources/SmileID/Classes/DocumentVerification/Model/OrchestratedDocumentVerificationViewModel.swift#L211 - TODOs should be resolved (- Fix when Michael changes thi...). (todo)

⚠️

Sources/SmileID/Classes/DocumentVerification/Model/OrchestratedDocumentVerificationViewModel.swift#L335 - Colons should be next to the identifier when specifying a type and next to the key in dictionary literals. (colon)

⚠️

Sources/SmileID/Classes/DocumentVerification/Model/OrchestratedDocumentVerificationViewModel.swift#L360 - Colons should be next to the identifier when specifying a type and next to the key in dictionary literals. (colon)

⚠️

Sources/SmileID/Classes/SmileID.swift#L161 - Function should have complexity 10 or less: currently complexity equals 11 (cyclomatic_complexity)

⚠️

Sources/SmileID/Classes/SmileID.swift#L188 - TODOs should be resolved (- Fix when Michael changes thi...). (todo)

Generated by 🚫 Danger Swift against f761c67

@prfectionist
Copy link

prfectionist bot commented Feb 5, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Best practice
Avoid hardcoding consent values and timestamps to ensure accurate consent tracking

Instead of hardcoding consent values to true and using current date, these values
should be passed as parameters to allow flexibility and accurate consent tracking.

Example/SmileID/DocumentVerification/EnhancedDocumentVerificationWithSelector.swift [21-26]

-consentInformation: ConsentInformation(
-    consentGrantedDate: Date().toISO8601WithMilliseconds(),
-    personalDetailsConsentGranted: true,
-    contactInformationConsentGranted: true,
-    documentInformationConsentGranted: true
-),
+consentInformation: consentInformation,
Suggestion importance[1-10]: 8

Why: Hardcoding consent values is a significant issue as it could lead to inaccurate consent tracking and potential legal/compliance problems. The suggestion correctly identifies the need to make consent values configurable.

8
Typo
Fix incorrect markdown header formatting in changelog

The version number format in the changelog is malformed with double hash symbols.
Remove one hash symbol for correct formatting.

CHANGELOG.md [3]

-## ## 10.4.0 Unreleased
+## 10.4.0 Unreleased
Suggestion importance[1-10]: 7

Why: The double hash symbols in the changelog header is a clear formatting error that affects readability and markdown rendering. This is a straightforward fix that improves documentation quality.

7

Copy link
Contributor

@robin-ankele robin-ankele left a comment

Choose a reason for hiding this comment

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

Tested an enhancedDocV flow and it adds as expected the consent object to the info.json. Thanks @jumaallan

@jumaallan jumaallan merged commit d71961d into main Feb 5, 2025
4 checks passed
@jumaallan jumaallan deleted the bugfix/add-consent-enhanced-docv branch February 5, 2025 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants