-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat: Add Push Primer feature using Custom Json (RMCCX-6702) #322
feat: Add Push Primer feature using Custom Json (RMCCX-6702) #322
Conversation
Current coverage for RInAppMessaging.framework is
|
Files changed | - | - |
---|---|---|
UIApplication+IAM.swift | 89.06% |
✅ |
CampaignsValidator.swift | 94.62% |
✅ |
FullViewPresenter.swift | 96.35% |
✅ |
CampaignData.swift | 100.00% |
✅ |
CampaignDataModels.swift | 100.00% |
✅ |
MainContainer.swift | 100.00% |
✅ |
Campaign.swift | 100.00% |
✅ |
Powered by xcov
Generated by 🚫 Danger
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.
- Please add test cases to validate new push primer implementation
- What about legacy dashboard push primer functionality? Would those implementation work / valid?
return nil | ||
}() | ||
|
||
private static let appNotificationSettingsURL = URL( |
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.
It could be method property of func openAppNotificationSettings()
. No need of having static property so that you can do nil check for notificationSettingsURLString
instead of assigning empty string.
string: notificationSettingsURLString ?? "" | ||
) | ||
|
||
func openAppNotificationSettings() -> Void { |
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.
nit: -> Void
not required to be mentioned.
@@ -56,16 +56,17 @@ internal class FullViewPresenter: BaseViewPresenter, FullViewPresenterType, Erro | |||
|
|||
func loadButtons() { | |||
let buttonList = campaign.data.messagePayload.messageSettings.controlSettings.buttons | |||
|
|||
let pushPrimerButton = campaign.data.customJson?.pushPrimer?.button ?? nil |
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.
?? nil
is not need if it can accept nil & no need any default value.
urlString = UIApplicationOpenNotificationSettingsURLString | ||
} else if #available(iOS 8.0, *) { | ||
urlString = UIApplication.openSettingsURLString | ||
} else { |
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.
else
part is not required.
@@ -32,7 +32,7 @@ internal struct Campaign: Codable, Hashable { | |||
}.filter { !$0.isEmpty } | |||
} | |||
var isPushPrimer: Bool { | |||
return data.customJson?.pushPrimer?.button != nil | |||
return RInAppMessaging.isRMCEnvironment && data.customJson?.pushPrimer?.button != nil |
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.
change property name else move the RMC environment check to different place
let semaphore = DispatchSemaphore(value: 0) | ||
var authorizationStatus: UNAuthorizationStatus = .notDetermined | ||
|
||
let center = UNUserNotificationCenter.current() |
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.
inject the UNUserNotificationCenter with custom protocol to test these feature
Tests/Tests/ViewPresenterSpec.swift
Outdated
@@ -658,6 +658,85 @@ class ViewPresenterSpec: QuickSpec { | |||
expect(impressionService.sentImpressions?.list).toNot(contain(.optOut)) | |||
} | |||
} | |||
context("when custom json data is available in Campaign Data") { |
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.
change the description to mention "when push primer content available" context.
@@ -90,4 +103,22 @@ internal struct CampaignsValidator: CampaignsValidatorType { | |||
|
|||
return triggeredEvents | |||
} | |||
|
|||
internal func isNotificationAuthorized() -> Bool { |
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.
By default func is internal, no need to specify explicitly
@@ -21,3 +21,27 @@ extension UNUserNotificationCenter: RemoteNotificationRequestable { | |||
} | |||
} | |||
} | |||
|
|||
protocol UserNotificationCenter { |
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.
make use of RemoteNotificationRequestable
& follow the implementation of that
authorizationStatus = settings.authorizationStatus | ||
semaphore.signal() | ||
} | ||
semaphore.wait() | ||
|
||
return authorizationStatus == .authorized | ||
switch authorizationStatus { |
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.
Change this to previous implementation where it was relay on .authorized
else it wouldn't work when status was provisional
.
Quality Gate passedIssues Measures |
Description
Added PushPrimer feature using Custom JSON
Added functionality to navigate to settings when authorization is previously denied
Added check for not displaying campaign if authorization is previously allowed
Added check for displaying Push Primer campaigns only for RMC users
Links
[RMCCX-6702]
Checklist
fastlane ci
without errorsSampleSPM/SampleSPM.xcodeproj
project