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

Liveness Timeout Images #253

Open
wants to merge 13 commits into
base: new-smart-selfie-capture
Choose a base branch
from

Conversation

tobitech
Copy link
Contributor

@tobitech tobitech commented Nov 13, 2024

Story: https://app.shortcut.com/smileid/story/13080/

Summary

  • Capture random images to complete the number of liveness images required when active liveness timeout elapses
  • Add { mobile_active_liveness_timed_out: true} to multipart request body data
  • Decouple SelfieViewModel from LivenessCheckManager.
  • Fix error subtitle message
  • Improve object references to avoid retain cycle.

Known Issues

N/A.

Test Instructions

  • Select Selfie Enrolment (Strict mode) product.
  • Capture Selfie but don't complete the active liveness challenge.
  • After 120secs a selfie enrolment job should be submitted regardless.
  • Result of the job should be "Failed On Device Liveness Check" just like this one https://portal.usesmileid.com/admin/job/production/197682513

Screenshot

N/A.

@tobitech tobitech self-assigned this Nov 13, 2024
@tobitech tobitech requested a review from a team as a code owner November 13, 2024 11:51
@prfectionist
Copy link

prfectionist bot commented Nov 13, 2024

Title

Liveness Timeout Images


User description

Story: https://app.shortcut.com/smileid/story/13080/

Summary

  • Capture random images to complete the number of liveness images required when active liveness timeout elapses
  • Add { mobile_active_liveness_timed_out: true} to multipart request body data
  • Decouple SelfieViewModel from LivenessCheckManager.
  • Fix error subtitle message
  • Improve object references to avoid retain cycle.

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

  • Decoupled LivenessCheckManager from SelfieViewModel through a new delegate protocol, improving code architecture and maintainability
  • Enhanced failure handling by implementing proper Encodable protocol for FailureReason and improving error messages
  • Improved memory management by fixing potential retain cycles with weak references
  • Refined timeout handling in liveness checks with proper delegate methods
  • Renamed variables for better clarity and updated validation logic
  • Added better error handling and file management throughout the codebase

Changes walkthrough 📝

Relevant files
Enhancement
LivenessCheckManager.swift
Decoupled LivenessCheckManager from SelfieViewModel           

Sources/SmileID/Classes/FaceDetector/LivenessCheckManager.swift

  • Added LivenessCheckManagerDelegate protocol for better decoupling
  • Removed direct dependency on SelfieViewModel
  • Improved timeout handling with delegate methods
  • +12/-8   
    FailureReason.swift
    Enhanced FailureReason enum with proper encoding                 

    Sources/SmileID/Classes/Networking/Models/FailureReason.swift

  • Made FailureReason conform to Encodable protocol
  • Renamed timeout case to mobileActiveLivenessTimeout
  • Implemented custom encoding logic
  • +10/-4   
    SelfieViewModelV2.swift
    Refactored SelfieViewModel with improved architecture       

    Sources/SmileID/Classes/SelfieCapture/SelfieViewModelV2.swift

  • Implemented LivenessCheckManagerDelegate
  • Added proper memory management with weak references
  • Improved timeout handling and image capture logic
  • +49/-24 
    SelfieSubmissionManager.swift
    Enhanced SelfieSubmissionManager with better error handling

    Sources/SmileID/Classes/SelfieCapture/SelfieSubmissionManager.swift

  • Renamed selfieImage to selfieImageUrl for clarity
  • Improved error handling and validation
  • Updated job submission logic with failure reason
  • +20/-23 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @prfectionist
    Copy link

    prfectionist bot commented Nov 13, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Memory Management
    The submissionTask cancellation in deinit could potentially leave resources in an inconsistent state if the task is cancelled mid-execution. Consider adding proper cleanup logic.

    Race Condition
    The currentFrameBuffer property is accessed from multiple places without synchronization, which could lead to race conditions.

    Error Handling
    The liveness challenge timeout handler captures random images without validating their quality, which could lead to poor quality submissions.

    Copy link

    github-actions bot commented Nov 13, 2024

    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#L115 - Function should have complexity 10 or less: currently complexity equals 15 (cyclomatic_complexity)

    ⚠️

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

    ⚠️

    Sources/SmileID/Classes/DocumentVerification/Model/OrchestratedDocumentVerificationViewModel.swift#L331 - 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#L357 - Colons should be next to the identifier when specifying a type and next to the key in dictionary literals. (colon)

    ⚠️

    Sources/SmileID/Classes/SelfieCapture/View/FaceShapedProgressIndicator.swift#L14 - TODOs should be resolved (Make this fill from bottom to ...). (todo)

    ⚠️

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

    ⚠️

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

    ⚠️

    Sources/SmileID/Classes/SmileID.swift#L214 - Collection literals should not have trailing commas. (trailing_comma)

    Generated by 🚫 Danger Swift against 1f890a4

    @prfectionist
    Copy link

    prfectionist bot commented Nov 13, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Cancel existing task before starting a new one to prevent concurrent operations

    The submissionTask should be cancelled before creating a new one to prevent multiple
    concurrent submissions.

    Sources/SmileID/Classes/SelfieCapture/SelfieViewModelV2.swift [276-278]

    +submissionTask?.cancel()
     submissionTask = Task {
         try await submitJob()
     }
    Suggestion importance[1-10]: 7

    Why: Important suggestion that prevents potential race conditions and resource leaks by ensuring only one submission task runs at a time. This could prevent subtle bugs in the submission process.

    7
    Possible bug
    Add proper error handling for missing image buffer in liveness timeout scenario

    Add error handling in the livenessChallengeTimeout method to handle potential nil
    imageBuffer.

    Sources/SmileID/Classes/SelfieCapture/SelfieViewModelV2.swift [346-349]

    +guard let imageBuffer = currentFrameBuffer else {
    +    debugPrint("No image buffer available for liveness capture")
    +    return
    +}
     for _ in 0..<count {
    -    if let imageBuffer = currentFrameBuffer {
    -        captureLivenessImage(imageBuffer)
    -    }
    +    captureLivenessImage(imageBuffer)
     }
    Suggestion importance[1-10]: 6

    Why: The suggestion improves error handling by checking for the image buffer availability upfront and provides better debugging information, which could help diagnose issues in the liveness capture process.

    6
    Best practice
    Add guard statement to safely unwrap weak self reference in closure to prevent potential nil access

    Consider using a weak reference to avoid potential retain cycles in the sink closure
    for the sampleBufferPublisher subscription.

    Sources/SmileID/Classes/SelfieCapture/SelfieViewModelV2.swift [146-148]

     .sink { [weak self] imageBuffer in
    -    self?.analyzeFrame(imageBuffer: imageBuffer)
    +    guard let self = self else { return }
    +    self.analyzeFrame(imageBuffer: imageBuffer)
     }
    Suggestion importance[1-10]: 4

    Why: While the current weak self usage is valid, adding a guard statement would improve code safety and readability by explicitly handling the nil case, though the impact is relatively minor since the current implementation is already safe.

    4

    Copy link
    Contributor

    @JNdhlovu JNdhlovu left a comment

    Choose a reason for hiding this comment

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

    iPhone X
    ios 16.7.10
    Stuck multiple times on the liveness tasks
    Video in slack

    JNdhlovu and others added 2 commits November 18, 2024 12:20
    * feat: update changelog
    
    * chore: lint fix
    Copy link
    Contributor

    @JNdhlovu JNdhlovu left a comment

    Choose a reason for hiding this comment

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

    @tobitech ran this fine through and through but here's a few things I found

    1. I think we need to debounce the feedback prompts sometimes they come on the screen all at once and can be confusing what to do next video in slack
    2. I ran jobs and got file not found on the sample app
    3. Attached screenshots are from the failed jobs on the portal
    Screenshot 2024-11-26 at 3 25 20 PM

    daviinaa and others added 6 commits November 26, 2024 18:49
    * added autoassign to workflow
    
    * added autoassign to workflow/fix
    # Conflicts:
    #	Example/Podfile.lock
    #	Sources/SmileID/Classes/SmileID.swift
    add a cancel toolbar button to all product screens.
    remove cancel button from liveness instructions screen.
    …veness capture
    
    hide liveness progress if face is not valid.
    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.

    3 participants