Firebase and Segment integration #291

rnr · 2024-02-21T15:39:10Z

This PR implements both Analytics options - Firebase+GoogleAnalytics standalone library and Segment+GoogleAnalytics.
In the config we have the ANALYTICS_SOURCE parameter:

FIREBASE:
    ENABLED: true
    ANALYTICS_SOURCE: 'segment'

This parameter may have values:

firebase
segment

Depending on this parameter, the application uses the appropriate library. For this purpose, FirebaseAnalyticsService and SegmentAnalyticsService are added. Both services are added to the DI.

chore: added abstract layer for push notifications

chore: test braze

… if firebase enabled

… file

…nd Segment managers

saeedbashir · 2024-02-22T03:39:17Z

Core/Core/Configuration/Config/SegmentConfig.swift

+        enabled = dictionary[SegmentKeys.enabled] as? Bool == true
+        writeKey = dictionary[SegmentKeys.writeKey] as? String ?? ""


Let's make enabled also dependent on writeKey like the writeKey shouldn't be empty.

saeedbashir · 2024-02-22T03:41:22Z

Authorization/Authorization/Presentation/Registration/SignUpViewModel.swift

@@ -131,7 +131,7 @@ public class SignUpViewModel: ObservableObject {
                fields: validateFields,
                isSocial: externalToken != nil
            )
-            analytics.setUserID("\(user.id)")
+            analytics.identify(id: "\(user.id)", username: user.username, email: user.email)


How about moving the analytics.identify(.....) call to showMainOrWhatsNewScreen so you don't have to worry about from where the main screen is being shown? At the moment it's being called from multiple places.

I don't really like the idea of passing User variable inside the router. However, we may come back to this question when we work with analytics (this is not part of this PR)

saeedbashir · 2024-02-22T03:46:19Z

OpenEdX/Managers/FirebaseAnalyticsManager/FirebaseAnalyticsManager.swift

+import Firebase
+import Core
+
+class FirebaseAnalyticsManager: AnalyticsService {


Although we have collaborated on it and name is also fine, but as it's more like a tracker instead of manager, how about renaming it to FirebaseAnalyticsTracker.

If it sounds good then the same will go for Segment.

What about FirebaseAnalyticsService?

saeedbashir · 2024-02-22T03:49:17Z

OpenEdX/Managers/SegmentAnalyticsManager/SegmentAnalyticsManager.swift

+                        .trackApplicationLifecycleEvents(true)
+                        .flushInterval(10)
+        analytics = Analytics(configuration: configuration)
+        if config.firebase.isAnalyticsSourceSegment {


Oh it got missed, please update condition for config.firebase.enabled as well so in case of missing keys app will not crash.

saeedbashir

LGTM 👍

volodymyr-chekyrta · 2024-02-24T15:49:31Z

OpenEdX/AppDelegate.swift

-            if let configuration = config.firebase.firebaseOptions {
-                FirebaseApp.configure(options: configuration)
-                Crashlytics.crashlytics().setCrashlyticsCollectionEnabled(true)
-            }


We can't remove that part of the code, Crashlytics will stop working.

@volodymyr-chekyrta We've added FirebaseCrashlyticsCollectionEnabled in the info.plist, I believe this will do the job.

Thank you! Got it, let me check it out.

volodymyr-chekyrta · 2024-02-24T15:53:29Z

OpenEdX/Managers/FirebaseAnalyticsService/FirebaseAnalyticsService.swift

+    public init(config: ConfigProtocol) {
+        guard config.firebase.enabled && config.firebase.isAnalyticsSourceFirebase else { return }
+
+        FirebaseApp.configure()


Since we don't have the GoogleServices.plist file to configure Firebase, we have to pass the configuration to the configure function.
https://github.com/openedx/openedx-app-ios/blob/main/OpenEdX/AppDelegate.swift#L42

@volodymyr-chekyrta The config processing script is already generating the GoogleService-Info.plist file by reading the values from the config for FIREBASE.

I see, thank you!

volodymyr-chekyrta

@rnr, LGTM 👍
@saeedbashir, thanks for comments

rnr added 21 commits January 24, 2024 15:22

chore: added abstract layer for push notifications

a6fc40d

Merge pull request #2 from touchapp/push_notifications_abstract

077ae85

chore: added abstract layer for push notifications

chore: abstract layer for deep linking

c87f7cb

chore: test braze experiment

3f4049c

chore: test braze

chore: added segment package

8bc62e7

chore: returned back default values

2d2ade2

Merge branch 'develop' into test/test_braze

dfdf6a4

chore: added firebase as swift package

c164495

chore: fixed upload crashlytic dsym build phase script

f9c6813

Merge branch 'develop' into test/segment_plus_firebase

53f73d5

chore: added AnalyticsService protocol and instances

5a61d28

chore: cleanup code

5d9dd2b

chore: renamed google analytics to firebase analitics. added checking…

c48416c

… if firebase enabled

chore: added FirebaseManager to setup standalone Firebase in separate…

59b5be7

… file

chore: moved analytics segment var into SegmentManager

78a4f3d

chore: moved AnalyticsService protocol implementation into Firebase a…

81984f9

…nd Segment managers

chore: moved setup into managers init methods, renamed managers

70285cc

chore: added check if services are enabled while init

17b8ee7

chore: renamed managers group

1c4939b

chore: cleanup code

5eb6e20

Merge branch 'develop' into test/segment_plus_firebase

d5d27ae

rnr requested review from forgotvas, saeedbashir and volodymyr-chekyrta February 21, 2024 15:39

This was linked to issues Feb 21, 2024

[iOS] Third-party Push Notifications tool SDK integration #250

Closed

[iOS] Segment SDK Implementation #272

Closed

rnr added 2 commits February 21, 2024 17:07

chore: re-added Segment package through https source

af25b64

chore: generated mocks

7c67115

saeedbashir reviewed Feb 22, 2024

View reviewed changes

refactor: address review feedback, added test for segment config

f40b7f2

rnr requested a review from saeedbashir February 22, 2024 11:54

forgotvas previously approved these changes Feb 22, 2024

View reviewed changes

saeedbashir previously approved these changes Feb 22, 2024

View reviewed changes

volodymyr-chekyrta reviewed Feb 24, 2024

View reviewed changes

Merge branch 'develop' into test/segment_plus_firebase

00a6c86

rnr dismissed stale reviews from saeedbashir and forgotvas via 00a6c86 February 26, 2024 09:14

volodymyr-chekyrta previously approved these changes Feb 26, 2024

View reviewed changes

chore: auto-generated mocks

fb22073

rnr dismissed volodymyr-chekyrta’s stale review via fb22073 February 26, 2024 10:01

forgotvas approved these changes Feb 26, 2024

View reviewed changes

rnr merged commit cf65ce9 into openedx:develop Feb 26, 2024
3 checks passed

rnr deleted the test/segment_plus_firebase branch February 26, 2024 11:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Firebase and Segment integration #291

Firebase and Segment integration #291

rnr commented Feb 21, 2024 •

edited

Loading

saeedbashir Feb 22, 2024

rnr Feb 22, 2024

saeedbashir Feb 22, 2024

rnr Feb 22, 2024

saeedbashir Feb 22, 2024

saeedbashir Feb 22, 2024

rnr Feb 22, 2024

rnr Feb 22, 2024

saeedbashir Feb 22, 2024

rnr Feb 22, 2024

saeedbashir left a comment

volodymyr-chekyrta Feb 24, 2024

saeedbashir Feb 26, 2024

volodymyr-chekyrta Feb 26, 2024

volodymyr-chekyrta Feb 24, 2024

saeedbashir Feb 26, 2024

volodymyr-chekyrta Feb 26, 2024

volodymyr-chekyrta left a comment

		enabled = dictionary[SegmentKeys.enabled] as? Bool == true
		writeKey = dictionary[SegmentKeys.writeKey] as? String ?? ""

Firebase and Segment integration #291

Firebase and Segment integration #291

Conversation

rnr commented Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saeedbashir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

volodymyr-chekyrta left a comment

Choose a reason for hiding this comment

rnr commented Feb 21, 2024 •

edited

Loading