Dispatch 'complete' event when streaming connection closed #383

timkimadobe · 2023-08-22T18:37:45Z

Description

Paired with Android implementation adobe/aepsdk-edge-android#83
This PR updates:

The NetworkResponseHandler to add a new method processResponseOnComplete which handles the sending of "complete" events
Test app to include new button that sends a request content event with the sendCompletion flag set to true
Unit and functional tests
Event reference docs to include new flag and dispatched event

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

codecov · 2023-08-22T18:45:55Z

Codecov Report

Merging #383 (4d10753) into dev (a630ae1) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #383      +/-   ##
==========================================
+ Coverage   96.73%   96.77%   +0.04%     
==========================================
  Files          27       27              
  Lines        1653     1671      +18     
==========================================
+ Hits         1599     1617      +18     
  Misses         54       54

Add more functional test cases for onComplete behavior

Correct the documentation for response content as a fallback to error response content Alphabetize items

Reorganize view to move scrollable data view to the top and not truncate text Add visual indicators to scrollable data view

kevinlind

Looks good. Added comments for a few minor issues.

kevinlind · 2023-08-23T02:37:56Z

Sources/EdgeNetworkHandlers/NetworkResponseHandler.swift

@@ -64,10 +64,10 @@ class NetworkResponseHandler {
    /// Remove the requestId in the internal `sentEventsWaitingResponse` along with the associated list of events.
    /// - Parameter requestId: batch request id
    /// - Returns: the list of unique event ids associated with the requestId that were removed


Update comment to state it returns the events.

kevinlind · 2023-08-23T02:46:29Z

Tests/FunctionalTests/NetworkResponseHandlerFunctionalTests.swift

+        XCTAssertEqual(1, dispatchedEvents.count)
+
+        let flattenedData = flattenDictionary(dict: dispatchedEvents.first?.data ?? [:])
+        XCTAssertEqual(1, flattenedData.count)
+        XCTAssertEqual(requestID, flattenedData["requestId"] as? String)


You can use assertResponseCompleteEventWithData here.

Thank you! Updated

kevinlind · 2023-08-23T04:44:54Z

Documentation/event-reference.md


 ### Edge response content

-This event is a response to an [Edge request content](#edge-request-content) event.
+This event is a fallback to an [Edge error response content](#edge-error-response-content) event.


This is technically the fallback when a network response handle doesn't define a type, not for errors. I've updated the Android definition to state this event include the Edge Network response handles and com.adobe.eventSource.responseContent is used if the response handle doesn't define a type. I'm not sure if we want to include that specific of information, however.

Thank you for clarifying! I've updated to align with the updates you've made on the Android side. I think it could be helpful to include in the off chance someone runs into this event case and uses the reference

kevinlind · 2023-08-23T05:06:10Z

Documentation/event-reference.md

+The `sendCompletion` flag is used within the Edge request to indicate whether a "complete" event should be dispatched once the streaming connection is closed. This flag is part of the `request` object, which is at the same hierarchical level as the `data` and `xdm` objects in the Edge Experience Event.
+
+```json
+{
+   "xdm": { ... },
+   "request": { "sendCompletion" : true }
+}
+```
+
+When the `sendCompletion` flag in the request is set to `true`, the Edge extension will dispatch a *paired* response event. This event notifies the caller that the connection has been closed and that no further streaming response handles will be dispatched. The [response event](#edge-content-complete) includes only the `requestId` in its data, for debugging purposes. The `parentID` of this event corresponds to the UUID of the originating request event.


I like this write up, however these parameters are only send in the request content events. Having a separate flags section seems to imply that any of the request event could contain the sendCompletion flag. In the Android PR, I've split this information to include the request parameter in the request content section, while adding the sendCompletion in the content complete section. Having all this under the request content section is a good option too. What do you think?

I agree that having the flags section separate could be confusing; I like the way that you've organized the information! Updated to align this doc with the latest Android changes

timkimadobe

Thanks for the review @kevinlind! Updated based on feedback

timkimadobe · 2023-08-23T18:42:28Z

Sources/EdgeNetworkHandlers/NetworkResponseHandler.swift

@@ -64,10 +64,10 @@ class NetworkResponseHandler {
    /// Remove the requestId in the internal `sentEventsWaitingResponse` along with the associated list of events.
    /// - Parameter requestId: batch request id
    /// - Returns: the list of unique event ids associated with the requestId that were removed


timkimadobe · 2023-08-23T18:45:52Z

Tests/FunctionalTests/NetworkResponseHandlerFunctionalTests.swift

+        XCTAssertEqual(1, dispatchedEvents.count)
+
+        let flattenedData = flattenDictionary(dict: dispatchedEvents.first?.data ?? [:])
+        XCTAssertEqual(1, flattenedData.count)
+        XCTAssertEqual(requestID, flattenedData["requestId"] as? String)


Thank you! Updated

timkimadobe · 2023-08-23T18:50:09Z

Documentation/event-reference.md


 ### Edge response content

-This event is a response to an [Edge request content](#edge-request-content) event.
+This event is a fallback to an [Edge error response content](#edge-error-response-content) event.


Thank you for clarifying! I've updated to align with the updates you've made on the Android side. I think it could be helpful to include in the off chance someone runs into this event case and uses the reference

timkimadobe · 2023-08-23T18:52:33Z

Documentation/event-reference.md

+The `sendCompletion` flag is used within the Edge request to indicate whether a "complete" event should be dispatched once the streaming connection is closed. This flag is part of the `request` object, which is at the same hierarchical level as the `data` and `xdm` objects in the Edge Experience Event.
+
+```json
+{
+   "xdm": { ... },
+   "request": { "sendCompletion" : true }
+}
+```
+
+When the `sendCompletion` flag in the request is set to `true`, the Edge extension will dispatch a *paired* response event. This event notifies the caller that the connection has been closed and that no further streaming response handles will be dispatched. The [response event](#edge-content-complete) includes only the `requestId` in its data, for debugging purposes. The `parentID` of this event corresponds to the UUID of the originating request event.


I agree that having the flags section separate could be confusing; I like the way that you've organized the information! Updated to align this doc with the latest Android changes

timkimadobe added 4 commits August 21, 2023 16:52

Add new process on complete to NetworkResponseHandler

7853e2b

Refactor existing test based on changed return type

64bea58

Add functional test for new complete event

f8909d3

Add network response handler functional tests

c351664

Update constant names to CONTENT_COMPLETE

a1338cb

Add more functional test cases for onComplete behavior

timkimadobe marked this pull request as ready for review August 22, 2023 21:22

Merge branch 'dev' into send-completion-event

4166a61

timkimadobe requested review from kevinlind, addb and cacheung August 22, 2023 21:23

timkimadobe added 5 commits August 22, 2023 15:03

Add new request send completion flag section

108e3ad

Correct the documentation for response content as a fallback to error response content Alphabetize items

Add test button for sending event with complete flag

74436d4

Reorganize view to move scrollable data view to the top and not truncate text Add visual indicators to scrollable data view

Add explicit types

a75e118

Apply swift lint

08e27b8

Fix line length violation

348313e

kevinlind reviewed Aug 23, 2023

View reviewed changes

timkimadobe added 3 commits August 23, 2023 11:42

Update method docs to reflect new return type

23c6bb9

Refactor test case to use assertion helper

69d0586

Align event reference doc with android version

4d10753

timkimadobe commented Aug 23, 2023

View reviewed changes

kevinlind approved these changes Aug 24, 2023

View reviewed changes

kevinlind merged commit 7cce7fc into adobe:dev Aug 24, 2023

timkimadobe deleted the send-completion-event branch August 24, 2023 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dispatch 'complete' event when streaming connection closed #383

Dispatch 'complete' event when streaming connection closed #383

timkimadobe commented Aug 22, 2023 •

edited

Loading

codecov bot commented Aug 22, 2023 •

edited

Loading

kevinlind left a comment

kevinlind Aug 23, 2023

timkimadobe Aug 23, 2023

kevinlind Aug 23, 2023

timkimadobe Aug 23, 2023

kevinlind Aug 23, 2023

timkimadobe Aug 23, 2023

kevinlind Aug 23, 2023

timkimadobe Aug 23, 2023

timkimadobe left a comment

timkimadobe Aug 23, 2023

timkimadobe Aug 23, 2023

timkimadobe Aug 23, 2023

timkimadobe Aug 23, 2023

Dispatch 'complete' event when streaming connection closed #383

Dispatch 'complete' event when streaming connection closed #383

Conversation

timkimadobe commented Aug 22, 2023 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

codecov bot commented Aug 22, 2023 • edited Loading

Codecov Report

kevinlind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timkimadobe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timkimadobe commented Aug 22, 2023 •

edited

Loading

codecov bot commented Aug 22, 2023 •

edited

Loading