add option for provisional push and app settings to push manager #117
Conversation
maerki
requested review from
choli,
zenokoller,
ubfelix,
heerubique and
a-f-schneider
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 SwiftLint (0.57.0)Sources/UBPush/UBPushManager.swift[ WalkthroughThe pull request introduces modifications to the push notification management in the Changes
Sequence DiagramsequenceDiagram
participant App
participant UBPushManager
participant NotificationCenter
App->>UBPushManager: requestPushPermissions(provisional, providesAppSettings)
UBPushManager->>UBPushManager: makeAuthorizationOptions()
UBPushManager->>NotificationCenter: requestAuthorization with options
NotificationCenter-->>UBPushManager: Authorization Result
UBPushManager-->>App: Callback with result
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
Sources/UBPush/UBPushManager.swift (1)
Line range hint
240-258: Fix duplicate option setting and parameter naming inconsistency.There are two issues in the authorization options setup:
The
providesAppNotificationSettingsoption is set twice:
- Once for
includingNotificationSettings- Again for
providesAppSettingsParameter naming is inconsistent for similar functionality:
includingNotificationSettingsprovidesAppSettingsApply this diff to fix the issues:
-private func makeAuthorizationOptions(includingCritical: Bool, includingNotificationSettings: Bool, provisional: Bool, providesAppSettings: Bool) -> UNAuthorizationOptions { +private func makeAuthorizationOptions(includingCritical: Bool, provisional: Bool, providesAppSettings: Bool) -> UNAuthorizationOptions { var options: UNAuthorizationOptions = [.alert, .badge, .sound] if includingCritical { options.insert(.criticalAlert) } - if includingNotificationSettings { - options.insert(.providesAppNotificationSettings) - } if provisional { options.insert(.provisional) } if providesAppSettings { options.insert(.providesAppNotificationSettings) } return options }Then update the call site:
let options = makeAuthorizationOptions( includingCritical: includingCritical, - includingNotificationSettings: includingNotificationSettings, provisional: provisional, providesAppSettings: providesAppSettings )
🧹 Nitpick comments (1)
Sources/UBPush/UBPushManager.swift (1)
192-193: LGTM! Consider enhancing the documentation.The new parameters are well-named and maintain backward compatibility. However, the method documentation should be updated to include descriptions for the new parameters.
Add parameter documentation:
/// - Parameters: /// - includingCritical: Also requests permissions for critical alerts; requires iOS 12 and needs special authorization from Apple /// - callback: The callback for handling the result of the request +/// - provisional: Requests provisional authorization that allows quiet notification delivery +/// - providesAppSettings: Indicates that the app supports in-app notification settings UI
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Package.swift(0 hunks)Sources/UBPush/UBPushManager.swift(4 hunks)
💤 Files with no reviewable changes (1)
- Package.swift
🔇 Additional comments (1)
Sources/UBPush/UBPushManager.swift (1)
Line range hint
192-210: Verify the impact on existing callers.The changes to
requestPushPermissionsmight affect existing code. Let's verify:
- Usage of the method in the codebase
- Impact on the example code in the class documentation
✅ Verification successful
Changes to
requestPushPermissionsare safe and backward compatibleThe method is not called anywhere in the codebase, and all new parameters have default values that maintain compatibility with existing external callers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to requestPushPermissions rg "requestPushPermissions\(" --type swift # Search for examples in documentation rg "requestPushPermissions.*\(" -A 5 -B 5 --type swiftLength of output: 1318
Summary by CodeRabbit
New Features
Dependency Changes