-
Notifications
You must be signed in to change notification settings - Fork 4
Feat: Update Android & iOS SDKs to v11.1.0 #131
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
Conversation
… and document verification
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
| fun String.toAutoCapture() : AutoCapture { | ||
| return when (this) { | ||
| "AutoCapture" -> AutoCapture.AutoCapture | ||
| "AutoCaptureOnly" -> AutoCapture.AutoCaptureOnly | ||
| "ManualCaptureOnly" -> AutoCapture.ManualCaptureOnly | ||
| else -> { | ||
| throw IllegalArgumentException( | ||
| "Invalid autoCapture value: $this. " + | ||
| "Expected 'AutoCapture', 'AutoCaptureOnly', or 'ManualCaptureOnly'." | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The function should handle null input gracefully since it's called with nullable strings. Consider returning a default value or making the parameter nullable to prevent runtime crashes. [possible issue, importance: 7]
| fun String.toAutoCapture() : AutoCapture { | |
| return when (this) { | |
| "AutoCapture" -> AutoCapture.AutoCapture | |
| "AutoCaptureOnly" -> AutoCapture.AutoCaptureOnly | |
| "ManualCaptureOnly" -> AutoCapture.ManualCaptureOnly | |
| else -> { | |
| throw IllegalArgumentException( | |
| "Invalid autoCapture value: $this. " + | |
| "Expected 'AutoCapture', 'AutoCaptureOnly', or 'ManualCaptureOnly'." | |
| ) | |
| } | |
| } | |
| } | |
| fun String?.toAutoCapture() : AutoCapture { | |
| return when (this) { | |
| "AutoCapture" -> AutoCapture.AutoCapture | |
| "AutoCaptureOnly" -> AutoCapture.AutoCaptureOnly | |
| "ManualCaptureOnly" -> AutoCapture.ManualCaptureOnly | |
| null -> AutoCapture.AutoCapture | |
| else -> { | |
| throw IllegalArgumentException( | |
| "Invalid autoCapture value: $this. " + | |
| "Expected 'AutoCapture', 'AutoCaptureOnly', or 'ManualCaptureOnly'." | |
| ) | |
| } | |
| } | |
| } |
| extension String { | ||
| func toAutoCapture() -> AutoCapture { | ||
| switch self { | ||
| case "AutoCapture": | ||
| return .autoCapture | ||
| case "AutoCaptureOnly": | ||
| return .autoCaptureOnly | ||
| case "ManualCaptureOnly": | ||
| return .manualCaptureOnly | ||
| default: | ||
| return .autoCapture | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The function silently returns a default value for invalid inputs, which could mask configuration errors. Consider throwing an error for invalid values to maintain consistency with the Android implementation. [general, importance: 5]
| extension String { | |
| func toAutoCapture() -> AutoCapture { | |
| switch self { | |
| case "AutoCapture": | |
| return .autoCapture | |
| case "AutoCaptureOnly": | |
| return .autoCaptureOnly | |
| case "ManualCaptureOnly": | |
| return .manualCaptureOnly | |
| default: | |
| return .autoCapture | |
| } | |
| } | |
| } | |
| extension String { | |
| func toAutoCapture() throws -> AutoCapture { | |
| switch self { | |
| case "AutoCapture": | |
| return .autoCapture | |
| case "AutoCaptureOnly": | |
| return .autoCaptureOnly | |
| case "ManualCaptureOnly": | |
| return .manualCaptureOnly | |
| default: | |
| throw NSError(domain: "SmileIDError", code: 1, userInfo: [NSLocalizedDescriptionKey: "Invalid autoCapture value: \(self). Expected 'AutoCapture', 'AutoCaptureOnly', or 'ManualCaptureOnly'."]) | |
| } | |
| } | |
| } |
| autoCaptureTimeout = autoCaptureTimeout?.seconds ?: 10.seconds, | ||
| autoCapture = autoCapture ?: AutoCapture.AutoCapture, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The timeout conversion could fail if autoCaptureTimeout contains an invalid value. Add validation to ensure the timeout is within reasonable bounds before conversion. [general, importance: 6]
| autoCaptureTimeout = autoCaptureTimeout?.seconds ?: 10.seconds, | |
| autoCapture = autoCapture ?: AutoCapture.AutoCapture, | |
| autoCaptureTimeout = autoCaptureTimeout?.let { | |
| if (it > 0) it.seconds else 10.seconds | |
| } ?: 10.seconds, | |
| autoCapture = autoCapture ?: AutoCapture.AutoCapture, |
| self.product.autoCaptureTimeout = params["autoCaptureTimeout"] as? Int ?? 10 | ||
| self.product.autoCapture = autoCapture?.toAutoCapture() ?? .autoCapture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The timeout value should be validated to ensure it's positive before assignment. Negative or zero timeouts could cause unexpected behavior in the capture process. [general, importance: 6]
| self.product.autoCaptureTimeout = params["autoCaptureTimeout"] as? Int ?? 10 | |
| self.product.autoCapture = autoCapture?.toAutoCapture() ?? .autoCapture | |
| let timeout = params["autoCaptureTimeout"] as? Int ?? 10 | |
| self.product.autoCaptureTimeout = max(1, timeout) | |
| self.product.autoCapture = autoCapture?.toAutoCapture() ?? .autoCapture |
User description
Story: https://app.shortcut.com/smileid/story/xxx
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.
PR Type
Enhancement
Description
• Replace
enableAutoCapturewithAutoCaptureenum for better control• Add
autoCaptureTimeoutparameter for configurable capture timing• Update Android and iOS SDKs to v11.1.0
• Remove default
ConsentInformationvalues, now supports nullChanges walkthrough 📝
18 files
Add AutoCapture enum mapping functionRemove default ConsentInformation requirementAdd auto capture timeout and enum supportReplace enableAutoCapture with timeout and enumUpdate auto capture and consent handlingMake consentInformation optional parameterAdd auto capture timeout and enum propertiesReplace boolean with timeout and enumUpdate auto capture and consent parametersAdd auto capture configuration examplesAdd AutoCapture enum and update interfacesAdd AutoCapture enum conversion functionRemove consent information validation requirementReplace enableAutoCapture with timeout and enumUpdate auto capture parametersReplace boolean with timeout and enumRemove default ConsentInformation creationAdd auto capture timeout and enum handling1 files
Fix BuildConfig import reference8 files