-
Notifications
You must be signed in to change notification settings - Fork 25
[Identity] Simplify readyForEvent by removing unnecessary configuration shared state checks #789
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
94b85e2
Cleanup StringEncoder
praveek cd3dc79
Update AndroidManifest.xml
praveek 0d6236b
Merge branch 'dev' of github.com:praveek/aepsdk-core-android into dev
praveek 949ad95
Merge branch 'dev' of https://github.com/adobe/aepsdk-core-android in…
praveek 6d1c710
Merge branch 'dev' of https://github.com/adobe/aepsdk-core-android in…
praveek ef03ffb
[Identity] Simplify readyForEvent by removing unnecessary configurati…
praveek a3b603a
Add configuration state check for appendUrl and getUrl events
praveek File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check for a valid Configuration is still needed for the block above (lines 213-232) for appendToUrl and getUrlVariables. The logic for that block relied on the Configuration state check if the block evaluated to true and did not return false due to the Analytics state check. If the current Configuration state is not set then appendToUrl/getUrlVariables won't necessarily fail, but they may not include the org ID in their responses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add an explicit configuration check but the behaviour now is same as before. The
forceSyncIdentifierscall only succeeds if the SDK has a valid configuration that includes an org-id. Right now, we only check the shared state in the context of the specific event, and the call is assumed to always pass ifforceSyncIdentifierssucceeds. In this particular case, we had a valid (but empty) configuration, which will allow the call to proceed, but the org-id was still missing.I think we should reconsider the broader approach of how we validate states in
readyForEvent.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I see your point. Adding this check back for configuration is just checking the configuration wasn't cleared which isn't really possible, so it's not necessary. Sorry for the confusion, you can roll back your last commit if you want.