Skip to content

Conversation

@Nightsd01
Copy link
Contributor

@Nightsd01 Nightsd01 commented Mar 22, 2018

• Moves badge increment/decrement logic from the OneSignal backend to the application itself
• This is accomplished using an app extension with an 'app group' to allow communication of data between the host app and it's notification extension service
• This allows the app to maintain a consistent badge count
• Also cleaned up some Firebase code and moved common strings to a definitions file
• Changed the SDK so that it will default to group.{bundle_id}.onesignal as the App Group name if one is not provided in Info.plist


This change is Reviewable

• Moves badge increment/decrement logic from the OneSignal backend to the application itself
• This is accomplished using an app extension with an 'app group' to allow communication of data between the host app and it's notification extension service
• This allows the app to maintain a consistent badge count
• Also cleaned up some Firebase code and moved common strings to a definitions file
• Changed the SDK so that it will default to group.{bundle_id}.onesignal as the App Group name if one is not provided in Info.plist
@Nightsd01 Nightsd01 requested a review from jkasten2 March 22, 2018 01:15
• Swizzles the setApplicationIconBadgeNumber() method so the SDK always has a consistent badge count even if the developer manually sets it.
• Adds a test to verify the extension badge handling logic works correctly
• Verifies that manually setting the badge number using UIApplication setApplicationIconBadgeNumber() also updates OneSignal SDK's cached badge value
• Verifies that the SDK correctly handles positive and negative (increment and decrement) badge_inc values
@jkasten2
Copy link
Member

Review status: 0 of 12 files reviewed at latest revision, 4 unresolved discussions.


iOS_SDK/OneSignalSDK/Source/OneSignalExtensionBadgeHandler.h, line 6 at r1 (raw file):

//
//  Created by Brad Hesse on 3/15/18.
//  Copyright © 2018 Hiptic. All rights reserved.

Fix copyright header


iOS_SDK/OneSignalSDK/Source/OneSignalExtensionBadgeHandler.m, line 6 at r2 (raw file):

//
//  Created by Brad Hesse on 3/15/18.
//  Copyright © 2018 Hiptic. All rights reserved.

Fix copyright header


iOS_SDK/OneSignalSDK/UnitTests/UnitTests.m, line 1835 at r2 (raw file):

            @"att": @{ @"id": @"http://domain.com/file.jpg" }
        },
        @"badge_inc" : @3

badge_inc should be under os_data or custom depending on the payload type. Otherwise this is going to end up in additional data when os_data is the format type.


iOS_SDK/OneSignalSDK/UnitTests/UnitTests.m, line 1854 at r2 (raw file):

    XCTAssert([(NSNumber *)[defaults objectForKey:ONESIGNAL_BADGE_KEY] isEqualToNumber:@2]);
}

Can we add an 2nd notification service extension service that the NSDefault is se when aps badge is set but badge_inc is not present?


Comments from Reviewable

• Modularizes access to NSUserDefaults in regards to badge logic to a single implementation file
• Moves the badge_inc property into the 'os_data' or 'custom' fields in push notification objects
• Extends badge tests to cover more scenarios
• Fixes header copyrights in multiple files
@Nightsd01
Copy link
Contributor Author

iOS_SDK/OneSignalSDK/Source/OneSignalExtensionBadgeHandler.h, line 6 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

Fix copyright header

Fixed in multiple files.


Comments from Reviewable

@Nightsd01
Copy link
Contributor Author

iOS_SDK/OneSignalSDK/UnitTests/UnitTests.m, line 1854 at r2 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

Can we add an 2nd notification service extension service that the NSDefault is se when aps badge is set but badge_inc is not present?

Moved the new badge tests to BadgeTests.m, added this as a test


Comments from Reviewable

@jkasten2
Copy link
Member

Reviewed 3 of 9 files at r1, 17 of 17 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@jkasten2
Copy link
Member

:lgtm:


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@Nightsd01 Nightsd01 merged commit a42a573 into master Mar 22, 2018
@Jeasmine Jeasmine deleted the extension_badge_handling branch May 12, 2020 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants