feat: added missing methods #17

Shahroz16 · 2023-01-10T03:38:24Z

closes : #20, https://github.com/customerio/issues/issues/8960
and #13

- register device - metric tracking

github-actions · 2023-01-10T03:38:37Z

Pull request title looks good 👍!

If this pull request gets merged, it will cause a new release of the software. Example: If this project's latest release version is 1.0.0. If this pull request gets merged in, the next release of this project will be 1.1.0. This pull request is not a breaking change.

All merged pull requests will eventually get deployed. But some types of pull requests will trigger a deployment (such as features and bug fixes) while some pull requests will wait to get deployed until a later time.

To merge this pull request, add the label Ready to merge to this pull request and I'll merge it for you.

This project uses a special format for pull requests titles. Expand this section to learn more (expand by clicking the ᐅ symbol on the left side of this sentence)...

This project uses a special format for pull requests titles. Don't worry, it's easy!

This pull request title should be in this format:

<type>: short description of change being made

If your pull request introduces breaking changes to the code, use this format:

<type>!: short description of breaking change

where <type> is one of the following:

feat: - A feature is being added or modified by this pull request. Use this if you made any changes to any of the features of the project.
fix: - A bug is being fixed by this pull request. Use this if you made any fixes to bugs in the project.
docs: - This pull request is making documentation changes, only.
refactor: - A change was made that doesn't fix a bug or add a feature.
test: - Adds missing tests or fixes broken tests.
style: - Changes that do not effect the code (whitespace, linting, formatting, semi-colons, etc)
perf: - Changes improve performance of the code.
build: - Changes to the build system (maven, npm, gulp, etc)
ci: - Changes to the CI build system (Travis, GitHub Actions, Circle, etc)
chore: - Other changes to project that don't modify source code or test files.
revert: - Reverts a previous commit that was made.

Examples:

feat: edit profile photo
refactor!: remove deprecated v1 endpoints
build: update npm dependencies
style: run formatter

Need more examples? Want to learn more about this format? Check out the official docs.

Note: If your pull request does multiple things such as adding a feature and makes changes to the CI server and fixes some bugs then you might want to consider splitting this pull request up into multiple smaller pull requests.

… shahroz/shared-wrapper-code

Changes the Slack channel for deployment messages to the new mobile-deployments channel See customerio/issues#9014

mrehan27

Just a few small queries, rest looks good 👍🏻

mrehan27 · 2023-02-09T15:28:14Z

ios/Classes/SwiftCustomerIoPlugin.swift

+            return
+        }
+
+        CustomerIO.shared.registerDeviceToken(token)


I ain't an iOS expert 😅 But doesn't if statement looks better here? e.g.

if let token = params[Keys.Tracking.token] as? String { CustomerIO.shared.registerDeviceToken(token) }

Mainly because I think the method is too small for early returns.

haha! I don't have any preference but I saw this being followed in RN iOS, so kept the same format. 😄

I prefer guard statements. Even beyond iOS development, the guard design pattern I find quite nice 😄. I have made suggestions in our RN SDK code reviews in the past to use the guard design pattern.

IMO, since this function is short, either solution proposed would do. When a function gets long and more complex, I think guard statements show their value more.

But even in short functions, I like to use guards.

mrehan27 · 2023-02-09T15:28:34Z

ios/Classes/SwiftCustomerIoPlugin.swift

+
+        CustomerIO.shared.trackMetric(deliveryID: deliveryId,
+                                      event: event,
+                                      deviceToken: deviceToken)


Same as above comment

#21

levibostian · 2023-02-10T18:48:04Z

android/src/main/kotlin/io/customer/customer_io/CustomerIoPlugin.kt

@@ -38,13 +40,13 @@ class CustomerIoPlugin : FlutterPlugin, MethodCallHandler, ActivityAware {
    /// when the Flutter Engine is detached from the Activity
    private lateinit var flutterCommunicationChannel: MethodChannel
    private lateinit var context: Context
-    private var activity: Activity? = null
+    private var activity: WeakReference<Activity>? = null


Suggested change

private var activity: WeakReference<Activity>? = null

private var activity: WeakReference<Activity> = WeakReference()

This feels overly complicated to me to have a WeakReference object that can be null since the object it keeps reference to can also be null.

Instead, I suggest making WeakReference never null and modify it when we want to make the Activity null.

So, we would also modify:

override fun onDetachedFromActivity() { this.activity.clear() }

Thanks, Levi, that was my first attempt too. We can't use WeakReference() but will have to use WeakReference(Activity())

I was unsure about this, so I searched more and the conclusion was It's better to initialize the WeakReference with null initially, rather than with a new Activity object. This is because the new Activity object will immediately become eligible for garbage collection, since it has no strong references to it, which could potentially lead to unexpected behavior.

By initializing the WeakReference with null, you ensure that the WeakReference does not hold a reference to an object that could be immediately eligible for garbage collection. When the plugin is attached to the host activity, you can then update the WeakReference to hold a reference to the host activity.

Ah, I see.

From the javadoc, it does appear that the constructor does not allow a null value.

Bummer, but this solution is OK.

android/src/main/kotlin/io/customer/customer_io/CustomerIoPlugin.kt

levibostian · 2023-02-10T18:50:43Z

lib/customer_io_method_channel.dart

+      if (kDebugMode) {
+        print(exception);
+      }


Is this logic something we should pull out into a Util method? To avoid copy/pasting it in many places in the code?

Have a method that contains the default logic behind how we handle caught exceptions.

That's a good point, I'll create a method in the same class.

levibostian · 2023-02-10T18:51:04Z

lib/customer_io_method_channel.dart

+      if (kDebugMode) {
+        print(exception);
+      }


See other comment in this file.

levibostian · 2023-02-10T18:53:08Z

test/customer_io_test.dart

+  @override
+  void registerDeviceToken({required String deviceToken}) {
+    // TODO: implement registerDeviceToken
+  }
+
+  @override
+  void trackMetric({required String deliveryID, required String deviceToken, required MetricEvent event}) {
+    // TODO: implement trackMetric
+  }
 }


Sorry for asking a question about a file that's already been in the code base for a while. But I want to ask, what is this file used for? Could we add a comment to this file explaining the reason behind having it?

Added comment for it.

levibostian · 2023-02-10T18:56:14Z

ios/Classes/SwiftCustomerIoPlugin.swift

+            return
+        }
+
+        CustomerIO.shared.registerDeviceToken(token)


I prefer guard statements. Even beyond iOS development, the guard design pattern I find quite nice 😄. I have made suggestions in our RN SDK code reviews in the past to use the guard design pattern.

IMO, since this function is short, either solution proposed would do. When a function gets long and more complex, I think guard statements show their value more.

But even in short functions, I like to use guards.

android/src/main/kotlin/io/customer/customer_io/CustomerIoPlugin.kt

levibostian · 2023-02-10T18:58:13Z

android/src/main/kotlin/io/customer/customer_io/CustomerIoPlugin.kt

@@ -142,9 +154,28 @@ class CustomerIoPlugin : FlutterPlugin, MethodCallHandler, ActivityAware {
        }
    }

+    private fun registerDeviceToken(params: Map<String, Any>) {


Curious, why this params a Map? I was expecting to see a String param?

Mostly for consistency, Since other methods are sending Map we have now a utility method that verifies the map and its keys.

Also, if we ever wanted to send more params along with token we won't have to change the signature of the method or implementation but rather just add another pair to the map.

But I agree that this looks like overkill and I might deviate away from this as well if we get 1 more method that just takes 1 string parameter.

I understand how it would be convenient that if we ever added another parameter besides token, the signature would not need modified.

I would agree with you that this does feel like overkill because (1) this method is private so modifying the parameters would not introduce breaking changes to the code base and (2) if you never add a new parameter besides token, this solution feels boilerplate and overkill.

I also prefer not using Map as you lose compiler checks. Instead of a strongly typed parameter of type String, the compiler wouldn't as easily find potential bugs with Map<String, Any>.

android/src/main/kotlin/io/customer/customer_io/CustomerIoPlugin.kt

levibostian · 2023-02-10T19:02:09Z

android/src/main/kotlin/io/customer/customer_io/CustomerIoPlugin.kt

    private fun setDeviceAttributes(params: Map<String, Any>) {
-        val attributes =
-            params.getProperty<Map<String, Any>>(Keys.Tracking.ATTRIBUTES) ?: emptyMap()
+        val attributes = params.getProperty<Map<String, Any>>(Keys.Tracking.ATTRIBUTES) ?: return


Since it's relevant to this PR, I wanted to put a comment here.

This is an example of using the guard design pattern in Kotlin 😄

levibostian

Looks good besides 1 new PR conversation I started in lib/customer_io_method_channel.dart file.

levibostian · 2023-02-13T17:39:45Z

lib/customer_io_method_channel.dart

@@ -91,11 +89,10 @@ class CustomerIOMethodChannel extends CustomerIOPlatform {
        TrackingConsts.identifier: identifier,
        TrackingConsts.attributes: attributes
      };
+      methodChannel.invokeMethod("method", "");


What is this? Added by mistake maybe?

Oh yes, such a good carch, that was added by mistake while I tried to convert Map to String. This line is leftover from there, will remove it. And create another PR with changes.

## [1.0.0-alpha.8](1.0.0-alpha.7...1.0.0-alpha.8) (2023-02-13) ### Features * added missing methods ([#17](#17)) ([73f29e6](73f29e6))

ami-ci · 2023-02-13T20:01:36Z

🎉 This PR is included in version 1.0.0-alpha.8 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

## 1.0.0-beta.1 (2023-03-07) ### Features * added missing methods ([#17](#17)) ([73f29e6](73f29e6)) * added SDK config ([#1](#1)) ([e8ed7dd](e8ed7dd)) * tracking and in-app added ([#2](#2)) ([c23f2d9](c23f2d9)) ### Bug Fixes * formatting issues ([d67fa7e](d67fa7e)) * in-app remove gist org id ([#19](#19)) ([ce4cc9e](ce4cc9e)) * missing methods and extra dependency ([2c5deca](2c5deca)) * obj-c bindings issue ([0dbe4ef](0dbe4ef)) * plugin version in user-agent ([a10e482](a10e482)) * typo fixed ([#9](#9)) ([a5107df](a5107df)), closes [#7](#7) [#8](#8) * updated android dependency to auto update ([#24](#24)) ([040cef2](040cef2)) * updated icon and typo ([57c6eef](57c6eef))

ami-ci · 2023-03-07T11:14:18Z

🎉 This PR is included in version 1.0.0-beta.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

## 1.0.0 (2023-04-03) ### Features * added missing methods ([#17](#17)) ([73f29e6](73f29e6)) * added SDK config ([#1](#1)) ([e8ed7dd](e8ed7dd)) * tracking and in-app added ([#2](#2)) ([c23f2d9](c23f2d9)) ### Bug Fixes * add test coverage and refactored scripts ([#34](#34)) ([f7f2387](f7f2387)) * formatting issues ([d67fa7e](d67fa7e)) * in-app remove gist org id ([#19](#19)) ([ce4cc9e](ce4cc9e)) * missing methods and extra dependency ([2c5deca](2c5deca)) * obj-c bindings issue ([0dbe4ef](0dbe4ef)) * plugin version in user-agent ([a10e482](a10e482)) * release script typo ([2a8b7ae](2a8b7ae)) * typo fixed ([#9](#9)) ([a5107df](a5107df)), closes [#7](#7) [#8](#8) * updated android dependency to auto update ([#24](#24)) ([040cef2](040cef2)) * updated icon and typo ([57c6eef](57c6eef))

Shahroz16 added 5 commits January 3, 2023 01:21

removed shared code

c1a9af8

refactored name

40cface

added features

fa45559

- register device - metric tracking

added support for env variables

da47520

added support for missing methods in iOS

75c6a19

Shahroz16 and others added 6 commits February 9, 2023 17:05

Merge branch 'alpha' of github.com:customerio/customerio-flutter into…

e461a8d

… shahroz/shared-wrapper-code

clean up

53ef79c

ci: changes Slack deployment message channel

dc624b7

Changes the Slack channel for deployment messages to the new mobile-deployments channel See customerio/issues#9014

updated ios dependency

1b52d1e

updating version manually until we fix auto updating

b4caf68

enable debugging

306a3e8

Shahroz16 marked this pull request as ready for review February 9, 2023 14:48

Shahroz16 requested a review from mrehan27 February 9, 2023 14:48

Shahroz16 self-assigned this Feb 9, 2023

mrehan27 approved these changes Feb 9, 2023

View reviewed changes

PR suggestions

ab398fd

#21

Shahroz16 requested a review from levibostian February 9, 2023 22:24

levibostian reviewed Feb 10, 2023

View reviewed changes

PR suggestions

d3e383a

Shahroz16 mentioned this pull request Feb 13, 2023

CustomerIO/MessagingPushFCM is broken #22

Closed

levibostian approved these changes Feb 13, 2023

View reviewed changes

clean up

ab75941

Shahroz16 merged commit 73f29e6 into alpha Feb 13, 2023

ami-ci pushed a commit that referenced this pull request Feb 13, 2023

chore: prepare for 1.0.0-alpha.8

511745f

## [1.0.0-alpha.8](1.0.0-alpha.7...1.0.0-alpha.8) (2023-02-13) ### Features * added missing methods ([#17](#17)) ([73f29e6](73f29e6))

ami-ci added the released on @alpha label Feb 13, 2023

ami-ci added the released on @beta label Mar 7, 2023

Shahroz16 deleted the shahroz/shared-wrapper-code branch March 7, 2023 11:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added missing methods #17

feat: added missing methods #17

Shahroz16 commented Jan 10, 2023 •

edited

Loading

github-actions bot commented Jan 10, 2023 •

edited

Loading

Examples:

mrehan27 left a comment

mrehan27 Feb 9, 2023

Shahroz16 Feb 9, 2023

levibostian Feb 10, 2023

mrehan27 Feb 9, 2023

levibostian Feb 10, 2023

Shahroz16 Feb 10, 2023

levibostian Feb 13, 2023

levibostian Feb 10, 2023

Shahroz16 Feb 10, 2023

levibostian Feb 10, 2023

levibostian Feb 10, 2023

Shahroz16 Feb 10, 2023

levibostian Feb 10, 2023

levibostian Feb 10, 2023

Shahroz16 Feb 10, 2023

levibostian Feb 13, 2023

levibostian Feb 10, 2023

levibostian left a comment

levibostian Feb 13, 2023

Shahroz16 Feb 13, 2023

ami-ci commented Feb 13, 2023

ami-ci commented Mar 7, 2023

	private var activity: WeakReference<Activity>? = null
	private var activity: WeakReference<Activity> = WeakReference()

feat: added missing methods #17

feat: added missing methods #17

Conversation

Shahroz16 commented Jan 10, 2023 • edited Loading

github-actions bot commented Jan 10, 2023 • edited Loading

Examples:

mrehan27 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

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

Choose a reason for hiding this comment

levibostian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ami-ci commented Feb 13, 2023

ami-ci commented Mar 7, 2023

Shahroz16 commented Jan 10, 2023 •

edited

Loading

github-actions bot commented Jan 10, 2023 •

edited

Loading