MOB-20804 - updateProposition api with a callback to handle timeout by akhiljain1907 · Pull Request #110 · adobe/aepsdk-optimize-ios

akhiljain1907 · 2024-08-22T11:20:51Z

Description

This PR supports update proposition public api call with an optional completion handler invoked with list of successful decision scopes and errors received by edge (if any). In order to provide error details back we have also created a public class AEPOptimizeError and kept an additional field of AEPError in it which is mapped on the basis of error status received.

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.

…Error will never succeed

…nctionality

…dk-optimize-ios into fix/updateProposition

spoorthipujariadobe · 2024-08-23T16:47:16Z

Podfile.lock

@@ -1,68 +0,0 @@
-PODS:


Does this need to be deleted? 🤔

Restored old podfile.lock from base branch.

spoorthipujariadobe · 2024-08-23T16:49:57Z

Sources/AEPOptimize/Optimize+PublicAPI.swift

    /// - Parameter data: Additional free-form data to be sent in the personalization request.
-    @objc(updatePropositions:withXdm:andData:)
-    static func updatePropositions(for decisionScopes: [DecisionScope], withXdm xdm: [String: Any]?, andData data: [String: Any]? = nil) {
+    /// - Parameter completion: An optional callback to be used by consumer


can you add what is returned in the callback?
- Parameter completion: Optional completion handler invoked with errors, if any

spoorthipujariadobe · 2024-08-23T16:51:49Z

Sources/AEPOptimize/Optimize+PublicAPI.swift

-    static func updatePropositions(for decisionScopes: [DecisionScope], withXdm xdm: [String: Any]?, andData data: [String: Any]? = nil) {
+    /// - Parameter completion: An optional callback to be used by consumer
+    @objc(updatePropositions:withXdm:andData:completion:)
+    static func updatePropositions(for decisionScopes: [DecisionScope], withXdm xdm: [String: Any]?, andData data: [String: Any]? = nil, _ completion: ((Bool, Error?) -> Void)? = nil) {


Wondering if we can do away with the boolean value and simplify the completion handler to just _ completion: ((Error?) -> Void)?. If there's no error, we can return null. The boolean field seems a bit superfluous. Thoughts?

We have changed this to keep list of successful decision scopes and AEPOptimize error as of now

Update - we are now returning ([DecisionScope: OptimizeProposition]?, AEPOptimizeError?) in completion

spoorthipujariadobe · 2024-08-23T17:13:55Z

Sources/AEPOptimize/Optimize.swift

+                type: EventType.optimize,
+                source: EventSource.responseContent,
+                data: [
+                    OptimizeConstants.EventDataKeys.COMPLETED_UPDATE_EVENT_ID: requestEventId


This value isn't being read or used in the public API response callback. Can we omit it? The public API does not need to aware of this edge request event id

Yes, from here we are sending list of successful decision scopes.

Update - now we are sending propositionsInProgress: [DecisionScope: OptimizeProposition] from here

spoorthipujariadobe · 2024-08-23T17:17:51Z

Sources/AEPOptimize/Optimize+PublicAPI.swift

+                completion?(false, AEPError.callbackTimeout)
+                return
+            }
+            if let error = responseEvent.data?[OptimizeConstants.EventDataKeys.RESPONSE_ERROR] as? AEPError {


nit: Can you check if you can use getTypedData here instead of casting?

let error: AEPError = responseEvent.getTypedData(for: OptimizeConstants.EventDataKeys.RESPONSE_ERROR)

spoorthipujariadobe

Please fix code formatting.
Make changes to existing tests if needed and ensure all of them pass.
In OptimizePublicAPITests, please add a test which has a completion handler passed in updatePropositions API call and verify its called in case of timeout.
In OptimizeIntegrationTests, please add a test testUpdatePropositions_timeoutError with a completion handler passed in and simulating a network request timeout case.

…ecision scopes and AEPOptimizeError

…timeout

akhiljain1907 · 2024-08-30T07:52:30Z

Hi @spoorthipujariadobe, I have made few changes based on your comments and added test cases as you suggested. Please review.

…changed default value of aepError to unexpected

spoorthipujariadobe · 2024-09-04T23:20:59Z

Sources/AEPOptimize/Event+Optimize.swift

                            source: EventSource.responseContent,
                            data: [
-                                OptimizeConstants.EventDataKeys.RESPONSE_ERROR: error.rawValue
+                                OptimizeConstants.EventDataKeys.RESPONSE_ERROR: error


nit: If createErrorResponseEvent(_ error: AEPError) isn't being used anymore, can we remove it?

since we return AEPError in getProposition api, we are using this function. If we return AEPOptimizeError instead, we can remove this function.

Discussed offline to return AEPOptimizeError in getPropositions as well for API consistency

spoorthipujariadobe · 2024-09-04T23:24:58Z

Sources/AEPOptimize/Optimize+PublicAPI.swift

-    static func updatePropositions(for decisionScopes: [DecisionScope], withXdm xdm: [String: Any]?, andData data: [String: Any]? = nil) {
+    /// - Parameter completion: Optional completion handler invoked with list of successful decision scopes and errors, if any
+    @objc(updatePropositions:withXdm:andData:completion:)
+    static func updatePropositions(for decisionScopes: [DecisionScope], withXdm xdm: [String: Any]?, andData data: [String: Any]? = nil, _ completion: (([DecisionScope]?, AEPOptimizeError?) -> Void)? = nil) {


I thought we're sending back the propositions that were successfully fetched in the completion handler. Should this be
_ completion: (([DecisionScope: OptimizeProposition]?, AEPOptimizeError?) -> Void)? =nil

As discussed offline, we are now sending ([DecisionScope: OptimizeProposition]?, AEPOptimizeError?) in completion

spoorthipujariadobe · 2024-09-04T23:35:57Z

Sources/AEPOptimize/Optimize.swift

+                type: EventType.optimize,
+                source: EventSource.responseContent,
+                data: [
+                    OptimizeConstants.EventDataKeys.DECISION_SCOPES: Array(self.propositionsInProgress.keys)


Same question here. Should we also be returning the propositionsInProgress.values which have the actual OptimizeProposition?

Yes, we are now sending propositionInProgress in event data.

spoorthipujariadobe · 2024-09-04T23:39:46Z

Sources/AEPOptimize/OptimizeError.swift

+    public let status: Int?
+    public let title: String?
+    public let detail: String?
+    public var aepError = AEPError.unexpected


Wondering why we added AEPError here? Could you please share how customers are going to use this?

In get proposition api, today we return ([DecisionScope: OptimizeProposition]?, Error?), where Error is of type AEPError. Considering the possibility customer has kept some check or performed any logic on their side on basis of AEPErrors we added this variable. We can discuss on sending back AEPOptimizeError in getProposition api as well instead of AEPError to improve on parity between api callback.

Sources/AEPOptimize/OptimizeError.swift

spoorthipujariadobe · 2024-09-04T23:56:29Z

Tests/AEPOptimizeTests/FunctionalTests/OptimizeFunctionalTests.swift

+        wait(for: [expectation], timeout: 12)
+        XCTAssertEqual(mockRuntime.dispatchedEvents.count, 2)

-        let dispatchedEvent = mockRuntime.dispatchedEvents.first


can you add assertions for the first event as well to verify the type, source and maybe the propositions that are returned in that event in case of success?

Same comment for all following tests

spoorthipujariadobe

Great job with the changes and test coverage! I just have a few small questions

spoorthipujariadobe · 2024-09-05T00:06:42Z

Sources/AEPOptimize/Optimize+PublicAPI.swift

+                    status: 408,
+                    title: "Request Timeout",
+                    detail: "Update proposition request resulted in a timeout.",


Can we add these values in OptimizeConstants?

spoorthipujariadobe · 2024-09-05T00:08:07Z

Sources/AEPOptimize/Optimize.swift

+                    status: 408,
+                    title: "Request Timeout",
+                    detail: "Update proposition request resulted in a timeout.",


Same comment here. Please add in OptimizeConstants and reuse wherever needed

spoorthipujariadobe · 2024-09-05T00:13:55Z

Sources/AEPOptimize/Optimize.swift

            }

+            // response event to provide success callback to updateProposition public api
+            let responseEventToSend = event.createResponseEvent(


Considering we're also using this same event type and source in processGetPropositions do you think it makes sense to move this also to Event+Optimize.swift, similar to createErrorResponseEvent?

Yes, since now we are returning ([DecisionScope: OptimizeProposition]?, AEPOptimizeError?) in update proposition's api, we can create a common util function in Event+Optimize.swift. However, in update proposition callback we can have scenario where we have successful [DecisionScope: OptimizeProposition] and AEPOptimizeError both if one upstream gives error and other succeed. So I tried creating a common function which is like this -
createCallbackResponseEvent(_ data: [DecisionScope: OptimizeProposition], _ error: AEPOptimizeError?) -> Event and in case of get proposition i tried sending error as nil which failed some of the test cases. I am debugging that and once I am clear on that I will push it or will include this in next PR which will cover other edge error handling apart from timeout.

spoorthipujariadobe · 2024-09-05T00:15:31Z

Tests/AEPOptimizeTests/IntegrationTests/OptimizeIntegrationTests.swift

        // update propositions
-        Optimize.updatePropositions(for: [decisionScope], withXdm: nil)
+        Optimize.updatePropositions(for: [decisionScope], withXdm: nil){ (scope,error) in
+            XCTAssertNotNil(scope)


nit: Can you also add XCTAssertNil(error) for success case?

…sition]? instead of [DecisionScope]? in callback and code review changes

…ve AEPOptimizeError as argument and minor changes

spoorthipujariadobe

Pending deciding if we plan to return AEPError or AEPOptimizeError in all public APIs

…ty and code optimization

sbenedicadb

looks ok other than it's not clear to me what the purpose is of the type property in OptimizeError.

let's either make it more clear what that property is for, or remove it if it's not in use.

sbenedicadb · 2024-09-18T17:46:39Z

Sources/AEPOptimize/OptimizeError.swift

+@objc(AEPOptimizeError)
+public class AEPOptimizeError: NSObject, Error {
+    typealias HTTPResponseCodes = OptimizeConstants.HTTPResponseCodes
+    public let type: String?


is this type property being used?

type property is received in edge error response content and we have kept it for that purpose. For timeout or any other invalid request case on optimize SDK side this will be nil.

akhiljain1907 · 2024-09-19T06:26:27Z

thanks a lot for reviewing @sbenedicadb, answering for purpose of keeping type field -
"We receive following fields from in Edge error response handler which are being sent from konductor itself -

type - string - A URI reference (RFC3986) that identifies the problem type, following the format https://ns.adobe.com/aep/errors/.
status - number - The HTTP status code generated by the server for this occurrence of the problem.
title - string - A short, human-readable summary of the problem type
detail - string - A short, human-readable description of the problem type
report - object - A map of additional properties that aid in debugging such as the request ID or the org ID. In some cases, it might contain data specific to the error at hand, such as a list of validation errors.

As discussed with team, we have currently decided to send the type field in the error object and thus we have kept it in OptimizeError class."

sbenedicadb · 2024-09-19T13:46:43Z

Thanks @akhiljain1907 - this is super helpful information. it would be good to include this detail in the code itself so anyone maintaining it could get this context as well.

akhiljain1907 added 4 commits August 20, 2024 18:06

Updated updateProposition public API to provide a callback to customer

1f5367c

fixed bug as typecasting error.rawValue which is an Integer to an AEP…

dc0be28

…Error will never succeed

updated valid edge response updateProposition's test with callback fu…

a722582

…nctionality

Delete Podfile.lock

46007b5

akhiljain1907 changed the title ~~Updated updateProposition api with a callback to handle timeout.~~ MOB-20804 - updateProposition api with a callback to handle timeout Aug 22, 2024

akhiljain1907 added 6 commits August 22, 2024 17:50

updated failed test case due to bugfix change also linting changes

83e2768

Merge branch 'fix/updateProposition' of github.com:akhiljain1907/aeps…

31126bc

…dk-optimize-ios into fix/updateProposition

minor change to pass linting check

9012e54

minor linting change

83da900

minor space linting change

c116277

fixed failed test case for bugfix

ca462b6

akhiljain1907 requested review from sbenedicadb and spoorthipujariadobe August 23, 2024 13:17

added few relevant comments

e06e507

spoorthipujariadobe reviewed Aug 23, 2024

View reviewed changes

spoorthipujariadobe requested changes Aug 23, 2024

View reviewed changes

akhiljain1907 marked this pull request as draft August 28, 2024 06:19

akhiljain1907 added 7 commits August 28, 2024 15:56

Restored Podfile.lock

0a0ad96

updated update propositions public api to return list of successful d…

f293e42

…ecision scopes and AEPOptimizeError

Merge branch 'fix/updateProposition' into dev-v5.0.2

d69ec25

Synced with 5.0.2 branch | updated test cases.

ae822b2

minor formatting change

117eb36

minor code formatting

93fdd31

added test cases for updateProposition api callback and verified for …

dd12423

…timeout

akhiljain1907 closed this Aug 29, 2024

akhiljain1907 reopened this Aug 29, 2024

minor change and formatting

f1514bc

akhiljain1907 marked this pull request as ready for review August 30, 2024 07:51

mapped aepError values in OptimizeError class on basis of status and …

2b65030

…changed default value of aepError to unexpected

spoorthipujariadobe reviewed Sep 4, 2024

View reviewed changes

Sources/AEPOptimize/OptimizeError.swift Show resolved Hide resolved

spoorthipujariadobe reviewed Sep 4, 2024

View reviewed changes

spoorthipujariadobe reviewed Sep 5, 2024

View reviewed changes

akhiljain1907 added 2 commits September 9, 2024 20:13

updated updateProposition api to return [DecisionScope: OptimizePropo…

841c1b8

…sition]? instead of [DecisionScope]? in callback and code review changes

removed createErrorResponseEvent(_ error: AEPError) as now we will ha…

421a6ef

…ve AEPOptimizeError as argument and minor changes

spoorthipujariadobe approved these changes Sep 10, 2024

View reviewed changes

akhiljain1907 requested a review from praveek September 11, 2024 09:32

akhiljain1907 added the enhancement New feature or request label Sep 12, 2024

akhiljain1907 added 3 commits September 16, 2024 14:48

added updateProposition api without callback for backward compatibili…

97f769f

…ty and code optimization

minor formatting change for code optimization

1453989

Minor formatting

9fb13bb

sbenedicadb approved these changes Sep 18, 2024

View reviewed changes

akhiljain1907 merged commit 47c1d3e into adobe:dev-v5.0.2 Sep 19, 2024

Conversation

akhiljain1907 commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spoorthipujariadobe left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akhiljain1907 commented Aug 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spoorthipujariadobe Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spoorthipujariadobe Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

spoorthipujariadobe Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spoorthipujariadobe left a comment

Choose a reason for hiding this comment

Uh oh!

spoorthipujariadobe Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

akhiljain1907 commented Aug 22, 2024 •

edited

Loading

spoorthipujariadobe left a comment •

edited

Loading

spoorthipujariadobe Sep 4, 2024 •

edited

Loading

spoorthipujariadobe Sep 4, 2024 •

edited

Loading

spoorthipujariadobe Sep 4, 2024 •

edited

Loading

spoorthipujariadobe Sep 5, 2024 •

edited

Loading

spoorthipujariadobe Sep 5, 2024 •

edited

Loading

spoorthipujariadobe left a comment •

edited

Loading