Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Dispatch 'complete' event when streaming connection closed #83

Merged
merged 7 commits into from
Aug 24, 2023

Conversation

kevinlind
Copy link
Contributor

Description

Dispatch a "complete" event when the streaming connection is closed, when requested. This event may be used by extensions who are not using the public API sendEvent but wish to know when all response handles are received for a given request event.

Allows callers to add a flag to the Edge request to signal a "complete" event should be dispatched when the streaming connection is closed. The flag sendCompletion is part of the request object, a sibling object to the data and xdm objects in the Edge Experience Event.

{
   "xdm": { ... },
   "request": { "sendCompletion" : true }
}

The presence of the sendCompletion flag in the request causes the Edge extension to dispatch a paired response event to signal to the caller that the connection is closed and no more streaming response handles will be dispatched. The response event is of type com.adobe.eventType.edge and source com.adobe.eventSource.contentComplete and contains data of just requestId for debugging. The parentId of the event will match the UUID of the original request 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.

@kevinlind kevinlind marked this pull request as draft August 21, 2023 20:48
@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #83 (ce87682) into dev-v2.2.1 (7689f2f) will increase coverage by 0.21%.
The diff coverage is 100.00%.

❗ Current head ce87682 differs from pull request most recent head f969ae3. Consider uploading reports for the commit f969ae3 to get more accurate results

@@               Coverage Diff                @@
##             dev-v2.2.1      #83      +/-   ##
================================================
+ Coverage         83.63%   83.84%   +0.21%     
- Complexity          380      385       +5     
================================================
  Files                29       29              
  Lines              1582     1578       -4     
  Branches            225      223       -2     
================================================
  Hits               1323     1323              
+ Misses              162      161       -1     
+ Partials             97       94       -3     
Flag Coverage Δ
functional-tests 66.48% <92.31%> (+0.29%) ⬆️
unit-tests 79.78% <100.00%> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...java/com/adobe/marketing/mobile/EdgeConstants.java 100.00% <ø> (ø)
...a/com/adobe/marketing/mobile/EdgeHitProcessor.java 87.22% <100.00%> (-0.37%) ⬇️
...adobe/marketing/mobile/NetworkResponseHandler.java 84.62% <100.00%> (+1.71%) ⬆️

@kevinlind kevinlind linked an issue Aug 21, 2023 that may be closed by this pull request
2 tasks
Copy link
Contributor

@timkimadobe timkimadobe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just had a question about the variable name for the new constants

Copy link
Contributor

@timkimadobe timkimadobe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you Kevin

Do we also want to include the new flag in the event reference documentation (https://github.com/adobe/aepsdk-edge-ios/pull/383/files#diff-49f4dd7f8fd0c3aa77b7402714be2b63f2a34fbfbace7def48fc4a6de450a344)? Or is there a better location for that one

@kevinlind
Copy link
Contributor Author

Looks great! Thank you Kevin

Do we also want to include the new flag in the event reference documentation (https://github.com/adobe/aepsdk-edge-ios/pull/383/files#diff-49f4dd7f8fd0c3aa77b7402714be2b63f2a34fbfbace7def48fc4a6de450a344)? Or is there a better location for that one

I'm not sure on the best place to include it. I've updated the event-reference to include the sendCompletion plus to clarify the response content event. Let me know what you think.

@kevinlind kevinlind marked this pull request as ready for review August 23, 2023 04:43
| Event type | Event source |
| ---------- | ------------ |
| com.adobe.eventType.edge | com.adobe.eventSource.responseContent |
| com.adobe.eventType.edge | defined by response handle type, or com.adobe.eventSource.responseContent |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nit: what do you think of capital "Defined" instead of defined?

@kevinlind kevinlind merged commit ddbd01f into adobe:dev-v2.2.1 Aug 24, 2023
@kevinlind kevinlind deleted the dispatch-complete branch August 24, 2023 21:11
timkimadobe added a commit that referenced this pull request Sep 18, 2023
* Fix warnings in Gradle files (#80)

* Fix deprecated use of

* Fix Groovy warnings in build files

* Dispatch 'complete' event when streaming connection closed (#83)

* Send 'complete' event after all response handles are recieved when requested

* Update event source variable names to match source name 'CONTENT_COMPLETE'

* Reword method description for sendCompletionRequested

* Add more tests to bump coverage numbers

* Add Content Complete event to event-reference documentation

* Update event-reference with sendCompletion property and response content definition.

* Capitalize 'Defined' in table for completion event description

* Bump version to 2.3.0

* Add content complete event to toc, update response content description

* Revert 'asNode(null)' to 'asNode()'.
Using 'asNode()' gives warning about implicit null value, however passing 'asNode(null)' fails to build publish step.

* Replace hardcoded event source with EventSource.CONTENT_COMPLETE (#89)

* Set Core dependency to 2.4.0

* Replace hardcoded event source with EventSource.CONTENT_COMPLETE

* Fix failing deep copy test after Core changes to ignore invalid objects.

* Initial setup of integration test class and dependencies

* Enable passing test suite preset values via command line and IDE

Update integration test setup steps

* Add convenience method setExpectationForNetworkRequest that accepts a testable network request

First integration test case

* Add new test constants for edge integration testing, alphabetized

* Add integration test cases and helpers

* Update integration test core dependency version to 2.4.0

* Add more test cases

* Apply lint formatting

* Create helper resetTestExpectations() and update TestHelper.resetTestExpectations call

* Remove unused methods

* Refactor networkResponses to store list of all responses for given request

Refactor usages
Update documentation

* Revert makefile changes for followup PR

* Add back junit Assert import lost after merge resolution

* Fix other merge issues

* Rename setResponseConnectionFor to setResponseFor to match iOS naming

---------

Co-authored-by: Kevin Lind <40409666+kevinlind@users.noreply.github.com>
Co-authored-by: Kevin Lind <lind@adobe.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optionally dispatch a "complete" Event when a streaming connection is closed
2 participants