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

SmileID Enhanced Views #268

Merged
merged 2 commits into from
Dec 13, 2024
Merged

SmileID Enhanced Views #268

merged 2 commits into from
Dec 13, 2024

Conversation

jumaallan
Copy link
Member

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.

@prfectionist
Copy link

prfectionist bot commented Dec 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

Code Smell
Using userId as jobId in multiple places with comment "we use a userId and not a jobId here". This seems like a workaround that could cause issues. The relationship between userId and jobId should be clearly defined.

Design Issue
Comment "we need to discuss this" indicates uncertainty about the implementation of saveOfflineMode. This should be resolved before merging.

Code Duplication
Significant code duplication between smartSelfieAuthenticationScreen and smartSelfieAuthenticationScreenEnhanced methods. Consider refactoring to reduce duplication.

Copy link

github-actions bot commented Dec 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/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#L324 - Lines should not have trailing whitespace. (trailing_whitespace)

⚠️

Sources/SmileID/Classes/SmileID.swift#L403 - Lines should not have trailing whitespace. (trailing_whitespace)

Generated by 🚫 Danger Swift against 16c491c

@prfectionist
Copy link

prfectionist bot commented Dec 13, 2024

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Using a dedicated job identifier instead of reusing userId improves job tracking and prevents potential conflicts

The comment "we need to discuss this" indicates uncertainty about using userId as
jobId. Consider using a dedicated jobId or documenting the rationale for using
userId as jobId to ensure proper job tracking and avoid potential conflicts.

Sources/SmileID/Classes/SelfieCapture/SelfieSubmissionManager.swift [106-110]

-// we need to discuss this
 private func saveOfflineMode(jobType: JobType) throws {
+    let jobId = generateUniqueJobId() // Generate a unique job ID
     try LocalStorage.saveOfflineJob(
-        jobId: userId,
+        jobId: jobId,
         userId: userId,
Suggestion importance[1-10]: 8

Why: Using userId as jobId could lead to tracking issues and potential conflicts in job management. Implementing a dedicated jobId generation system would improve reliability and data integrity.

8
Maintainability
Replace implementation comments with proper documentation that explains the design rationale

The comment about using userId instead of jobId should be replaced with proper
documentation explaining the design decision and implications.

Sources/SmileID/Classes/SelfieCapture/EnhancedSmartSelfieViewModel.swift [288-290]

-// we use a userId and not a jobId here
+/// Creates a selfie file using userId as the identifier for consistent file management
+/// across the enhanced selfie capture flow
 self.selfieImageURL = try LocalStorage.createSelfieFile(
     jobId: userId, selfieFile: imageData)
Suggestion importance[1-10]: 4

Why: While the suggestion improves code documentation clarity, it's primarily a documentation enhancement that doesn't affect functionality. The impact is moderate as it helps future maintainability.

4

Copy link

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

Looks good to me. Thanks @jumaallan

@jumaallan jumaallan merged commit dc7d002 into main Dec 13, 2024
3 checks passed
@jumaallan jumaallan deleted the feat/update-smileid branch December 13, 2024 16:17
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