Skip to content

Fixed processing order for update/ get events in the extension#82

Merged
swarna04 merged 3 commits intoadobe:dev-v4.0.2from
swarna04:MOB-19606
Sep 28, 2023
Merged

Fixed processing order for update/ get events in the extension#82
swarna04 merged 3 commits intoadobe:dev-v4.0.2from
swarna04:MOB-19606

Conversation

@swarna04
Copy link
Copy Markdown
Contributor

Description

Ref: [MOB-19606]

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.

Copy link
Copy Markdown
Member

@sbenedicadb sbenedicadb left a comment

Choose a reason for hiding this comment

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

looks good - couple of small nits

```
dependencies: [
.package(url: "https://github.com/adobe/aepsdk-optimize-ios.git", .upToNextMajor(from: "4.0.0"))
.package(url: "https://github.com/adobe/aepsdk-optimize-ios.git", .upToNextMajor(from: "4.0.2"))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i guess they're synonymous, but the change is fine :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup, just making sure the value here reflects the latest version!

eventsQueue.setHandler { event -> Bool in
if event.isGetEvent {
self.processGetPropositions(event: event)
} else if event.type == EventType.edge {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

use your extension?

} else if event.isUpdateEvent {

Copy link
Copy Markdown
Contributor Author

@swarna04 swarna04 Sep 26, 2023

Choose a reason for hiding this comment

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

Nope, we are tracking the edge-request content dispatched to edge extension (as a result of update propositions API call). The content complete response event will contain the edge event's ID for parent ID.

.compactMap { $0.name }

if targetDecisionScopes.isEmpty {
guard !validDecisionScopes.isEmpty else {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

Comment on lines +216 to +221
// In AEP Response Event handle, `requestEventId` corresponds to the UUID for the Edge request.
// Storing the request event UUID to compare and process only the anticipated response in the extension.
updateRequestEventIdsInProgress[edgeEvent.id.uuidString] = validDecisionScopes

// add the Edge event to update propositions in the events queue.
eventsQueue.add(edgeEvent)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

makes more sense to the reader to move these above the MobileCore.dispatch call i think.

particularly, i notice manipulating the updateRequestEventIdsInProgress dictionary in the callback, but we hadn't seen any entry made to that dictionary at that point.

Copy link
Copy Markdown
Contributor Author

@swarna04 swarna04 Sep 26, 2023

Choose a reason for hiding this comment

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

Yeah, can move those above for clarity purposes, however I believe these will almost always be executed before the async callback is invoked.

@swarna04 swarna04 merged commit f73b9f9 into adobe:dev-v4.0.2 Sep 28, 2023
swarna04 added a commit that referenced this pull request Oct 4, 2023
* Fixed processing order for update/ get events in the extension (#82)

* Fix events processing order in the extension.

* Added/ updated tests

* Incorporated feedback

* More tests for events ordering + minor fixes (#83)

* More tests + minor fixes

* feedback update

* Reset in-progress propositions in unexpected scenarios (#85)

* More tests + minor fixes

* feedback update

* Reset in-progress propositions if request eventId is no longer being tracked.

* more fixes
@swarna04 swarna04 mentioned this pull request Oct 4, 2023
10 tasks
swarna04 added a commit that referenced this pull request Oct 5, 2023
* Fixed processing order for update/ get events in the extension (#82)

* Fix events processing order in the extension.

* Added/ updated tests

* Incorporated feedback

* More tests for events ordering + minor fixes (#83)

* More tests + minor fixes

* feedback update

* Reset in-progress propositions in unexpected scenarios (#85)

* More tests + minor fixes

* feedback update

* Reset in-progress propositions if request eventId is no longer being tracked.

* more fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants