From 5c999ccdd69c317b229887b890be3b27f3847027 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Thu, 9 Nov 2023 09:23:14 +0100 Subject: [PATCH 01/39] refactor: extract json mapping logic to Kotlin object --- .../parsely/parselyandroid/JsonSerializer.kt | 26 +++++++++++++++++++ .../parselyandroid/ParselyTracker.java | 23 ++-------------- 2 files changed, 28 insertions(+), 21 deletions(-) create mode 100644 parsely/src/main/java/com/parsely/parselyandroid/JsonSerializer.kt diff --git a/parsely/src/main/java/com/parsely/parselyandroid/JsonSerializer.kt b/parsely/src/main/java/com/parsely/parselyandroid/JsonSerializer.kt new file mode 100644 index 00000000..95d6b314 --- /dev/null +++ b/parsely/src/main/java/com/parsely/parselyandroid/JsonSerializer.kt @@ -0,0 +1,26 @@ +package com.parsely.parselyandroid + +import com.fasterxml.jackson.databind.ObjectMapper +import java.io.IOException +import java.io.StringWriter + +internal object JsonSerializer { + /** + * Encode an event Map as JSON. + * + * @param map The Map object to encode as JSON. + * @return The JSON-encoded value of `map`. + */ + fun toJson(map: Map): String? { + val mapper = ObjectMapper() + var ret: String? = null + try { + val strWriter = StringWriter() + mapper.writeValue(strWriter, map) + ret = strWriter.toString() + } catch (e: IOException) { + e.printStackTrace() + } + return ret + } +} diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index 6a72bbb0..57359dc6 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -450,10 +450,10 @@ private void sendBatchRequest(ArrayList> events) { PLog("Debug mode on. Not sending to Parse.ly"); purgeEventsQueue(); } else { - new ParselyAPIConnection(this).execute(ROOT_URL + "mobileproxy", JsonEncode(batchMap)); + new ParselyAPIConnection(this).execute(ROOT_URL + "mobileproxy", JsonSerializer.INSTANCE.toJson(batchMap)); PLog("Requested %s", ROOT_URL); } - PLog("POST Data %s", JsonEncode(batchMap)); + PLog("POST Data %s", JsonSerializer.INSTANCE.toJson(batchMap)); } /** @@ -472,25 +472,6 @@ void purgeEventsQueue() { localStorageRepository.purgeStoredQueue(); } - /** - * Encode an event Map as JSON. - * - * @param map The Map object to encode as JSON. - * @return The JSON-encoded value of `map`. - */ - private String JsonEncode(Map map) { - ObjectMapper mapper = new ObjectMapper(); - String ret = null; - try { - StringWriter strWriter = new StringWriter(); - mapper.writeValue(strWriter, map); - ret = strWriter.toString(); - } catch (IOException e) { - e.printStackTrace(); - } - return ret; - } - /** * Start the timer to flush events to Parsely. *

From de701d2cc30574810d191aac3a11d8dfca397ac9 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Thu, 9 Nov 2023 09:59:01 +0100 Subject: [PATCH 02/39] refactor: introduce `SendEvents` use case with same logic as `ParselyTracker#sendBatchRequest` --- .../parselyandroid/ParselyTracker.java | 30 ++++------------ .../com/parsely/parselyandroid/SendEvents.kt | 34 +++++++++++++++++++ 2 files changed, 40 insertions(+), 24 deletions(-) create mode 100644 parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index 57359dc6..6849d662 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -27,13 +27,8 @@ import androidx.lifecycle.LifecycleEventObserver; import androidx.lifecycle.ProcessLifecycleOwner; -import com.fasterxml.jackson.databind.ObjectMapper; - -import java.io.IOException; -import java.io.StringWriter; import java.util.ArrayList; import java.util.Formatter; -import java.util.HashMap; import java.util.Map; import java.util.Timer; import java.util.UUID; @@ -51,8 +46,8 @@ public class ParselyTracker { private static final int DEFAULT_FLUSH_INTERVAL_SECS = 60; private static final int DEFAULT_ENGAGEMENT_INTERVAL_MILLIS = 10500; @SuppressWarnings("StringOperationCanBeSimplified") -// private static final String ROOT_URL = "http://10.0.2.2:5001/".intern(); // emulator localhost - private static final String ROOT_URL = "https://p1.parsely.com/".intern(); +// static final String ROOT_URL = "http://10.0.2.2:5001/".intern(); // emulator localhost + static final String ROOT_URL = "https://p1.parsely.com/".intern(); private boolean isDebug; private final Context context; private final Timer timer; @@ -68,6 +63,8 @@ public class ParselyTracker { private final LocalStorageRepository localStorageRepository; @NonNull private final InMemoryBuffer inMemoryBuffer; + @NonNull + private final SendEvents sendEvents; /** * Create a new ParselyTracker instance. @@ -85,6 +82,7 @@ protected ParselyTracker(String siteId, int flushInterval, Context c) { } return Unit.INSTANCE; }); + sendEvents = new SendEvents(this, localStorageRepository); // get the adkey straight away on instantiation timer = new Timer(); @@ -437,23 +435,7 @@ public void flushEventQueue() { * @param events The list of event dictionaries to serialize */ private void sendBatchRequest(ArrayList> events) { - if (events == null || events.size() == 0) { - return; - } - PLog("Sending request with %d events", events.size()); - - // Put in a Map for the proxy server - Map batchMap = new HashMap<>(); - batchMap.put("events", events); - - if (isDebug) { - PLog("Debug mode on. Not sending to Parse.ly"); - purgeEventsQueue(); - } else { - new ParselyAPIConnection(this).execute(ROOT_URL + "mobileproxy", JsonSerializer.INSTANCE.toJson(batchMap)); - PLog("Requested %s", ROOT_URL); - } - PLog("POST Data %s", JsonSerializer.INSTANCE.toJson(batchMap)); + sendEvents.invoke(isDebug); } /** diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt new file mode 100644 index 00000000..476d5257 --- /dev/null +++ b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt @@ -0,0 +1,34 @@ +package com.parsely.parselyandroid + +import com.parsely.parselyandroid.JsonSerializer.toJson + +internal class SendEvents( + private val parselyTracker: ParselyTracker, + private val localStorageRepository: LocalStorageRepository +) { + + operator fun invoke(isDebug: Boolean) { + val eventsToSend = localStorageRepository.getStoredQueue() + + if (eventsToSend.isEmpty()) { + return + } + ParselyTracker.PLog("Sending request with %d events", eventsToSend.size) + + val batchMap: MutableMap = HashMap() + batchMap["events"] = eventsToSend + val jsonPayload = toJson(batchMap) + + if (isDebug) { + ParselyTracker.PLog("Debug mode on. Not sending to Parse.ly") + localStorageRepository.purgeStoredQueue() + } else { + ParselyTracker.PLog("Requested %s", ParselyTracker.ROOT_URL) + ParselyAPIConnection(parselyTracker).execute( + ParselyTracker.ROOT_URL + "mobileproxy", + jsonPayload + ) + } + ParselyTracker.PLog("POST Data %s", toJson(batchMap)) + } +} From 4ae78b20778cc2e051938507cf5f7d337be9cbef Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Thu, 9 Nov 2023 10:25:00 +0100 Subject: [PATCH 03/39] refactor: migrate `ParselyAPIConnection` to Coroutines --- .../parselyandroid/ParselyAPIConnection.kt | 62 +++++++++---------- .../parselyandroid/ParselyTracker.java | 2 +- .../com/parsely/parselyandroid/SendEvents.kt | 42 +++++++------ 3 files changed, 54 insertions(+), 52 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt b/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt index 8d0634bd..7f8a3d07 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt @@ -13,50 +13,48 @@ See the License for the specific language governing permissions and limitations under the License. */ -@file:Suppress("DEPRECATION") package com.parsely.parselyandroid -import android.os.AsyncTask +import com.parsely.parselyandroid.ParselyTracker.ROOT_URL import java.net.HttpURLConnection import java.net.URL - -internal class ParselyAPIConnection(private val tracker: ParselyTracker) : AsyncTask() { +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.withContext + +internal class ParselyAPIConnection @JvmOverloads constructor( + private val url: String, + private val tracker: ParselyTracker, + private val dispatcher: CoroutineDispatcher = Dispatchers.IO +) { private var exception: Exception? = null - @Deprecated("Deprecated in Java") - override fun doInBackground(vararg data: String?): Void? { - var connection: HttpURLConnection? = null - try { - if (data.size == 1) { // non-batched (since no post data is included) - connection = URL(data[0]).openConnection() as HttpURLConnection - connection.inputStream - } else if (data.size == 2) { // batched (post data included) - connection = URL(data[0]).openConnection() as HttpURLConnection - connection.doOutput = true // Triggers POST (aka silliest interface ever) + suspend fun send(payload: String) { + withContext(dispatcher) { + val connection: HttpURLConnection? + try { + connection = URL(url).openConnection() as HttpURLConnection + connection.doOutput = true connection.setRequestProperty("Content-Type", "application/json") val output = connection.outputStream - output.write(data[1]?.toByteArray()) + output.write(payload.toByteArray()) output.close() connection.inputStream + } catch (ex: Exception) { + exception = ex } - } catch (ex: Exception) { - exception = ex - } - return null - } - @Deprecated("Deprecated in Java") - override fun onPostExecute(result: Void?) { - if (exception != null) { - ParselyTracker.PLog("Pixel request exception") - ParselyTracker.PLog(exception.toString()) - } else { - ParselyTracker.PLog("Pixel request success") - - // only purge the queue if the request was successful - tracker.purgeEventsQueue() - ParselyTracker.PLog("Event queue empty, flush timer cleared.") - tracker.stopFlushTimer() + if (exception != null) { + ParselyTracker.PLog("Pixel request exception") + ParselyTracker.PLog(exception.toString()) + } else { + ParselyTracker.PLog("Pixel request success") + + // only purge the queue if the request was successful + tracker.purgeEventsQueue() + ParselyTracker.PLog("Event queue empty, flush timer cleared.") + tracker.stopFlushTimer() + } } } } diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index 6849d662..3d7e01d7 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -82,7 +82,7 @@ protected ParselyTracker(String siteId, int flushInterval, Context c) { } return Unit.INSTANCE; }); - sendEvents = new SendEvents(this, localStorageRepository); + sendEvents = new SendEvents(this, localStorageRepository, new ParselyAPIConnection(ROOT_URL + "mobileproxy", this), ParselyCoroutineScopeKt.getSdkScope()); // get the adkey straight away on instantiation timer = new Timer(); diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt index 476d5257..c18201cb 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt @@ -1,34 +1,38 @@ package com.parsely.parselyandroid import com.parsely.parselyandroid.JsonSerializer.toJson +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.launch internal class SendEvents( private val parselyTracker: ParselyTracker, - private val localStorageRepository: LocalStorageRepository + private val localStorageRepository: LocalStorageRepository, + private val parselyAPIConnection: ParselyAPIConnection, + private val scope: CoroutineScope ) { operator fun invoke(isDebug: Boolean) { - val eventsToSend = localStorageRepository.getStoredQueue() + scope.launch { + val eventsToSend = localStorageRepository.getStoredQueue() - if (eventsToSend.isEmpty()) { - return - } - ParselyTracker.PLog("Sending request with %d events", eventsToSend.size) + if (eventsToSend.isEmpty()) { + return@launch + } + ParselyTracker.PLog("Sending request with %d events", eventsToSend.size) + + val batchMap: MutableMap = HashMap() + batchMap["events"] = eventsToSend + val jsonPayload = toJson(batchMap).orEmpty() - val batchMap: MutableMap = HashMap() - batchMap["events"] = eventsToSend - val jsonPayload = toJson(batchMap) + if (isDebug) { + ParselyTracker.PLog("Debug mode on. Not sending to Parse.ly") + localStorageRepository.purgeStoredQueue() + } else { + ParselyTracker.PLog("Requested %s", ParselyTracker.ROOT_URL) - if (isDebug) { - ParselyTracker.PLog("Debug mode on. Not sending to Parse.ly") - localStorageRepository.purgeStoredQueue() - } else { - ParselyTracker.PLog("Requested %s", ParselyTracker.ROOT_URL) - ParselyAPIConnection(parselyTracker).execute( - ParselyTracker.ROOT_URL + "mobileproxy", - jsonPayload - ) + parselyAPIConnection.send(jsonPayload) + } + ParselyTracker.PLog("POST Data %s", toJson(batchMap)) } - ParselyTracker.PLog("POST Data %s", toJson(batchMap)) } } From 920e995306ce010948823edbe7b04642d837b442 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Thu, 9 Nov 2023 10:34:34 +0100 Subject: [PATCH 04/39] test: update `ParselyAPIConnectionTest` to support Coroutines --- .../ParselyAPIConnectionTest.kt | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt index 815abc93..80d461b1 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt @@ -1,6 +1,8 @@ package com.parsely.parselyandroid import androidx.test.core.app.ApplicationProvider +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer import org.assertj.core.api.Assertions.assertThat @@ -13,7 +15,6 @@ import org.robolectric.annotation.LooperMode import org.robolectric.shadows.ShadowLooper.shadowMainLooper @RunWith(RobolectricTestRunner::class) -@LooperMode(LooperMode.Mode.PAUSED) class ParselyAPIConnectionTest { private lateinit var sut: ParselyAPIConnection @@ -23,7 +24,7 @@ class ParselyAPIConnectionTest { @Before fun setUp() { - sut = ParselyAPIConnection(tracker) + sut = ParselyAPIConnection(url, tracker) } @After @@ -32,12 +33,12 @@ class ParselyAPIConnectionTest { } @Test - fun `given successful response, when making connection without any events, then make GET request`() { + fun `given successful response, when making connection without any events, then make GET request`() = runTest { // given mockServer.enqueue(MockResponse().setResponseCode(200)) // when - sut.execute(url).get() + sut.send("") shadowMainLooper().idle(); // then @@ -49,13 +50,13 @@ class ParselyAPIConnectionTest { } @Test - fun `given successful response, when making connection with events, then make POST request with JSON Content-Type header`() { + fun `given successful response, when making connection with events, then make POST request with JSON Content-Type header`() = runTest { // given mockServer.enqueue(MockResponse().setResponseCode(200)) // when - sut.execute(url, pixelPayload).get() - shadowMainLooper().idle(); + sut.send(pixelPayload) + runCurrent() // then assertThat(mockServer.takeRequest()).satisfies({ @@ -66,14 +67,14 @@ class ParselyAPIConnectionTest { } @Test - fun `given successful response, when request is made, then purge events queue and stop flush timer`() { + fun `given successful response, when request is made, then purge events queue and stop flush timer`() = runTest { // given mockServer.enqueue(MockResponse().setResponseCode(200)) tracker.events.add(mapOf("idsite" to "example.com")) // when - sut.execute(url).get() - shadowMainLooper().idle(); + sut.send(pixelPayload) + runCurrent() // then assertThat(tracker.events).isEmpty() @@ -81,15 +82,15 @@ class ParselyAPIConnectionTest { } @Test - fun `given unsuccessful response, when request is made, then do not purge events queue and do not stop flush timer`() { + fun `given unsuccessful response, when request is made, then do not purge events queue and do not stop flush timer`() = runTest { // given mockServer.enqueue(MockResponse().setResponseCode(400)) val sampleEvents = mapOf("idsite" to "example.com") tracker.events.add(sampleEvents) // when - sut.execute(url).get() - shadowMainLooper().idle(); + sut.send(pixelPayload) + runCurrent() // then assertThat(tracker.events).containsExactly(sampleEvents) From 17e7ecd107e48cdd0a96833e4917fff29f95c68b Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Thu, 9 Nov 2023 10:47:34 +0100 Subject: [PATCH 05/39] tests: remove unit tests that checks for `GET` request on empty payload This behavior was intentionally removed in 4ae78b2. It's not present on iOS. --- .../parselyandroid/ParselyAPIConnectionTest.kt | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt index 80d461b1..8356a65e 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt @@ -32,23 +32,6 @@ class ParselyAPIConnectionTest { mockServer.shutdown() } - @Test - fun `given successful response, when making connection without any events, then make GET request`() = runTest { - // given - mockServer.enqueue(MockResponse().setResponseCode(200)) - - // when - sut.send("") - shadowMainLooper().idle(); - - // then - val request = mockServer.takeRequest() - assertThat(request).satisfies({ - assertThat(it.method).isEqualTo("GET") - assertThat(it.failure).isNull() - }) - } - @Test fun `given successful response, when making connection with events, then make POST request with JSON Content-Type header`() = runTest { // given From 5bff337cd1b37ac22571aac23c52fa83d2e66821 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Thu, 9 Nov 2023 10:59:09 +0100 Subject: [PATCH 06/39] tests: update `ROOT_URL` before `ParselyTracker` constructor Now, as `ROOT_URL` is used inside the constructor, we have to change this field **before** the `ParselyTracker` is initialized --- .../java/com/parsely/parselyandroid/FunctionalTests.kt | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt b/parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt index 48bf7503..ead1058c 100644 --- a/parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt +++ b/parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt @@ -214,13 +214,12 @@ class FunctionalTests { activity: Activity, flushInterval: Duration = defaultFlushInterval ): ParselyTracker { + val field: Field = ParselyTracker::class.java.getDeclaredField("ROOT_URL") + field.isAccessible = true + field.set(this, url) return ParselyTracker.sharedInstance( siteId, flushInterval.inWholeSeconds.toInt(), activity.application - ).apply { - val f: Field = this::class.java.getDeclaredField("ROOT_URL") - f.isAccessible = true - f.set(this, url) - } + ) } private companion object { From 51c89b9f077d145d23794f5b720649538a1ec3d4 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Thu, 9 Nov 2023 11:04:59 +0100 Subject: [PATCH 07/39] feat: closing the connection after successful request --- .../java/com/parsely/parselyandroid/ParselyAPIConnection.kt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt b/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt index 7f8a3d07..00195ca5 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt @@ -31,7 +31,7 @@ internal class ParselyAPIConnection @JvmOverloads constructor( suspend fun send(payload: String) { withContext(dispatcher) { - val connection: HttpURLConnection? + var connection: HttpURLConnection? = null try { connection = URL(url).openConnection() as HttpURLConnection connection.doOutput = true @@ -42,6 +42,8 @@ internal class ParselyAPIConnection @JvmOverloads constructor( connection.inputStream } catch (ex: Exception) { exception = ex + } finally { + connection?.disconnect() } if (exception != null) { From 479d262c378746202f114ed498e5d94c0f8aba5a Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Thu, 9 Nov 2023 11:12:00 +0100 Subject: [PATCH 08/39] feat: close the connection after successful request --- .../main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt | 1 - 1 file changed, 1 deletion(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt b/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt index 00195ca5..401cecc1 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt @@ -15,7 +15,6 @@ */ package com.parsely.parselyandroid -import com.parsely.parselyandroid.ParselyTracker.ROOT_URL import java.net.HttpURLConnection import java.net.URL import kotlinx.coroutines.CoroutineDispatcher From 742fc5d9d5f66f2d5a3261ae1a52da0361a2fa09 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Thu, 9 Nov 2023 12:28:07 +0100 Subject: [PATCH 09/39] feat: make `ParselyAPIConnection` return `Result` --- .../parselyandroid/ParselyAPIConnection.kt | 26 +++-- .../ParselyAPIConnectionTest.kt | 94 ++++++++++--------- 2 files changed, 61 insertions(+), 59 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt b/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt index 401cecc1..6f2789ed 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt @@ -26,10 +26,8 @@ internal class ParselyAPIConnection @JvmOverloads constructor( private val tracker: ParselyTracker, private val dispatcher: CoroutineDispatcher = Dispatchers.IO ) { - private var exception: Exception? = null - - suspend fun send(payload: String) { - withContext(dispatcher) { + suspend fun send(payload: String): Result { + return withContext(dispatcher) { var connection: HttpURLConnection? = null try { connection = URL(url).openConnection() as HttpURLConnection @@ -40,22 +38,20 @@ internal class ParselyAPIConnection @JvmOverloads constructor( output.close() connection.inputStream } catch (ex: Exception) { - exception = ex + ParselyTracker.PLog("Pixel request exception") + ParselyTracker.PLog(ex.toString()) + return@withContext Result.failure(ex) } finally { connection?.disconnect() } - if (exception != null) { - ParselyTracker.PLog("Pixel request exception") - ParselyTracker.PLog(exception.toString()) - } else { - ParselyTracker.PLog("Pixel request success") + ParselyTracker.PLog("Pixel request success") - // only purge the queue if the request was successful - tracker.purgeEventsQueue() - ParselyTracker.PLog("Event queue empty, flush timer cleared.") - tracker.stopFlushTimer() - } + // only purge the queue if the request was successful + tracker.purgeEventsQueue() + ParselyTracker.PLog("Event queue empty, flush timer cleared.") + tracker.stopFlushTimer() + Result.success(Unit) } } } diff --git a/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt index 8356a65e..dad4698e 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt @@ -5,14 +5,13 @@ import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer +import okio.IOException import org.assertj.core.api.Assertions.assertThat import org.junit.After import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner -import org.robolectric.annotation.LooperMode -import org.robolectric.shadows.ShadowLooper.shadowMainLooper @RunWith(RobolectricTestRunner::class) class ParselyAPIConnectionTest { @@ -33,52 +32,59 @@ class ParselyAPIConnectionTest { } @Test - fun `given successful response, when making connection with events, then make POST request with JSON Content-Type header`() = runTest { - // given - mockServer.enqueue(MockResponse().setResponseCode(200)) - - // when - sut.send(pixelPayload) - runCurrent() - - // then - assertThat(mockServer.takeRequest()).satisfies({ - assertThat(it.method).isEqualTo("POST") - assertThat(it.headers["Content-Type"]).isEqualTo("application/json") - assertThat(it.body.readUtf8()).isEqualTo(pixelPayload) - }) - } + fun `given successful response, when making connection with events, then make POST request with JSON Content-Type header`() = + runTest { + // given + mockServer.enqueue(MockResponse().setResponseCode(200)) + + // when + val result = sut.send(pixelPayload) + runCurrent() + + // then + assertThat(mockServer.takeRequest()).satisfies({ + assertThat(it.method).isEqualTo("POST") + assertThat(it.headers["Content-Type"]).isEqualTo("application/json") + assertThat(it.body.readUtf8()).isEqualTo(pixelPayload) + }) + assertThat(result.isSuccess).isTrue + } @Test - fun `given successful response, when request is made, then purge events queue and stop flush timer`() = runTest { - // given - mockServer.enqueue(MockResponse().setResponseCode(200)) - tracker.events.add(mapOf("idsite" to "example.com")) - - // when - sut.send(pixelPayload) - runCurrent() - - // then - assertThat(tracker.events).isEmpty() - assertThat(tracker.flushTimerStopped).isTrue - } + fun `given successful response, when request is made, then purge events queue and stop flush timer`() = + runTest { + // given + mockServer.enqueue(MockResponse().setResponseCode(200)) + tracker.events.add(mapOf("idsite" to "example.com")) + + // when + val result = sut.send(pixelPayload) + runCurrent() + + // then + assertThat(tracker.events).isEmpty() + assertThat(tracker.flushTimerStopped).isTrue + assertThat(result.isSuccess).isTrue + } @Test - fun `given unsuccessful response, when request is made, then do not purge events queue and do not stop flush timer`() = runTest { - // given - mockServer.enqueue(MockResponse().setResponseCode(400)) - val sampleEvents = mapOf("idsite" to "example.com") - tracker.events.add(sampleEvents) - - // when - sut.send(pixelPayload) - runCurrent() - - // then - assertThat(tracker.events).containsExactly(sampleEvents) - assertThat(tracker.flushTimerStopped).isFalse - } + fun `given unsuccessful response, when request is made, then do not purge events queue and do not stop flush timer`() = + runTest { + // given + mockServer.enqueue(MockResponse().setResponseCode(400)) + val sampleEvents = mapOf("idsite" to "example.com") + tracker.events.add(sampleEvents) + + // when + val result = sut.send(pixelPayload) + runCurrent() + + // then + assertThat(tracker.events).containsExactly(sampleEvents) + assertThat(tracker.flushTimerStopped).isFalse + assertThat(result.isFailure).isTrue + assertThat(result.exceptionOrNull()).isInstanceOf(IOException::class.java) + } companion object { val pixelPayload: String = From 9668a91dba26b8015d20fda5198de5ee58a07fc5 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Thu, 9 Nov 2023 12:41:00 +0100 Subject: [PATCH 10/39] refactor: move handling HTTP request results to `SendEvents` The responsibility of `ParselyAPIConnection` should be "sending http requests" only. Orchestrating flow of events will be handled by `SendEvents` use case. This reduces coupling. --- .../parselyandroid/ParselyAPIConnection.kt | 9 --------- .../parsely/parselyandroid/ParselyTracker.java | 2 +- .../com/parsely/parselyandroid/SendEvents.kt | 16 ++++++++++++++-- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt b/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt index 6f2789ed..f9f621e5 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt @@ -23,7 +23,6 @@ import kotlinx.coroutines.withContext internal class ParselyAPIConnection @JvmOverloads constructor( private val url: String, - private val tracker: ParselyTracker, private val dispatcher: CoroutineDispatcher = Dispatchers.IO ) { suspend fun send(payload: String): Result { @@ -38,19 +37,11 @@ internal class ParselyAPIConnection @JvmOverloads constructor( output.close() connection.inputStream } catch (ex: Exception) { - ParselyTracker.PLog("Pixel request exception") - ParselyTracker.PLog(ex.toString()) return@withContext Result.failure(ex) } finally { connection?.disconnect() } - ParselyTracker.PLog("Pixel request success") - - // only purge the queue if the request was successful - tracker.purgeEventsQueue() - ParselyTracker.PLog("Event queue empty, flush timer cleared.") - tracker.stopFlushTimer() Result.success(Unit) } } diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index 3d7e01d7..1a049607 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -82,7 +82,7 @@ protected ParselyTracker(String siteId, int flushInterval, Context c) { } return Unit.INSTANCE; }); - sendEvents = new SendEvents(this, localStorageRepository, new ParselyAPIConnection(ROOT_URL + "mobileproxy", this), ParselyCoroutineScopeKt.getSdkScope()); + sendEvents = new SendEvents(this, localStorageRepository, new ParselyAPIConnection(ROOT_URL + "mobileproxy"), ParselyCoroutineScopeKt.getSdkScope()); // get the adkey straight away on instantiation timer = new Timer(); diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt index c18201cb..a1b5be18 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt @@ -24,15 +24,27 @@ internal class SendEvents( batchMap["events"] = eventsToSend val jsonPayload = toJson(batchMap).orEmpty() + ParselyTracker.PLog("POST Data %s", jsonPayload) + if (isDebug) { ParselyTracker.PLog("Debug mode on. Not sending to Parse.ly") localStorageRepository.purgeStoredQueue() } else { ParselyTracker.PLog("Requested %s", ParselyTracker.ROOT_URL) - parselyAPIConnection.send(jsonPayload) + .fold( + onSuccess = { + ParselyTracker.PLog("Pixel request success") + parselyTracker.purgeEventsQueue() + ParselyTracker.PLog("Event queue empty, flush timer cleared.") + parselyTracker.stopFlushTimer() + }, + onFailure = { + ParselyTracker.PLog("Pixel request exception") + ParselyTracker.PLog(it.toString()) + } + ) } - ParselyTracker.PLog("POST Data %s", toJson(batchMap)) } } } From b7c98a745f2c22bc91d762e8533ac94f045f6ed7 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Thu, 9 Nov 2023 16:48:14 +0100 Subject: [PATCH 11/39] tests: update unit tests to reflect current scope of work of `ParselyAPIConnection` --- .../ParselyAPIConnectionTest.kt | 44 ++----------------- 1 file changed, 4 insertions(+), 40 deletions(-) diff --git a/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt index dad4698e..135ca268 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt @@ -1,6 +1,6 @@ package com.parsely.parselyandroid -import androidx.test.core.app.ApplicationProvider +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import okhttp3.mockwebserver.MockResponse @@ -13,17 +13,17 @@ import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner +@OptIn(ExperimentalCoroutinesApi::class) @RunWith(RobolectricTestRunner::class) class ParselyAPIConnectionTest { private lateinit var sut: ParselyAPIConnection private val mockServer = MockWebServer() private val url = mockServer.url("").toString() - private val tracker = FakeTracker() @Before fun setUp() { - sut = ParselyAPIConnection(url, tracker) + sut = ParselyAPIConnection(url) } @After @@ -51,37 +51,17 @@ class ParselyAPIConnectionTest { } @Test - fun `given successful response, when request is made, then purge events queue and stop flush timer`() = - runTest { - // given - mockServer.enqueue(MockResponse().setResponseCode(200)) - tracker.events.add(mapOf("idsite" to "example.com")) - - // when - val result = sut.send(pixelPayload) - runCurrent() - - // then - assertThat(tracker.events).isEmpty() - assertThat(tracker.flushTimerStopped).isTrue - assertThat(result.isSuccess).isTrue - } - - @Test - fun `given unsuccessful response, when request is made, then do not purge events queue and do not stop flush timer`() = + fun `given unsuccessful response, when request is made, then return failure with exception`() = runTest { // given mockServer.enqueue(MockResponse().setResponseCode(400)) val sampleEvents = mapOf("idsite" to "example.com") - tracker.events.add(sampleEvents) // when val result = sut.send(pixelPayload) runCurrent() // then - assertThat(tracker.events).containsExactly(sampleEvents) - assertThat(tracker.flushTimerStopped).isFalse assertThat(result.isFailure).isTrue assertThat(result.exceptionOrNull()).isInstanceOf(IOException::class.java) } @@ -90,20 +70,4 @@ class ParselyAPIConnectionTest { val pixelPayload: String = this::class.java.getResource("pixel_payload.json")?.readText().orEmpty() } - - private class FakeTracker : ParselyTracker( - "siteId", 10, ApplicationProvider.getApplicationContext() - ) { - - var flushTimerStopped = false - val events = mutableListOf>() - - override fun purgeEventsQueue() { - events.clear() - } - - override fun stopFlushTimer() { - flushTimerStopped = true - } - } } From 038c0abb716b1fa538bee74da8aaf6644d094df9 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 10 Nov 2023 13:57:10 +0100 Subject: [PATCH 12/39] refactor: use `LocalStorageRepository` methods directly Not using `ParselyTracker` as middleman, reduces complexity --- .../main/java/com/parsely/parselyandroid/ParselyTracker.java | 4 ---- .../src/main/java/com/parsely/parselyandroid/SendEvents.kt | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index 1a049607..328aef4f 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -450,10 +450,6 @@ private boolean isReachable() { return netInfo != null && netInfo.isConnectedOrConnecting(); } - void purgeEventsQueue() { - localStorageRepository.purgeStoredQueue(); - } - /** * Start the timer to flush events to Parsely. *

diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt index a1b5be18..2b4f3391 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt @@ -35,7 +35,7 @@ internal class SendEvents( .fold( onSuccess = { ParselyTracker.PLog("Pixel request success") - parselyTracker.purgeEventsQueue() + localStorageRepository.purgeStoredQueue() ParselyTracker.PLog("Event queue empty, flush timer cleared.") parselyTracker.stopFlushTimer() }, From f294f73c8d0d5dafa01944918314dc3d826f8b6f Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Sat, 11 Nov 2023 18:29:46 +0100 Subject: [PATCH 13/39] refactor: use `FlushManager` methods directly Not using `ParselyTracker` as middleman, reduces complexity --- .../main/java/com/parsely/parselyandroid/ParselyTracker.java | 2 +- .../src/main/java/com/parsely/parselyandroid/SendEvents.kt | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index 328aef4f..e1ed6205 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -82,7 +82,7 @@ protected ParselyTracker(String siteId, int flushInterval, Context c) { } return Unit.INSTANCE; }); - sendEvents = new SendEvents(this, localStorageRepository, new ParselyAPIConnection(ROOT_URL + "mobileproxy"), ParselyCoroutineScopeKt.getSdkScope()); + sendEvents = new SendEvents(flushManager, localStorageRepository, new ParselyAPIConnection(ROOT_URL + "mobileproxy"), ParselyCoroutineScopeKt.getSdkScope()); // get the adkey straight away on instantiation timer = new Timer(); diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt index 2b4f3391..05e27c86 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt @@ -5,7 +5,7 @@ import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.launch internal class SendEvents( - private val parselyTracker: ParselyTracker, + private val flushManager: FlushManager, private val localStorageRepository: LocalStorageRepository, private val parselyAPIConnection: ParselyAPIConnection, private val scope: CoroutineScope @@ -37,7 +37,7 @@ internal class SendEvents( ParselyTracker.PLog("Pixel request success") localStorageRepository.purgeStoredQueue() ParselyTracker.PLog("Event queue empty, flush timer cleared.") - parselyTracker.stopFlushTimer() + flushManager.stop() }, onFailure = { ParselyTracker.PLog("Pixel request exception") From c93b5678278006d46dda785e18474ad210ea0a25 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Sat, 11 Nov 2023 18:38:56 +0100 Subject: [PATCH 14/39] fix: on successful request, remove only sent events If an event would be added to local repository between getting stored queue from local repository and sending it in `SendEvents`, it'd be removed and never sent. This change fixes this risk by removing only events that were sent. --- .../java/com/parsely/parselyandroid/LocalStorageRepository.kt | 2 +- .../src/main/java/com/parsely/parselyandroid/SendEvents.kt | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt index d3873326..e835d1a2 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt @@ -40,7 +40,7 @@ internal open class LocalStorageRepository(private val context: Context) { persistObject(ArrayList>()) } - fun remove(toRemove: List>) { + suspend fun remove(toRemove: List?>) = mutex.withLock { persistObject(getStoredQueue() - toRemove.toSet()) } diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt index 05e27c86..bb3deaa2 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt @@ -28,14 +28,14 @@ internal class SendEvents( if (isDebug) { ParselyTracker.PLog("Debug mode on. Not sending to Parse.ly") - localStorageRepository.purgeStoredQueue() + localStorageRepository.remove(eventsToSend) } else { ParselyTracker.PLog("Requested %s", ParselyTracker.ROOT_URL) parselyAPIConnection.send(jsonPayload) .fold( onSuccess = { ParselyTracker.PLog("Pixel request success") - localStorageRepository.purgeStoredQueue() + localStorageRepository.remove(eventsToSend) ParselyTracker.PLog("Event queue empty, flush timer cleared.") flushManager.stop() }, From debd023d421dd4ca0a4406c5ac4d3aa3cea9829b Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Sat, 11 Nov 2023 18:41:05 +0100 Subject: [PATCH 15/39] style: remove unused methods of `LocalStorageRepository` --- .../parselyandroid/LocalStorageRepository.kt | 16 ---------------- .../parselyandroid/LocalStorageRepositoryTest.kt | 13 ------------- 2 files changed, 29 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt index e835d1a2..16912ea1 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt @@ -5,7 +5,6 @@ import java.io.EOFException import java.io.FileNotFoundException import java.io.ObjectInputStream import java.io.ObjectOutputStream -import kotlinx.coroutines.currentCoroutineContext import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock @@ -33,13 +32,6 @@ internal open class LocalStorageRepository(private val context: Context) { } } - /** - * Delete the stored queue from persistent storage. - */ - fun purgeStoredQueue() { - persistObject(ArrayList>()) - } - suspend fun remove(toRemove: List?>) = mutex.withLock { persistObject(getStoredQueue() - toRemove.toSet()) } @@ -71,14 +63,6 @@ internal open class LocalStorageRepository(private val context: Context) { return storedQueue } - /** - * Delete an event from the stored queue. - */ - open fun expelStoredEvent() { - val storedQueue = getStoredQueue() - storedQueue.removeAt(0) - } - /** * Save the event queue to persistent storage. */ diff --git a/parsely/src/test/java/com/parsely/parselyandroid/LocalStorageRepositoryTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/LocalStorageRepositoryTest.kt index f92b0d59..76e5f186 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/LocalStorageRepositoryTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/LocalStorageRepositoryTest.kt @@ -24,19 +24,6 @@ class LocalStorageRepositoryTest { sut = LocalStorageRepository(context) } - @Test - fun `when expelling stored event, then assert that it has no effect`() = runTest { - // given - sut.insertEvents(((1..100).map { mapOf("index" to it) })) - runCurrent() - - // when - sut.expelStoredEvent() - - // then - assertThat(sut.getStoredQueue()).hasSize(100) - } - @Test fun `given the list of events, when persisting the list, then querying the list returns the same result`() = runTest { // given From 940b2412de9c299976ba13da04237bb8b818b251 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Sat, 11 Nov 2023 19:11:51 +0100 Subject: [PATCH 16/39] tests: add tests for SendEvents usecase --- .../parsely/parselyandroid/FlushManager.kt | 8 +- .../parselyandroid/LocalStorageRepository.kt | 2 +- .../parselyandroid/ParselyAPIConnection.kt | 4 +- .../parsely/parselyandroid/SendEventsTest.kt | 206 ++++++++++++++++++ 4 files changed, 213 insertions(+), 7 deletions(-) create mode 100644 parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt diff --git a/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt b/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt index 121b6bf9..d351a181 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt @@ -13,14 +13,14 @@ import kotlinx.coroutines.launch * Handles stopping and starting the flush timer. The flush timer * controls how often we send events to Parse.ly servers. */ -internal class FlushManager( +internal open class FlushManager( private val parselyTracker: ParselyTracker, val intervalMillis: Long, private val coroutineScope: CoroutineScope ) { private var job: Job? = null - fun start() { + open fun start() { if (job?.isActive == true) return job = coroutineScope.launch { @@ -31,8 +31,8 @@ internal class FlushManager( } } - fun stop() = job?.cancel() + open fun stop() = job?.cancel() - val isRunning: Boolean + open val isRunning: Boolean get() = job?.isActive ?: false } diff --git a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt index 16912ea1..be198ef2 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt @@ -32,7 +32,7 @@ internal open class LocalStorageRepository(private val context: Context) { } } - suspend fun remove(toRemove: List?>) = mutex.withLock { + open suspend fun remove(toRemove: List?>) = mutex.withLock { persistObject(getStoredQueue() - toRemove.toSet()) } diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt b/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt index f9f621e5..79ecada8 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt @@ -21,11 +21,11 @@ import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext -internal class ParselyAPIConnection @JvmOverloads constructor( +internal open class ParselyAPIConnection @JvmOverloads constructor( private val url: String, private val dispatcher: CoroutineDispatcher = Dispatchers.IO ) { - suspend fun send(payload: String): Result { + open suspend fun send(payload: String): Result { return withContext(dispatcher) { var connection: HttpURLConnection? = null try { diff --git a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt new file mode 100644 index 00000000..5bc72dd4 --- /dev/null +++ b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt @@ -0,0 +1,206 @@ +package com.parsely.parselyandroid + +import androidx.test.core.app.ApplicationProvider +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import org.assertj.core.api.Assertions.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +@OptIn(ExperimentalCoroutinesApi::class) +@RunWith(RobolectricTestRunner::class) +class SendEventsTest { + + private lateinit var sut: SendEvents + + @Test + fun `given empty local storage, when sending events, then do nothing`() = + runTest { + // given + sut = SendEvents( + FakeFlushManager(this), + FakeLocalStorageRepository(), + FakeParselyAPIConnection(), + this + ) + + // when + sut.invoke(false) + runCurrent() + + // then + assertThat(FakeLocalStorageRepository().getStoredQueue()).isEmpty() + } + + @Test + fun `given non-empty local storage and debug mode off, when sending events, then events are sent and removed from local storage`() = + runTest { + // given + val repository = FakeLocalStorageRepository().apply { + insertEvents(listOf(mapOf("test" to 123))) + } + val parselyAPIConnection = FakeParselyAPIConnection().apply { + nextResult = Result.success(Unit) + } + sut = SendEvents( + FakeFlushManager(this), + repository, + parselyAPIConnection, + this + ) + + // when + sut.invoke(false) + runCurrent() + + // then + assertThat(repository.getStoredQueue()).isEmpty() + } + + @Test + fun `given non-empty local storage and debug mode on, when sending events, then events are not sent and removed from local storage`() = + runTest { + // given + val repository = FakeLocalStorageRepository().apply { + insertEvents(listOf(mapOf("test" to 123))) + } + sut = SendEvents( + FakeFlushManager(this), + repository, + FakeParselyAPIConnection(), + this + ) + + // when + sut.invoke(true) + runCurrent() + + // then + assertThat(repository.getStoredQueue()).isEmpty() + } + + @Test + fun `given non-empty local storage and debug mode off, when sending events fails, then events are not removed from local storage`() = + runTest { + // given + val repository = FakeLocalStorageRepository().apply { + insertEvents(listOf(mapOf("test" to 123))) + } + val parselyAPIConnection = FakeParselyAPIConnection().apply { + nextResult = Result.failure(Exception()) + } + sut = SendEvents( + FakeFlushManager(this), + repository, + parselyAPIConnection, + this + ) + + // when + sut.invoke(false) + runCurrent() + + // then + assertThat(repository.getStoredQueue()).isNotEmpty + } + + @Test + fun `given non-empty local storage and debug mode off, when sending events, then flush manager is stopped`() = + runTest { + // given + val flushManager = FakeFlushManager(this) + val repository = FakeLocalStorageRepository().apply { + insertEvents(listOf(mapOf("test" to 123))) + } + val parselyAPIConnection = FakeParselyAPIConnection().apply { + nextResult = Result.success(Unit) + } + sut = SendEvents( + flushManager, + repository, + parselyAPIConnection, + this + ) + + // when + sut.invoke(false) + runCurrent() + + // then + assertThat(flushManager.stopped).isTrue + } + + @Test + fun `given non-empty local storage and debug mode off, when sending events fails, then flush manager is not stopped`() = + runTest { + // given + val flushManager = FakeFlushManager(this) + val repository = FakeLocalStorageRepository().apply { + insertEvents(listOf(mapOf("test" to 123))) + } + val parselyAPIConnection = FakeParselyAPIConnection().apply { + nextResult = Result.failure(Exception()) + } + sut = SendEvents( + flushManager, + repository, + parselyAPIConnection, + this + ) + + // when + sut.invoke(false) + runCurrent() + + // then + assertThat(flushManager.stopped).isFalse + } + + private class FakeFlushManager(scope: CoroutineScope) : FlushManager(FakeTracker(), 10, scope) { + var stopped = false + + override fun stop() { + stopped = true + } + } + + private class FakeTracker : ParselyTracker( + "siteId", 10, ApplicationProvider.getApplicationContext() + ) { + + var flushTimerStopped = false + + override fun stopFlushTimer() { + flushTimerStopped = true + } + } + + private class FakeLocalStorageRepository : + LocalStorageRepository(ApplicationProvider.getApplicationContext()) { + private var storage = emptyList?>() + + override suspend fun insertEvents(toInsert: List?>) { + storage = storage + toInsert + } + + override suspend fun remove(toRemove: List?>) { + storage = storage - toRemove.toSet() + } + + override fun getStoredQueue(): ArrayList?> { + return ArrayList(storage) + } + } + + private class FakeParselyAPIConnection : ParselyAPIConnection("") { + + var nextResult: Result? = null + + override suspend fun send(payload: String): Result { + return nextResult!! + } + } +} From fa73ccd8134f795ee6896190b4fefcc897a3e917 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Sat, 11 Nov 2023 19:15:34 +0100 Subject: [PATCH 17/39] refactor: move serialization details to `JsonSerializer` `SendEvents` shouldn't know details about structure of JSON payload. --- .../java/com/parsely/parselyandroid/JsonSerializer.kt | 8 +++++++- .../main/java/com/parsely/parselyandroid/SendEvents.kt | 6 ++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/JsonSerializer.kt b/parsely/src/main/java/com/parsely/parselyandroid/JsonSerializer.kt index 95d6b314..dde232ce 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/JsonSerializer.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/JsonSerializer.kt @@ -5,13 +5,19 @@ import java.io.IOException import java.io.StringWriter internal object JsonSerializer { + + fun toParselyEventsPayload(eventsToSend: List?>): String { + val batchMap: MutableMap = HashMap() + batchMap["events"] = eventsToSend + return toJson(batchMap).orEmpty() + } /** * Encode an event Map as JSON. * * @param map The Map object to encode as JSON. * @return The JSON-encoded value of `map`. */ - fun toJson(map: Map): String? { + private fun toJson(map: Map): String? { val mapper = ObjectMapper() var ret: String? = null try { diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt index bb3deaa2..1b1f4c96 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt @@ -1,6 +1,6 @@ package com.parsely.parselyandroid -import com.parsely.parselyandroid.JsonSerializer.toJson +import com.parsely.parselyandroid.JsonSerializer.toParselyEventsPayload import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.launch @@ -20,9 +20,7 @@ internal class SendEvents( } ParselyTracker.PLog("Sending request with %d events", eventsToSend.size) - val batchMap: MutableMap = HashMap() - batchMap["events"] = eventsToSend - val jsonPayload = toJson(batchMap).orEmpty() + val jsonPayload = toParselyEventsPayload(eventsToSend) ParselyTracker.PLog("POST Data %s", jsonPayload) From fab4007aae2588525480a8c4a9a775ff8bcdf7ec Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Sat, 11 Nov 2023 19:44:00 +0100 Subject: [PATCH 18/39] fix: do not stop flush manager if local queue is not empty This fixes a possible unwanted stop of flush manager in case that, between querying and successfully sending a event queue, a new event was added. In such case, we should not stop the queue. --- .../com/parsely/parselyandroid/SendEvents.kt | 4 ++- .../parsely/parselyandroid/SendEventsTest.kt | 30 ++++++++++++++++++- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt index 1b1f4c96..532c0c1f 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt @@ -35,7 +35,9 @@ internal class SendEvents( ParselyTracker.PLog("Pixel request success") localStorageRepository.remove(eventsToSend) ParselyTracker.PLog("Event queue empty, flush timer cleared.") - flushManager.stop() + if (localStorageRepository.getStoredQueue().isEmpty()) { + flushManager.stop() + } }, onFailure = { ParselyTracker.PLog("Pixel request exception") diff --git a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt index 5bc72dd4..12451a70 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt @@ -159,6 +159,34 @@ class SendEventsTest { assertThat(flushManager.stopped).isFalse } + @Test + fun `given non-empty local storage and debug mode off, when storage is not empty after successful event, then flush manager is not stopped`() = + runTest { + // given + val flushManager = FakeFlushManager(this) + val repository = object : FakeLocalStorageRepository() { + override fun getStoredQueue(): ArrayList?> { + return ArrayList(listOf(mapOf("test" to 123))) + } + } + val parselyAPIConnection = FakeParselyAPIConnection().apply { + nextResult = Result.success(Unit) + } + sut = SendEvents( + flushManager, + repository, + parselyAPIConnection, + this + ) + + // when + sut.invoke(false) + runCurrent() + + // then + assertThat(flushManager.stopped).isFalse + } + private class FakeFlushManager(scope: CoroutineScope) : FlushManager(FakeTracker(), 10, scope) { var stopped = false @@ -178,7 +206,7 @@ class SendEventsTest { } } - private class FakeLocalStorageRepository : + private open class FakeLocalStorageRepository : LocalStorageRepository(ApplicationProvider.getApplicationContext()) { private var storage = emptyList?>() From 4237fcdfa59c2b8ed9139f5c170be7eb9b491fe4 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Sat, 11 Nov 2023 19:55:18 +0100 Subject: [PATCH 19/39] fix: add mutual execution for `SendEvents#invoke` Make the `SendEvents` run once at a time, to prevent a scenario, when we send multiple requests with the same events. In current state of SDK, it could happen when `FlushManager` counts to next flush interval **and** user moves app to the background at the same time. --- .../com/parsely/parselyandroid/SendEvents.kt | 64 ++++++++++--------- 1 file changed, 35 insertions(+), 29 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt index 532c0c1f..a28fa940 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt @@ -3,6 +3,8 @@ package com.parsely.parselyandroid import com.parsely.parselyandroid.JsonSerializer.toParselyEventsPayload import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.launch +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock internal class SendEvents( private val flushManager: FlushManager, @@ -11,39 +13,43 @@ internal class SendEvents( private val scope: CoroutineScope ) { + private val mutex = Mutex() + operator fun invoke(isDebug: Boolean) { scope.launch { - val eventsToSend = localStorageRepository.getStoredQueue() + mutex.withLock { + val eventsToSend = localStorageRepository.getStoredQueue() - if (eventsToSend.isEmpty()) { - return@launch - } - ParselyTracker.PLog("Sending request with %d events", eventsToSend.size) - - val jsonPayload = toParselyEventsPayload(eventsToSend) - - ParselyTracker.PLog("POST Data %s", jsonPayload) - - if (isDebug) { - ParselyTracker.PLog("Debug mode on. Not sending to Parse.ly") - localStorageRepository.remove(eventsToSend) - } else { - ParselyTracker.PLog("Requested %s", ParselyTracker.ROOT_URL) - parselyAPIConnection.send(jsonPayload) - .fold( - onSuccess = { - ParselyTracker.PLog("Pixel request success") - localStorageRepository.remove(eventsToSend) - ParselyTracker.PLog("Event queue empty, flush timer cleared.") - if (localStorageRepository.getStoredQueue().isEmpty()) { - flushManager.stop() + if (eventsToSend.isEmpty()) { + return@launch + } + ParselyTracker.PLog("Sending request with %d events", eventsToSend.size) + + val jsonPayload = toParselyEventsPayload(eventsToSend) + + ParselyTracker.PLog("POST Data %s", jsonPayload) + + if (isDebug) { + ParselyTracker.PLog("Debug mode on. Not sending to Parse.ly") + localStorageRepository.remove(eventsToSend) + } else { + ParselyTracker.PLog("Requested %s", ParselyTracker.ROOT_URL) + parselyAPIConnection.send(jsonPayload) + .fold( + onSuccess = { + ParselyTracker.PLog("Pixel request success") + localStorageRepository.remove(eventsToSend) + ParselyTracker.PLog("Event queue empty, flush timer cleared.") + if (localStorageRepository.getStoredQueue().isEmpty()) { + flushManager.stop() + } + }, + onFailure = { + ParselyTracker.PLog("Pixel request exception") + ParselyTracker.PLog(it.toString()) } - }, - onFailure = { - ParselyTracker.PLog("Pixel request exception") - ParselyTracker.PLog(it.toString()) - } - ) + ) + } } } } From 69f12b3c94598e4b11fd585de5f24b29ea3581d9 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Sat, 11 Nov 2023 20:02:52 +0100 Subject: [PATCH 20/39] refactor: remove `FlushQueue` AsyncTask The operations covered by this AsyncTask are no longer needed to run on background thread. The responsibility of checking network state is moved to `flushEvents` and stopping flush timer in case of empty queue - to `SendEvents` --- .../parselyandroid/ParselyTracker.java | 27 ++++--------------- .../com/parsely/parselyandroid/SendEvents.kt | 1 + .../parsely/parselyandroid/SendEventsTest.kt | 19 +++++++++++++ 3 files changed, 25 insertions(+), 22 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index e1ed6205..0df53ffe 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -501,29 +501,12 @@ public int storedEventsCount() { return ar.size(); } - private class FlushQueue extends AsyncTask { - @Override - protected synchronized Void doInBackground(Void... params) { - ArrayList> storedQueue = localStorageRepository.getStoredQueue(); - PLog("%d events in stored queue", storedEventsCount()); - // in case both queues have been flushed and app quits, don't crash - if (storedQueue.isEmpty()) { - stopFlushTimer(); - return null; - } - if (!isReachable()) { - PLog("Network unreachable. Not flushing."); - return null; - } - - PLog("Flushing queue"); - sendBatchRequest(storedQueue); - return null; - } - } - void flushEvents() { - new FlushQueue().execute(); + if (!isReachable()) { + PLog("Network unreachable. Not flushing."); + return; + } + sendEvents.invoke(isDebug); } } diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt index a28fa940..4172debb 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt @@ -21,6 +21,7 @@ internal class SendEvents( val eventsToSend = localStorageRepository.getStoredQueue() if (eventsToSend.isEmpty()) { + flushManager.stop() return@launch } ParselyTracker.PLog("Sending request with %d events", eventsToSend.size) diff --git a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt index 12451a70..485b354a 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt @@ -187,6 +187,25 @@ class SendEventsTest { assertThat(flushManager.stopped).isFalse } + @Test + fun `given empty local storage, when invoked, then flush manager is stopped`() = runTest { + // given + val flushManager = FakeFlushManager(this) + sut = SendEvents( + flushManager, + FakeLocalStorageRepository(), + FakeParselyAPIConnection(), + this + ) + + // when + sut.invoke(false) + runCurrent() + + // then + assertThat(flushManager.stopped).isTrue() + } + private class FakeFlushManager(scope: CoroutineScope) : FlushManager(FakeTracker(), 10, scope) { var stopped = false From ca330abe8cfe674e27cac440e9ac668233f29c89 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Sat, 11 Nov 2023 20:05:29 +0100 Subject: [PATCH 21/39] style: remove unused methods BREAKING CHANGE: this commit breaks API contract by removing `stopFlushTimer` but it was never an intention to allow clients to stop flush timer under any conditions. The lifecycle of timer is handled by the SDK internally. --- .../parselyandroid/ParselyTracker.java | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index 0df53ffe..1da832fc 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -426,18 +426,6 @@ public void flushEventQueue() { // no-op } - /** - * Send the batched event request to Parsely. - *

- * Creates a POST request containing the JSON encoding of the event queue. - * Sends this request to Parse.ly servers. - * - * @param events The list of event dictionaries to serialize - */ - private void sendBatchRequest(ArrayList> events) { - sendEvents.invoke(isDebug); - } - /** * Returns whether the network is accessible and Parsely is reachable. * @@ -470,13 +458,6 @@ public boolean flushTimerIsActive() { return flushManager.isRunning(); } - /** - * Stop the event queue flush timer. - */ - public void stopFlushTimer() { - flushManager.stop(); - } - @NonNull private String generatePixelId() { return UUID.randomUUID().toString(); From 8d4edd4658889a25a6cbbfacac9fef59cd9974f3 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Sat, 11 Nov 2023 21:18:27 +0100 Subject: [PATCH 22/39] fix: make `LocalStorageRepository#getStoredQueue` thread safe and off main thread BREAKING CHANGE: this commit removes `queueSize` and `storedEventsCount` methods. They were never a part of documented public API and added probably only for the need of example app. They were also not safe, as they were triggering I/O operation on the main thread. --- .../main/java/com/example/MainActivity.java | 21 +++------------ example/src/main/res/layout/activity_main.xml | 19 ++------------ .../parselyandroid/LocalStorageRepository.kt | 3 ++- .../parselyandroid/ParselyTracker.java | 26 ++----------------- .../com/parsely/parselyandroid/SdkInit.kt | 18 +++++++++++++ .../parselyandroid/InMemoryBufferTest.kt | 2 +- .../LocalStorageRepositoryTest.kt | 4 +-- .../parsely/parselyandroid/SendEventsTest.kt | 10 ++----- 8 files changed, 32 insertions(+), 71 deletions(-) create mode 100644 parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt diff --git a/example/src/main/java/com/example/MainActivity.java b/example/src/main/java/com/example/MainActivity.java index ee286ef8..6fff5e6c 100644 --- a/example/src/main/java/com/example/MainActivity.java +++ b/example/src/main/java/com/example/MainActivity.java @@ -30,33 +30,18 @@ protected void onCreate(Bundle savedInstanceState) { // Set debugging to true so we don't actually send things to Parse.ly ParselyTracker.sharedInstance().setDebug(true); - final TextView queueView = (TextView) findViewById(R.id.queue_size); - queueView.setText(String.format("Queued events: %d", ParselyTracker.sharedInstance().queueSize())); - - final TextView storedView = (TextView) findViewById(R.id.stored_size); - storedView.setText(String.format("Stored events: %d", ParselyTracker.sharedInstance().storedEventsCount())); - final TextView intervalView = (TextView) findViewById(R.id.interval); - storedView.setText(String.format("Flush interval: %d", ParselyTracker.sharedInstance().getFlushInterval())); updateEngagementStrings(); - final TextView views[] = new TextView[3]; - views[0] = queueView; - views[1] = storedView; - views[2] = intervalView; + final TextView views[] = new TextView[1]; + views[0] = intervalView; final Handler mHandler = new Handler() { @Override public void handleMessage(Message msg) { TextView[] v = (TextView[]) msg.obj; - TextView qView = v[0]; - qView.setText(String.format("Queued events: %d", ParselyTracker.sharedInstance().queueSize())); - - TextView sView = v[1]; - sView.setText(String.format("Stored events: %d", ParselyTracker.sharedInstance().storedEventsCount())); - - TextView iView = v[2]; + TextView iView = v[0]; if (ParselyTracker.sharedInstance().flushTimerIsActive()) { iView.setText(String.format("Flush Interval: %d", ParselyTracker.sharedInstance().getFlushInterval())); } else { diff --git a/example/src/main/res/layout/activity_main.xml b/example/src/main/res/layout/activity_main.xml index ce23ecdd..e86a9223 100644 --- a/example/src/main/res/layout/activity_main.xml +++ b/example/src/main/res/layout/activity_main.xml @@ -68,26 +68,11 @@ android:onClick="trackReset" android:text="@string/button_reset_video" /> - - - - - \ No newline at end of file + diff --git a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt index be198ef2..2727ba4b 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt @@ -41,7 +41,8 @@ internal open class LocalStorageRepository(private val context: Context) { * * @return The stored queue of events. */ - open fun getStoredQueue(): ArrayList?> { + open suspend fun getStoredQueue(): ArrayList?> = mutex.withLock { + var storedQueue: ArrayList?> = ArrayList() try { val fis = context.applicationContext.openFileInput(STORAGE_KEY) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index 1da832fc..7ce0ebc0 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -19,7 +19,6 @@ import android.content.Context; import android.net.ConnectivityManager; import android.net.NetworkInfo; -import android.os.AsyncTask; import androidx.annotation.NonNull; import androidx.annotation.Nullable; @@ -27,7 +26,6 @@ import androidx.lifecycle.LifecycleEventObserver; import androidx.lifecycle.ProcessLifecycleOwner; -import java.util.ArrayList; import java.util.Formatter; import java.util.Map; import java.util.Timer; @@ -88,9 +86,8 @@ protected ParselyTracker(String siteId, int flushInterval, Context c) { timer = new Timer(); isDebug = false; - if (localStorageRepository.getStoredQueue().size() > 0) { - startFlushTimer(); - } + final SdkInit sdkInit = new SdkInit(ParselyCoroutineScopeKt.getSdkScope(), localStorageRepository, flushManager); + sdkInit.initialize(); ProcessLifecycleOwner.get().getLifecycle().addObserver( (LifecycleEventObserver) (lifecycleOwner, event) -> { @@ -463,25 +460,6 @@ private String generatePixelId() { return UUID.randomUUID().toString(); } - /** - * Get the number of events waiting to be flushed to Parsely. - * - * @return The number of events waiting to be flushed to Parsely. - */ - public int queueSize() { - return localStorageRepository.getStoredQueue().size(); - } - - /** - * Get the number of events stored in persistent storage. - * - * @return The number of events stored in persistent storage. - */ - public int storedEventsCount() { - ArrayList> ar = localStorageRepository.getStoredQueue(); - return ar.size(); - } - void flushEvents() { if (!isReachable()) { PLog("Network unreachable. Not flushing."); diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt b/parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt new file mode 100644 index 00000000..6e5de8c1 --- /dev/null +++ b/parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt @@ -0,0 +1,18 @@ +package com.parsely.parselyandroid + +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.launch + +internal class SdkInit( + private val scope: CoroutineScope, + private val localStorageRepository: LocalStorageRepository, + private val flushManager: FlushManager, +) { + fun initialize() { + scope.launch { + if (localStorageRepository.getStoredQueue().isNotEmpty()) { + flushManager.start() + } + } + } +} diff --git a/parsely/src/test/java/com/parsely/parselyandroid/InMemoryBufferTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/InMemoryBufferTest.kt index 66bc9614..71266c46 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/InMemoryBufferTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/InMemoryBufferTest.kt @@ -87,7 +87,7 @@ internal class InMemoryBufferTest { events.addAll(toInsert) } - override fun getStoredQueue(): ArrayList?> { + override suspend fun getStoredQueue(): ArrayList?> { return ArrayList(events) } } diff --git a/parsely/src/test/java/com/parsely/parselyandroid/LocalStorageRepositoryTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/LocalStorageRepositoryTest.kt index 76e5f186..47a60057 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/LocalStorageRepositoryTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/LocalStorageRepositoryTest.kt @@ -38,7 +38,7 @@ class LocalStorageRepositoryTest { } @Test - fun `given no locally stored list, when requesting stored queue, then return an empty list`() { + fun `given no locally stored list, when requesting stored queue, then return an empty list`() = runTest { assertThat(sut.getStoredQueue()).isEmpty() } @@ -79,7 +79,7 @@ class LocalStorageRepositoryTest { } @Test - fun `given stored file with serialized events, when querying the queue, then list has expected events`() { + fun `given stored file with serialized events, when querying the queue, then list has expected events`() = runTest { // given val file = File(context.filesDir.path + "/parsely-events.ser") File(ClassLoader.getSystemResource("valid-java-parsely-events.ser")?.path!!).copyTo(file) diff --git a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt index 485b354a..f3a0991f 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt @@ -165,7 +165,7 @@ class SendEventsTest { // given val flushManager = FakeFlushManager(this) val repository = object : FakeLocalStorageRepository() { - override fun getStoredQueue(): ArrayList?> { + override suspend fun getStoredQueue(): ArrayList?> { return ArrayList(listOf(mapOf("test" to 123))) } } @@ -217,12 +217,6 @@ class SendEventsTest { private class FakeTracker : ParselyTracker( "siteId", 10, ApplicationProvider.getApplicationContext() ) { - - var flushTimerStopped = false - - override fun stopFlushTimer() { - flushTimerStopped = true - } } private open class FakeLocalStorageRepository : @@ -237,7 +231,7 @@ class SendEventsTest { storage = storage - toRemove.toSet() } - override fun getStoredQueue(): ArrayList?> { + override suspend fun getStoredQueue(): ArrayList?> { return ArrayList(storage) } } From 643defe4c926e8c1db644fc25aa764c1daea1cd0 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Sat, 11 Nov 2023 21:20:21 +0100 Subject: [PATCH 23/39] fix: do not create a deadlock As `getStoredQueue` is now using `mutex`, we cannot run it in the scope of the same `Mutex#withLock` as it'll conflict and lock both operations. --- .../parselyandroid/LocalStorageRepository.kt | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt index 2727ba4b..038d38d6 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt @@ -32,8 +32,12 @@ internal open class LocalStorageRepository(private val context: Context) { } } - open suspend fun remove(toRemove: List?>) = mutex.withLock { - persistObject(getStoredQueue() - toRemove.toSet()) + open suspend fun remove(toRemove: List?>) { + val storedEvents = getStoredQueue() + + mutex.withLock { + persistObject(storedEvents - toRemove.toSet()) + } } /** @@ -67,8 +71,12 @@ internal open class LocalStorageRepository(private val context: Context) { /** * Save the event queue to persistent storage. */ - open suspend fun insertEvents(toInsert: List?>) = mutex.withLock { - persistObject(ArrayList((toInsert + getStoredQueue()).distinct())) + open suspend fun insertEvents(toInsert: List?>){ + val storedEvents = getStoredQueue() + + mutex.withLock { + persistObject(ArrayList((toInsert + storedEvents).distinct())) + } } companion object { From 39f883125ae76ab167446e1eab7ad570b8e809a1 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Mon, 13 Nov 2023 21:20:31 +0100 Subject: [PATCH 24/39] fix: do not specify context for `ParselyAPIConnection` `ParselyAPIConnection` will always run in scope of `sdkScope` which has `Dispatchers.IO` as it constant context. --- .../parselyandroid/ParselyAPIConnection.kt | 37 ++++++++----------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt b/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt index 79ecada8..91d4aa3e 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt @@ -21,28 +21,23 @@ import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext -internal open class ParselyAPIConnection @JvmOverloads constructor( - private val url: String, - private val dispatcher: CoroutineDispatcher = Dispatchers.IO -) { +internal open class ParselyAPIConnection(private val url: String) { open suspend fun send(payload: String): Result { - return withContext(dispatcher) { - var connection: HttpURLConnection? = null - try { - connection = URL(url).openConnection() as HttpURLConnection - connection.doOutput = true - connection.setRequestProperty("Content-Type", "application/json") - val output = connection.outputStream - output.write(payload.toByteArray()) - output.close() - connection.inputStream - } catch (ex: Exception) { - return@withContext Result.failure(ex) - } finally { - connection?.disconnect() - } - - Result.success(Unit) + var connection: HttpURLConnection? = null + try { + connection = URL(url).openConnection() as HttpURLConnection + connection.doOutput = true + connection.setRequestProperty("Content-Type", "application/json") + val output = connection.outputStream + output.write(payload.toByteArray()) + output.close() + connection.inputStream + } catch (ex: Exception) { + return Result.failure(ex) + } finally { + connection?.disconnect() } + + return Result.success(Unit) } } From dbc73e166e7c9b303988174a4bab7f4e30b4a113 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Mon, 13 Nov 2023 21:22:58 +0100 Subject: [PATCH 25/39] tests: fix loading an empty file for `ParselyAPIConnectionTest` --- .../com/parsely/parselyandroid/ParselyAPIConnectionTest.kt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt index 135ca268..497e5d5c 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/ParselyAPIConnectionTest.kt @@ -55,7 +55,6 @@ class ParselyAPIConnectionTest { runTest { // given mockServer.enqueue(MockResponse().setResponseCode(400)) - val sampleEvents = mapOf("idsite" to "example.com") // when val result = sut.send(pixelPayload) @@ -68,6 +67,8 @@ class ParselyAPIConnectionTest { companion object { val pixelPayload: String = - this::class.java.getResource("pixel_payload.json")?.readText().orEmpty() + ClassLoader.getSystemResource("pixel_payload.json").readText().apply { + assert(isNotBlank()) + } } } From 366dae5669de3be03dbb73dd4b84180aabe1cf87 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 12:05:48 +0100 Subject: [PATCH 26/39] refactor: rewrite SdkInit to simple function --- .../parsely/parselyandroid/ParselyTracker.java | 7 +++++-- .../java/com/parsely/parselyandroid/SdkInit.kt | 16 +++++++--------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index 7ce0ebc0..f8226291 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -86,8 +86,11 @@ protected ParselyTracker(String siteId, int flushInterval, Context c) { timer = new Timer(); isDebug = false; - final SdkInit sdkInit = new SdkInit(ParselyCoroutineScopeKt.getSdkScope(), localStorageRepository, flushManager); - sdkInit.initialize(); + SdkInitKt.initialize( + ParselyCoroutineScopeKt.getSdkScope(), + localStorageRepository, + flushManager + ); ProcessLifecycleOwner.get().getLifecycle().addObserver( (LifecycleEventObserver) (lifecycleOwner, event) -> { diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt b/parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt index 6e5de8c1..07e00cc5 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt @@ -3,16 +3,14 @@ package com.parsely.parselyandroid import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.launch -internal class SdkInit( - private val scope: CoroutineScope, - private val localStorageRepository: LocalStorageRepository, - private val flushManager: FlushManager, +internal fun initialize( + scope: CoroutineScope, + localStorageRepository: LocalStorageRepository, + flushManager: FlushManager, ) { - fun initialize() { - scope.launch { - if (localStorageRepository.getStoredQueue().isNotEmpty()) { - flushManager.start() - } + scope.launch { + if (localStorageRepository.getStoredQueue().isNotEmpty()) { + flushManager.start() } } } From acae5f688b553a94dd21ceb92a73c2a179e17752 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 12:36:35 +0100 Subject: [PATCH 27/39] refactor: introduce `FlushManager` interface Makes creating fake objects less problematic by removing a need to call real object constructor. --- .../parsely/parselyandroid/FlushManager.kt | 21 ++++++++++---- .../parselyandroid/ParselyTracker.java | 2 +- .../parselyandroid/FlushManagerTest.kt | 10 +++---- .../parsely/parselyandroid/SendEventsTest.kt | 29 ++++++++++--------- 4 files changed, 37 insertions(+), 25 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt b/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt index d351a181..1a84e7a6 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt @@ -13,14 +13,21 @@ import kotlinx.coroutines.launch * Handles stopping and starting the flush timer. The flush timer * controls how often we send events to Parse.ly servers. */ -internal open class FlushManager( +internal interface FlushManager { + fun start() + fun stop() + val isRunning: Boolean + val intervalMillis: Long +} + +internal class ParselyFlushManager( private val parselyTracker: ParselyTracker, - val intervalMillis: Long, + override val intervalMillis: Long, private val coroutineScope: CoroutineScope -) { +) : FlushManager { private var job: Job? = null - open fun start() { + override fun start() { if (job?.isActive == true) return job = coroutineScope.launch { @@ -31,8 +38,10 @@ internal open class FlushManager( } } - open fun stop() = job?.cancel() + override fun stop() { + job?.cancel() + } - open val isRunning: Boolean + override val isRunning: Boolean get() = job?.isActive ?: false } diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index f8226291..eb309c67 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -71,7 +71,7 @@ protected ParselyTracker(String siteId, int flushInterval, Context c) { context = c.getApplicationContext(); eventsBuilder = new EventsBuilder(context, siteId); localStorageRepository = new LocalStorageRepository(context); - flushManager = new FlushManager(this, flushInterval * 1000L, + flushManager = new ParselyFlushManager(this, flushInterval * 1000L, ParselyCoroutineScopeKt.getSdkScope()); inMemoryBuffer = new InMemoryBuffer(ParselyCoroutineScopeKt.getSdkScope(), localStorageRepository, () -> { if (!flushTimerIsActive()) { diff --git a/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt index cf2ef157..25f987c3 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt @@ -20,7 +20,7 @@ class FlushManagerTest { @Test fun `when timer starts and interval time passes, then flush queue`() = runTest { - sut = FlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(DEFAULT_INTERVAL_MILLIS) @@ -31,7 +31,7 @@ class FlushManagerTest { @Test fun `when timer starts and three interval time passes, then flush queue 3 times`() = runTest { - sut = FlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(3 * DEFAULT_INTERVAL_MILLIS) @@ -43,7 +43,7 @@ class FlushManagerTest { @Test fun `when timer starts and is stopped after 2 intervals passes, then flush queue 2 times`() = runTest { - sut = FlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(2 * DEFAULT_INTERVAL_MILLIS) @@ -58,7 +58,7 @@ class FlushManagerTest { @Test fun `when timer starts, is stopped before end of interval and then time of interval passes, then do not flush queue`() = runTest { - sut = FlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(DEFAULT_INTERVAL_MILLIS / 2) @@ -73,7 +73,7 @@ class FlushManagerTest { @Test fun `when timer starts, and another timer starts after some time, then flush queue according to the first start`() = runTest { - sut = FlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(DEFAULT_INTERVAL_MILLIS / 2) diff --git a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt index f3a0991f..da5ffdfb 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt @@ -21,7 +21,7 @@ class SendEventsTest { runTest { // given sut = SendEvents( - FakeFlushManager(this), + FakeFlushManager(), FakeLocalStorageRepository(), FakeParselyAPIConnection(), this @@ -46,7 +46,7 @@ class SendEventsTest { nextResult = Result.success(Unit) } sut = SendEvents( - FakeFlushManager(this), + FakeFlushManager(), repository, parselyAPIConnection, this @@ -68,7 +68,7 @@ class SendEventsTest { insertEvents(listOf(mapOf("test" to 123))) } sut = SendEvents( - FakeFlushManager(this), + FakeFlushManager(), repository, FakeParselyAPIConnection(), this @@ -93,7 +93,7 @@ class SendEventsTest { nextResult = Result.failure(Exception()) } sut = SendEvents( - FakeFlushManager(this), + FakeFlushManager(), repository, parselyAPIConnection, this @@ -111,7 +111,7 @@ class SendEventsTest { fun `given non-empty local storage and debug mode off, when sending events, then flush manager is stopped`() = runTest { // given - val flushManager = FakeFlushManager(this) + val flushManager = FakeFlushManager() val repository = FakeLocalStorageRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } @@ -137,7 +137,7 @@ class SendEventsTest { fun `given non-empty local storage and debug mode off, when sending events fails, then flush manager is not stopped`() = runTest { // given - val flushManager = FakeFlushManager(this) + val flushManager = FakeFlushManager() val repository = FakeLocalStorageRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } @@ -163,7 +163,7 @@ class SendEventsTest { fun `given non-empty local storage and debug mode off, when storage is not empty after successful event, then flush manager is not stopped`() = runTest { // given - val flushManager = FakeFlushManager(this) + val flushManager = FakeFlushManager() val repository = object : FakeLocalStorageRepository() { override suspend fun getStoredQueue(): ArrayList?> { return ArrayList(listOf(mapOf("test" to 123))) @@ -190,7 +190,7 @@ class SendEventsTest { @Test fun `given empty local storage, when invoked, then flush manager is stopped`() = runTest { // given - val flushManager = FakeFlushManager(this) + val flushManager = FakeFlushManager() sut = SendEvents( flushManager, FakeLocalStorageRepository(), @@ -206,17 +206,20 @@ class SendEventsTest { assertThat(flushManager.stopped).isTrue() } - private class FakeFlushManager(scope: CoroutineScope) : FlushManager(FakeTracker(), 10, scope) { + private class FakeFlushManager : FlushManager { var stopped = false + override fun start() { + TODO("Not implemented") + } override fun stop() { stopped = true } - } - private class FakeTracker : ParselyTracker( - "siteId", 10, ApplicationProvider.getApplicationContext() - ) { + override val isRunning + get() = TODO("Not implemented") + override val intervalMillis + get() = TODO("Not implemented") } private open class FakeLocalStorageRepository : From 77303c61969bb1418844763f83be7f0a871b9356 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 12:44:37 +0100 Subject: [PATCH 28/39] refactor: introduce `QueueManager` interface Makes creating fake objects less problematic by removing a need to call real object constructor. --- .../com/parsely/parselyandroid/InMemoryBuffer.kt | 2 +- .../parselyandroid/LocalStorageRepository.kt | 14 ++++++++++---- .../java/com/parsely/parselyandroid/SendEvents.kt | 2 +- .../parsely/parselyandroid/InMemoryBufferTest.kt | 8 +++++--- .../com/parsely/parselyandroid/SendEventsTest.kt | 3 +-- 5 files changed, 18 insertions(+), 11 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/InMemoryBuffer.kt b/parsely/src/main/java/com/parsely/parselyandroid/InMemoryBuffer.kt index c92c0c7a..619e993d 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/InMemoryBuffer.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/InMemoryBuffer.kt @@ -10,7 +10,7 @@ import kotlinx.coroutines.sync.withLock internal class InMemoryBuffer( private val coroutineScope: CoroutineScope, - private val localStorageRepository: LocalStorageRepository, + private val localStorageRepository: QueueRepository, private val onEventAddedListener: () -> Unit, ) { diff --git a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt index 038d38d6..dc7d7134 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt @@ -8,7 +8,13 @@ import java.io.ObjectOutputStream import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock -internal open class LocalStorageRepository(private val context: Context) { +interface QueueRepository { + suspend fun remove(toRemove: List?>) + suspend fun getStoredQueue(): ArrayList?> + suspend fun insertEvents(toInsert: List?>) +} + +internal class LocalStorageRepository(private val context: Context): QueueRepository { private val mutex = Mutex() @@ -32,7 +38,7 @@ internal open class LocalStorageRepository(private val context: Context) { } } - open suspend fun remove(toRemove: List?>) { + override suspend fun remove(toRemove: List?>) { val storedEvents = getStoredQueue() mutex.withLock { @@ -45,7 +51,7 @@ internal open class LocalStorageRepository(private val context: Context) { * * @return The stored queue of events. */ - open suspend fun getStoredQueue(): ArrayList?> = mutex.withLock { + override suspend fun getStoredQueue(): ArrayList?> = mutex.withLock { var storedQueue: ArrayList?> = ArrayList() try { @@ -71,7 +77,7 @@ internal open class LocalStorageRepository(private val context: Context) { /** * Save the event queue to persistent storage. */ - open suspend fun insertEvents(toInsert: List?>){ + override suspend fun insertEvents(toInsert: List?>){ val storedEvents = getStoredQueue() mutex.withLock { diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt index 4172debb..1be22b5d 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt @@ -8,7 +8,7 @@ import kotlinx.coroutines.sync.withLock internal class SendEvents( private val flushManager: FlushManager, - private val localStorageRepository: LocalStorageRepository, + private val localStorageRepository: QueueRepository, private val parselyAPIConnection: ParselyAPIConnection, private val scope: CoroutineScope ) { diff --git a/parsely/src/test/java/com/parsely/parselyandroid/InMemoryBufferTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/InMemoryBufferTest.kt index 71266c46..e4f354ff 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/InMemoryBufferTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/InMemoryBufferTest.kt @@ -1,6 +1,5 @@ package com.parsely.parselyandroid -import androidx.test.core.app.ApplicationProvider import kotlin.time.Duration.Companion.seconds import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.cancel @@ -78,8 +77,7 @@ internal class InMemoryBufferTest { assertThat(repository.getStoredQueue()).containsOnlyOnceElementsOf(events) } - class FakeLocalStorageRepository : - LocalStorageRepository(ApplicationProvider.getApplicationContext()) { + class FakeLocalStorageRepository : QueueRepository { private val events = mutableListOf?>() @@ -87,6 +85,10 @@ internal class InMemoryBufferTest { events.addAll(toInsert) } + override suspend fun remove(toRemove: List?>) { + TODO("Not implemented") + } + override suspend fun getStoredQueue(): ArrayList?> { return ArrayList(events) } diff --git a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt index da5ffdfb..677d1892 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt @@ -222,8 +222,7 @@ class SendEventsTest { get() = TODO("Not implemented") } - private open class FakeLocalStorageRepository : - LocalStorageRepository(ApplicationProvider.getApplicationContext()) { + private class FakeLocalStorageRepository : QueueRepository { private var storage = emptyList?>() override suspend fun insertEvents(toInsert: List?>) { From be74b2898171be16d0d08491a5cd664e2fb4ef79 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 12:46:15 +0100 Subject: [PATCH 29/39] refactor: introduce `QueueManager` interface Makes creating fake objects less problematic by removing a need to call real object constructor. --- .../parselyandroid/LocalStorageRepository.kt | 2 +- .../com/parsely/parselyandroid/SendEvents.kt | 10 ++++----- .../parsely/parselyandroid/SendEventsTest.kt | 22 +++++++++---------- 3 files changed, 16 insertions(+), 18 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt index dc7d7134..6bc9bd24 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt @@ -8,7 +8,7 @@ import java.io.ObjectOutputStream import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock -interface QueueRepository { +internal interface QueueRepository { suspend fun remove(toRemove: List?>) suspend fun getStoredQueue(): ArrayList?> suspend fun insertEvents(toInsert: List?>) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt index 1be22b5d..e613f13b 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt @@ -8,7 +8,7 @@ import kotlinx.coroutines.sync.withLock internal class SendEvents( private val flushManager: FlushManager, - private val localStorageRepository: QueueRepository, + private val repository: QueueRepository, private val parselyAPIConnection: ParselyAPIConnection, private val scope: CoroutineScope ) { @@ -18,7 +18,7 @@ internal class SendEvents( operator fun invoke(isDebug: Boolean) { scope.launch { mutex.withLock { - val eventsToSend = localStorageRepository.getStoredQueue() + val eventsToSend = repository.getStoredQueue() if (eventsToSend.isEmpty()) { flushManager.stop() @@ -32,16 +32,16 @@ internal class SendEvents( if (isDebug) { ParselyTracker.PLog("Debug mode on. Not sending to Parse.ly") - localStorageRepository.remove(eventsToSend) + repository.remove(eventsToSend) } else { ParselyTracker.PLog("Requested %s", ParselyTracker.ROOT_URL) parselyAPIConnection.send(jsonPayload) .fold( onSuccess = { ParselyTracker.PLog("Pixel request success") - localStorageRepository.remove(eventsToSend) + repository.remove(eventsToSend) ParselyTracker.PLog("Event queue empty, flush timer cleared.") - if (localStorageRepository.getStoredQueue().isEmpty()) { + if (repository.getStoredQueue().isEmpty()) { flushManager.stop() } }, diff --git a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt index 677d1892..cb3a2524 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt @@ -1,7 +1,5 @@ package com.parsely.parselyandroid -import androidx.test.core.app.ApplicationProvider -import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest @@ -22,7 +20,7 @@ class SendEventsTest { // given sut = SendEvents( FakeFlushManager(), - FakeLocalStorageRepository(), + FakeRepository(), FakeParselyAPIConnection(), this ) @@ -32,14 +30,14 @@ class SendEventsTest { runCurrent() // then - assertThat(FakeLocalStorageRepository().getStoredQueue()).isEmpty() + assertThat(FakeRepository().getStoredQueue()).isEmpty() } @Test fun `given non-empty local storage and debug mode off, when sending events, then events are sent and removed from local storage`() = runTest { // given - val repository = FakeLocalStorageRepository().apply { + val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } val parselyAPIConnection = FakeParselyAPIConnection().apply { @@ -64,7 +62,7 @@ class SendEventsTest { fun `given non-empty local storage and debug mode on, when sending events, then events are not sent and removed from local storage`() = runTest { // given - val repository = FakeLocalStorageRepository().apply { + val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } sut = SendEvents( @@ -86,7 +84,7 @@ class SendEventsTest { fun `given non-empty local storage and debug mode off, when sending events fails, then events are not removed from local storage`() = runTest { // given - val repository = FakeLocalStorageRepository().apply { + val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } val parselyAPIConnection = FakeParselyAPIConnection().apply { @@ -112,7 +110,7 @@ class SendEventsTest { runTest { // given val flushManager = FakeFlushManager() - val repository = FakeLocalStorageRepository().apply { + val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } val parselyAPIConnection = FakeParselyAPIConnection().apply { @@ -138,7 +136,7 @@ class SendEventsTest { runTest { // given val flushManager = FakeFlushManager() - val repository = FakeLocalStorageRepository().apply { + val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } val parselyAPIConnection = FakeParselyAPIConnection().apply { @@ -164,7 +162,7 @@ class SendEventsTest { runTest { // given val flushManager = FakeFlushManager() - val repository = object : FakeLocalStorageRepository() { + val repository = object : FakeRepository() { override suspend fun getStoredQueue(): ArrayList?> { return ArrayList(listOf(mapOf("test" to 123))) } @@ -193,7 +191,7 @@ class SendEventsTest { val flushManager = FakeFlushManager() sut = SendEvents( flushManager, - FakeLocalStorageRepository(), + FakeRepository(), FakeParselyAPIConnection(), this ) @@ -222,7 +220,7 @@ class SendEventsTest { get() = TODO("Not implemented") } - private class FakeLocalStorageRepository : QueueRepository { + private open class FakeRepository : QueueRepository { private var storage = emptyList?>() override suspend fun insertEvents(toInsert: List?>) { From a13485cdf98815ab9016de458562bd8d8d23dfde Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 12:53:08 +0100 Subject: [PATCH 30/39] refactor: introduce `RestClient` interface Makes creating fake objects less problematic by removing a need to call real object constructor. --- .../parselyandroid/ParselyAPIConnection.kt | 11 ++++++----- .../com/parsely/parselyandroid/SendEvents.kt | 4 ++-- .../parsely/parselyandroid/SendEventsTest.kt | 18 +++++++++--------- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt b/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt index 91d4aa3e..c1c1e422 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt @@ -17,12 +17,13 @@ package com.parsely.parselyandroid import java.net.HttpURLConnection import java.net.URL -import kotlinx.coroutines.CoroutineDispatcher -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.withContext -internal open class ParselyAPIConnection(private val url: String) { - open suspend fun send(payload: String): Result { +internal interface RestClient { + suspend fun send(payload: String): Result +} + +internal class ParselyAPIConnection(private val url: String) : RestClient { + override suspend fun send(payload: String): Result { var connection: HttpURLConnection? = null try { connection = URL(url).openConnection() as HttpURLConnection diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt index e613f13b..b1f06e20 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt @@ -9,7 +9,7 @@ import kotlinx.coroutines.sync.withLock internal class SendEvents( private val flushManager: FlushManager, private val repository: QueueRepository, - private val parselyAPIConnection: ParselyAPIConnection, + private val restClient: RestClient, private val scope: CoroutineScope ) { @@ -35,7 +35,7 @@ internal class SendEvents( repository.remove(eventsToSend) } else { ParselyTracker.PLog("Requested %s", ParselyTracker.ROOT_URL) - parselyAPIConnection.send(jsonPayload) + restClient.send(jsonPayload) .fold( onSuccess = { ParselyTracker.PLog("Pixel request success") diff --git a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt index cb3a2524..77be99d6 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt @@ -21,7 +21,7 @@ class SendEventsTest { sut = SendEvents( FakeFlushManager(), FakeRepository(), - FakeParselyAPIConnection(), + FakeRestClient(), this ) @@ -40,7 +40,7 @@ class SendEventsTest { val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } - val parselyAPIConnection = FakeParselyAPIConnection().apply { + val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.success(Unit) } sut = SendEvents( @@ -68,7 +68,7 @@ class SendEventsTest { sut = SendEvents( FakeFlushManager(), repository, - FakeParselyAPIConnection(), + FakeRestClient(), this ) @@ -87,7 +87,7 @@ class SendEventsTest { val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } - val parselyAPIConnection = FakeParselyAPIConnection().apply { + val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.failure(Exception()) } sut = SendEvents( @@ -113,7 +113,7 @@ class SendEventsTest { val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } - val parselyAPIConnection = FakeParselyAPIConnection().apply { + val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.success(Unit) } sut = SendEvents( @@ -139,7 +139,7 @@ class SendEventsTest { val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } - val parselyAPIConnection = FakeParselyAPIConnection().apply { + val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.failure(Exception()) } sut = SendEvents( @@ -167,7 +167,7 @@ class SendEventsTest { return ArrayList(listOf(mapOf("test" to 123))) } } - val parselyAPIConnection = FakeParselyAPIConnection().apply { + val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.success(Unit) } sut = SendEvents( @@ -192,7 +192,7 @@ class SendEventsTest { sut = SendEvents( flushManager, FakeRepository(), - FakeParselyAPIConnection(), + FakeRestClient(), this ) @@ -236,7 +236,7 @@ class SendEventsTest { } } - private class FakeParselyAPIConnection : ParselyAPIConnection("") { + private class FakeRestClient : RestClient { var nextResult: Result? = null From cb5113c8ba298cfedb33813fe0705d5aa8516d2b Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 13:14:04 +0100 Subject: [PATCH 31/39] feat: start `FlushManager` without checking state of stored queue first The `FlushManager` eventually invokes `SendEvents` which checks for the size of stored queue anyways. This change reduces unnecessary complexity and overhead. More context: https://github.com/Parsely/parsely-android/pull/92#discussion_r1396052648 --- .../parsely/parselyandroid/ParselyTracker.java | 6 +----- .../java/com/parsely/parselyandroid/SdkInit.kt | 16 ---------------- 2 files changed, 1 insertion(+), 21 deletions(-) delete mode 100644 parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index eb309c67..d67a91e0 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -86,11 +86,7 @@ protected ParselyTracker(String siteId, int flushInterval, Context c) { timer = new Timer(); isDebug = false; - SdkInitKt.initialize( - ParselyCoroutineScopeKt.getSdkScope(), - localStorageRepository, - flushManager - ); + flushManager.start(); ProcessLifecycleOwner.get().getLifecycle().addObserver( (LifecycleEventObserver) (lifecycleOwner, event) -> { diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt b/parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt deleted file mode 100644 index 07e00cc5..00000000 --- a/parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt +++ /dev/null @@ -1,16 +0,0 @@ -package com.parsely.parselyandroid - -import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.launch - -internal fun initialize( - scope: CoroutineScope, - localStorageRepository: LocalStorageRepository, - flushManager: FlushManager, -) { - scope.launch { - if (localStorageRepository.getStoredQueue().isNotEmpty()) { - flushManager.start() - } - } -} From 4b4e47116c8ea5c9e22fedc4cdd7dafbacc3dee6 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 15:21:33 +0100 Subject: [PATCH 32/39] refactor: pass `onFlush` lambda to `ParselyFlushManager` This change decouples `ParselyFlushManager` and `ParselyTracker`. It also makes `FlushManagerTest` resistant to `ParselyTracker` implementation changes. --- .../parsely/parselyandroid/FlushManager.kt | 4 +- .../parselyandroid/ParselyTracker.java | 10 ++++- .../parselyandroid/FlushManagerTest.kt | 43 ++++++++----------- 3 files changed, 27 insertions(+), 30 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt b/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt index 1a84e7a6..5026c8d8 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt @@ -21,7 +21,7 @@ internal interface FlushManager { } internal class ParselyFlushManager( - private val parselyTracker: ParselyTracker, + private val onFlush: () -> Unit, override val intervalMillis: Long, private val coroutineScope: CoroutineScope ) : FlushManager { @@ -33,7 +33,7 @@ internal class ParselyFlushManager( job = coroutineScope.launch { while (isActive) { delay(intervalMillis) - parselyTracker.flushEvents() + onFlush.invoke() } } } diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index d67a91e0..cffa9938 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -32,6 +32,7 @@ import java.util.UUID; import kotlin.Unit; +import kotlin.jvm.functions.Function0; /** * Tracks Parse.ly app views in Android apps @@ -71,7 +72,13 @@ protected ParselyTracker(String siteId, int flushInterval, Context c) { context = c.getApplicationContext(); eventsBuilder = new EventsBuilder(context, siteId); localStorageRepository = new LocalStorageRepository(context); - flushManager = new ParselyFlushManager(this, flushInterval * 1000L, + flushManager = new ParselyFlushManager(new Function0() { + @Override + public Unit invoke() { + flushEvents(); + return Unit.INSTANCE; + } + }, flushInterval * 1000L, ParselyCoroutineScopeKt.getSdkScope()); inMemoryBuffer = new InMemoryBuffer(ParselyCoroutineScopeKt.getSdkScope(), localStorageRepository, () -> { if (!flushTimerIsActive()) { @@ -466,5 +473,4 @@ void flushEvents() { } sendEvents.invoke(isDebug); } - } diff --git a/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt index 25f987c3..7621b09a 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt @@ -1,6 +1,5 @@ package com.parsely.parselyandroid -import androidx.test.core.app.ApplicationProvider import kotlin.time.Duration.Companion.seconds import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.advanceTimeBy @@ -8,42 +7,42 @@ import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import org.assertj.core.api.Assertions.assertThat import org.junit.Test -import org.junit.runner.RunWith -import org.robolectric.RobolectricTestRunner @OptIn(ExperimentalCoroutinesApi::class) -@RunWith(RobolectricTestRunner::class) class FlushManagerTest { private lateinit var sut: FlushManager - private val tracker = FakeTracker() + private var flushEventsCounter = 0 @Test fun `when timer starts and interval time passes, then flush queue`() = runTest { - sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = + ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(DEFAULT_INTERVAL_MILLIS) runCurrent() - assertThat(tracker.flushEventsCounter).isEqualTo(1) + assertThat(flushEventsCounter).isEqualTo(1) } @Test fun `when timer starts and three interval time passes, then flush queue 3 times`() = runTest { - sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = + ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(3 * DEFAULT_INTERVAL_MILLIS) runCurrent() - assertThat(tracker.flushEventsCounter).isEqualTo(3) + assertThat(flushEventsCounter).isEqualTo(3) } @Test fun `when timer starts and is stopped after 2 intervals passes, then flush queue 2 times`() = runTest { - sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = + ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(2 * DEFAULT_INTERVAL_MILLIS) @@ -52,13 +51,14 @@ class FlushManagerTest { advanceTimeBy(DEFAULT_INTERVAL_MILLIS) runCurrent() - assertThat(tracker.flushEventsCounter).isEqualTo(2) + assertThat(flushEventsCounter).isEqualTo(2) } @Test fun `when timer starts, is stopped before end of interval and then time of interval passes, then do not flush queue`() = runTest { - sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = + ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(DEFAULT_INTERVAL_MILLIS / 2) @@ -67,13 +67,14 @@ class FlushManagerTest { advanceTimeBy(DEFAULT_INTERVAL_MILLIS / 2) runCurrent() - assertThat(tracker.flushEventsCounter).isEqualTo(0) + assertThat(flushEventsCounter).isEqualTo(0) } @Test fun `when timer starts, and another timer starts after some time, then flush queue according to the first start`() = runTest { - sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = + ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(DEFAULT_INTERVAL_MILLIS / 2) @@ -82,25 +83,15 @@ class FlushManagerTest { advanceTimeBy(DEFAULT_INTERVAL_MILLIS / 2) runCurrent() - assertThat(tracker.flushEventsCounter).isEqualTo(1) + assertThat(flushEventsCounter).isEqualTo(1) advanceTimeBy(DEFAULT_INTERVAL_MILLIS / 2) runCurrent() - assertThat(tracker.flushEventsCounter).isEqualTo(1) + assertThat(flushEventsCounter).isEqualTo(1) } private companion object { val DEFAULT_INTERVAL_MILLIS: Long = 30.seconds.inWholeMilliseconds } - - class FakeTracker : ParselyTracker( - "", 0, ApplicationProvider.getApplicationContext() - ) { - var flushEventsCounter = 0 - - override fun flushEvents() { - flushEventsCounter++ - } - } } From c9872d488a070234975b32b1f605f4f46de2c626 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 15:33:33 +0100 Subject: [PATCH 33/39] refactor: rename `SendEvents` to `FlushQueue` and `isDebug` to `skipSendingEvents` --- .../{SendEvents.kt => FlushQueue.kt} | 6 ++-- .../parselyandroid/ParselyTracker.java | 6 ++-- .../{SendEventsTest.kt => FlushQueueTest.kt} | 32 +++++++++---------- 3 files changed, 22 insertions(+), 22 deletions(-) rename parsely/src/main/java/com/parsely/parselyandroid/{SendEvents.kt => FlushQueue.kt} (94%) rename parsely/src/test/java/com/parsely/parselyandroid/{SendEventsTest.kt => FlushQueueTest.kt} (83%) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt similarity index 94% rename from parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt rename to parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt index b1f06e20..867c536e 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt @@ -6,7 +6,7 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock -internal class SendEvents( +internal class FlushQueue( private val flushManager: FlushManager, private val repository: QueueRepository, private val restClient: RestClient, @@ -15,7 +15,7 @@ internal class SendEvents( private val mutex = Mutex() - operator fun invoke(isDebug: Boolean) { + operator fun invoke(skipSendingEvents: Boolean) { scope.launch { mutex.withLock { val eventsToSend = repository.getStoredQueue() @@ -30,7 +30,7 @@ internal class SendEvents( ParselyTracker.PLog("POST Data %s", jsonPayload) - if (isDebug) { + if (skipSendingEvents) { ParselyTracker.PLog("Debug mode on. Not sending to Parse.ly") repository.remove(eventsToSend) } else { diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index cffa9938..6b341e87 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -63,7 +63,7 @@ public class ParselyTracker { @NonNull private final InMemoryBuffer inMemoryBuffer; @NonNull - private final SendEvents sendEvents; + private final FlushQueue flushQueue; /** * Create a new ParselyTracker instance. @@ -87,7 +87,7 @@ public Unit invoke() { } return Unit.INSTANCE; }); - sendEvents = new SendEvents(flushManager, localStorageRepository, new ParselyAPIConnection(ROOT_URL + "mobileproxy"), ParselyCoroutineScopeKt.getSdkScope()); + flushQueue = new FlushQueue(flushManager, localStorageRepository, new ParselyAPIConnection(ROOT_URL + "mobileproxy"), ParselyCoroutineScopeKt.getSdkScope()); // get the adkey straight away on instantiation timer = new Timer(); @@ -471,6 +471,6 @@ void flushEvents() { PLog("Network unreachable. Not flushing."); return; } - sendEvents.invoke(isDebug); + flushQueue.invoke(isDebug); } } diff --git a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt similarity index 83% rename from parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt rename to parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt index 77be99d6..75a8fe9b 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt @@ -10,15 +10,15 @@ import org.robolectric.RobolectricTestRunner @OptIn(ExperimentalCoroutinesApi::class) @RunWith(RobolectricTestRunner::class) -class SendEventsTest { +class FlushQueueTest { - private lateinit var sut: SendEvents + private lateinit var sut: FlushQueue @Test fun `given empty local storage, when sending events, then do nothing`() = runTest { // given - sut = SendEvents( + sut = FlushQueue( FakeFlushManager(), FakeRepository(), FakeRestClient(), @@ -34,7 +34,7 @@ class SendEventsTest { } @Test - fun `given non-empty local storage and debug mode off, when sending events, then events are sent and removed from local storage`() = + fun `given non-empty local storage, when flushing queue with not skipping sending events, then events are sent and removed from local storage`() = runTest { // given val repository = FakeRepository().apply { @@ -43,7 +43,7 @@ class SendEventsTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.success(Unit) } - sut = SendEvents( + sut = FlushQueue( FakeFlushManager(), repository, parselyAPIConnection, @@ -59,13 +59,13 @@ class SendEventsTest { } @Test - fun `given non-empty local storage and debug mode on, when sending events, then events are not sent and removed from local storage`() = + fun `given non-empty local storage, when flushing queue with skipping sending events, then events are not sent and removed from local storage`() = runTest { // given val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } - sut = SendEvents( + sut = FlushQueue( FakeFlushManager(), repository, FakeRestClient(), @@ -81,7 +81,7 @@ class SendEventsTest { } @Test - fun `given non-empty local storage and debug mode off, when sending events fails, then events are not removed from local storage`() = + fun `given non-empty local storage, when flushing queue with not skipping sending events fails, then events are not removed from local storage`() = runTest { // given val repository = FakeRepository().apply { @@ -90,7 +90,7 @@ class SendEventsTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.failure(Exception()) } - sut = SendEvents( + sut = FlushQueue( FakeFlushManager(), repository, parselyAPIConnection, @@ -106,7 +106,7 @@ class SendEventsTest { } @Test - fun `given non-empty local storage and debug mode off, when sending events, then flush manager is stopped`() = + fun `given non-empty local storage, when flushing queue with not skipping sending events, then flush manager is stopped`() = runTest { // given val flushManager = FakeFlushManager() @@ -116,7 +116,7 @@ class SendEventsTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.success(Unit) } - sut = SendEvents( + sut = FlushQueue( flushManager, repository, parselyAPIConnection, @@ -132,7 +132,7 @@ class SendEventsTest { } @Test - fun `given non-empty local storage and debug mode off, when sending events fails, then flush manager is not stopped`() = + fun `given non-empty local storage, when flushing queue with not skipping sending events fails, then flush manager is not stopped`() = runTest { // given val flushManager = FakeFlushManager() @@ -142,7 +142,7 @@ class SendEventsTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.failure(Exception()) } - sut = SendEvents( + sut = FlushQueue( flushManager, repository, parselyAPIConnection, @@ -158,7 +158,7 @@ class SendEventsTest { } @Test - fun `given non-empty local storage and debug mode off, when storage is not empty after successful event, then flush manager is not stopped`() = + fun `given non-empty local storage, when storage is not empty after successful flushing queue with not skipping sending events, then flush manager is not stopped`() = runTest { // given val flushManager = FakeFlushManager() @@ -170,7 +170,7 @@ class SendEventsTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.success(Unit) } - sut = SendEvents( + sut = FlushQueue( flushManager, repository, parselyAPIConnection, @@ -189,7 +189,7 @@ class SendEventsTest { fun `given empty local storage, when invoked, then flush manager is stopped`() = runTest { // given val flushManager = FakeFlushManager() - sut = SendEvents( + sut = FlushQueue( flushManager, FakeRepository(), FakeRestClient(), From 269a9d64f3146e23063237b02774916cdee3ce18 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 15:40:34 +0100 Subject: [PATCH 34/39] style: improve position of logging statements in `FlushQueue` --- .../main/java/com/parsely/parselyandroid/FlushQueue.kt | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt index 867c536e..2d880159 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt @@ -24,16 +24,14 @@ internal class FlushQueue( flushManager.stop() return@launch } - ParselyTracker.PLog("Sending request with %d events", eventsToSend.size) - - val jsonPayload = toParselyEventsPayload(eventsToSend) - - ParselyTracker.PLog("POST Data %s", jsonPayload) if (skipSendingEvents) { - ParselyTracker.PLog("Debug mode on. Not sending to Parse.ly") + ParselyTracker.PLog("Debug mode on. Not sending to Parse.ly. Otherwise, would sent ${eventsToSend.size} events") repository.remove(eventsToSend) } else { + ParselyTracker.PLog("Sending request with %d events", eventsToSend.size) + val jsonPayload = toParselyEventsPayload(eventsToSend) + ParselyTracker.PLog("POST Data %s", jsonPayload) ParselyTracker.PLog("Requested %s", ParselyTracker.ROOT_URL) restClient.send(jsonPayload) .fold( From 409c42d89639e1f7139db7487ab3766bff3bf8f9 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 15:44:27 +0100 Subject: [PATCH 35/39] style: return from `FlushQueue` if `skipSendingEvents` To improve readability and align with `eventsToSend.isEmpty()` check --- parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt | 1 + 1 file changed, 1 insertion(+) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt index 2d880159..54a4f2b8 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt @@ -28,6 +28,7 @@ internal class FlushQueue( if (skipSendingEvents) { ParselyTracker.PLog("Debug mode on. Not sending to Parse.ly. Otherwise, would sent ${eventsToSend.size} events") repository.remove(eventsToSend) + return@launch } else { ParselyTracker.PLog("Sending request with %d events", eventsToSend.size) val jsonPayload = toParselyEventsPayload(eventsToSend) From 95f7a65278cd120895bea01df3e15b3e2dde0d5a Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 24 Nov 2023 12:38:03 +0100 Subject: [PATCH 36/39] style: make variables test-local where possible --- .../parsely/parselyandroid/FlushManagerTest.kt | 18 ++++++++++-------- .../parsely/parselyandroid/FlushQueueTest.kt | 18 ++++++++---------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt index 7621b09a..02842a2c 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt @@ -11,12 +11,10 @@ import org.junit.Test @OptIn(ExperimentalCoroutinesApi::class) class FlushManagerTest { - private lateinit var sut: FlushManager - private var flushEventsCounter = 0 - @Test fun `when timer starts and interval time passes, then flush queue`() = runTest { - sut = + var flushEventsCounter = 0 + val sut = ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() @@ -28,7 +26,8 @@ class FlushManagerTest { @Test fun `when timer starts and three interval time passes, then flush queue 3 times`() = runTest { - sut = + var flushEventsCounter = 0 + val sut = ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() @@ -41,7 +40,8 @@ class FlushManagerTest { @Test fun `when timer starts and is stopped after 2 intervals passes, then flush queue 2 times`() = runTest { - sut = + var flushEventsCounter = 0 + val sut = ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() @@ -57,7 +57,8 @@ class FlushManagerTest { @Test fun `when timer starts, is stopped before end of interval and then time of interval passes, then do not flush queue`() = runTest { - sut = + var flushEventsCounter = 0 + val sut = ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() @@ -73,7 +74,8 @@ class FlushManagerTest { @Test fun `when timer starts, and another timer starts after some time, then flush queue according to the first start`() = runTest { - sut = + var flushEventsCounter = 0 + val sut = ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() diff --git a/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt index 75a8fe9b..d92c5705 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt @@ -12,13 +12,11 @@ import org.robolectric.RobolectricTestRunner @RunWith(RobolectricTestRunner::class) class FlushQueueTest { - private lateinit var sut: FlushQueue - @Test fun `given empty local storage, when sending events, then do nothing`() = runTest { // given - sut = FlushQueue( + val sut = FlushQueue( FakeFlushManager(), FakeRepository(), FakeRestClient(), @@ -43,7 +41,7 @@ class FlushQueueTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.success(Unit) } - sut = FlushQueue( + val sut = FlushQueue( FakeFlushManager(), repository, parselyAPIConnection, @@ -65,7 +63,7 @@ class FlushQueueTest { val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } - sut = FlushQueue( + val sut = FlushQueue( FakeFlushManager(), repository, FakeRestClient(), @@ -90,7 +88,7 @@ class FlushQueueTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.failure(Exception()) } - sut = FlushQueue( + val sut = FlushQueue( FakeFlushManager(), repository, parselyAPIConnection, @@ -116,7 +114,7 @@ class FlushQueueTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.success(Unit) } - sut = FlushQueue( + val sut = FlushQueue( flushManager, repository, parselyAPIConnection, @@ -142,7 +140,7 @@ class FlushQueueTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.failure(Exception()) } - sut = FlushQueue( + val sut = FlushQueue( flushManager, repository, parselyAPIConnection, @@ -170,7 +168,7 @@ class FlushQueueTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.success(Unit) } - sut = FlushQueue( + val sut = FlushQueue( flushManager, repository, parselyAPIConnection, @@ -189,7 +187,7 @@ class FlushQueueTest { fun `given empty local storage, when invoked, then flush manager is stopped`() = runTest { // given val flushManager = FakeFlushManager() - sut = FlushQueue( + val sut = FlushQueue( flushManager, FakeRepository(), FakeRestClient(), From c32302f9094b5eef7342ec065ba3315b5c1a8564 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 24 Nov 2023 13:12:06 +0100 Subject: [PATCH 37/39] feat: update lock logic on local storage repo Now, instead of multiple locks in case of removing or inserting data, we do the same with a single lock. See: https://github.com/Parsely/parsely-android/pull/92#discussion_r1400768099 --- .../parselyandroid/LocalStorageRepository.kt | 41 +++++++++---------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt index 6bc9bd24..1f1f28fc 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt @@ -14,7 +14,7 @@ internal interface QueueRepository { suspend fun insertEvents(toInsert: List?>) } -internal class LocalStorageRepository(private val context: Context): QueueRepository { +internal class LocalStorageRepository(private val context: Context) : QueueRepository { private val mutex = Mutex() @@ -38,21 +38,7 @@ internal class LocalStorageRepository(private val context: Context): QueueReposi } } - override suspend fun remove(toRemove: List?>) { - val storedEvents = getStoredQueue() - - mutex.withLock { - persistObject(storedEvents - toRemove.toSet()) - } - } - - /** - * Get the stored event queue from persistent storage. - * - * @return The stored queue of events. - */ - override suspend fun getStoredQueue(): ArrayList?> = mutex.withLock { - + private fun getInternalStoredQueue(): ArrayList?> { var storedQueue: ArrayList?> = ArrayList() try { val fis = context.applicationContext.openFileInput(STORAGE_KEY) @@ -74,15 +60,26 @@ internal class LocalStorageRepository(private val context: Context): QueueReposi return storedQueue } + override suspend fun remove(toRemove: List?>) = mutex.withLock { + val storedEvents = getInternalStoredQueue() + persistObject(storedEvents - toRemove.toSet()) + } + /** - * Save the event queue to persistent storage. + * Get the stored event queue from persistent storage. + * + * @return The stored queue of events. */ - override suspend fun insertEvents(toInsert: List?>){ - val storedEvents = getStoredQueue() + override suspend fun getStoredQueue(): ArrayList?> = mutex.withLock { + getInternalStoredQueue() + } - mutex.withLock { - persistObject(ArrayList((toInsert + storedEvents).distinct())) - } + /** + * Save the event queue to persistent storage. + */ + override suspend fun insertEvents(toInsert: List?>) = mutex.withLock { + val storedEvents = getInternalStoredQueue() + persistObject(ArrayList((toInsert + storedEvents).distinct())) } companion object { From 2c0ce6e5e726cb2afc4185cf1dfa2fceb5be6a4b Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 24 Nov 2023 14:11:58 +0100 Subject: [PATCH 38/39] style: remove unnecessary `else` from `FlushQueue` --- .../com/parsely/parselyandroid/FlushQueue.kt | 39 +++++++++---------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt index 54a4f2b8..b5864e91 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt @@ -29,27 +29,26 @@ internal class FlushQueue( ParselyTracker.PLog("Debug mode on. Not sending to Parse.ly. Otherwise, would sent ${eventsToSend.size} events") repository.remove(eventsToSend) return@launch - } else { - ParselyTracker.PLog("Sending request with %d events", eventsToSend.size) - val jsonPayload = toParselyEventsPayload(eventsToSend) - ParselyTracker.PLog("POST Data %s", jsonPayload) - ParselyTracker.PLog("Requested %s", ParselyTracker.ROOT_URL) - restClient.send(jsonPayload) - .fold( - onSuccess = { - ParselyTracker.PLog("Pixel request success") - repository.remove(eventsToSend) - ParselyTracker.PLog("Event queue empty, flush timer cleared.") - if (repository.getStoredQueue().isEmpty()) { - flushManager.stop() - } - }, - onFailure = { - ParselyTracker.PLog("Pixel request exception") - ParselyTracker.PLog(it.toString()) - } - ) } + ParselyTracker.PLog("Sending request with %d events", eventsToSend.size) + val jsonPayload = toParselyEventsPayload(eventsToSend) + ParselyTracker.PLog("POST Data %s", jsonPayload) + ParselyTracker.PLog("Requested %s", ParselyTracker.ROOT_URL) + restClient.send(jsonPayload) + .fold( + onSuccess = { + ParselyTracker.PLog("Pixel request success") + repository.remove(eventsToSend) + ParselyTracker.PLog("Event queue empty, flush timer cleared.") + if (repository.getStoredQueue().isEmpty()) { + flushManager.stop() + } + }, + onFailure = { + ParselyTracker.PLog("Pixel request exception") + ParselyTracker.PLog(it.toString()) + } + ) } } } From 784a0211aaddccc3ce22287cac3f8927c6463268 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 24 Nov 2023 14:52:41 +0100 Subject: [PATCH 39/39] feat: do not stop FlushManager on successful flush If stored queue will be empty, on next execution of `FlushQueue`, the manager will be stopped anyway. https://github.com/Parsely/parsely-android/pull/92#discussion_r1400718192 --- .../com/parsely/parselyandroid/FlushQueue.kt | 4 --- .../parsely/parselyandroid/FlushQueueTest.kt | 26 ------------------- 2 files changed, 30 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt index b5864e91..4a989b95 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt @@ -39,10 +39,6 @@ internal class FlushQueue( onSuccess = { ParselyTracker.PLog("Pixel request success") repository.remove(eventsToSend) - ParselyTracker.PLog("Event queue empty, flush timer cleared.") - if (repository.getStoredQueue().isEmpty()) { - flushManager.stop() - } }, onFailure = { ParselyTracker.PLog("Pixel request exception") diff --git a/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt index d92c5705..e49605b6 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt @@ -103,32 +103,6 @@ class FlushQueueTest { assertThat(repository.getStoredQueue()).isNotEmpty } - @Test - fun `given non-empty local storage, when flushing queue with not skipping sending events, then flush manager is stopped`() = - runTest { - // given - val flushManager = FakeFlushManager() - val repository = FakeRepository().apply { - insertEvents(listOf(mapOf("test" to 123))) - } - val parselyAPIConnection = FakeRestClient().apply { - nextResult = Result.success(Unit) - } - val sut = FlushQueue( - flushManager, - repository, - parselyAPIConnection, - this - ) - - // when - sut.invoke(false) - runCurrent() - - // then - assertThat(flushManager.stopped).isTrue - } - @Test fun `given non-empty local storage, when flushing queue with not skipping sending events fails, then flush manager is not stopped`() = runTest {