Draft: AB#274406: Derive the region exclusively from provided credentials#77
Open
graciecooper wants to merge 7 commits intomasterfrom
Open
Draft: AB#274406: Derive the region exclusively from provided credentials#77graciecooper wants to merge 7 commits intomasterfrom
graciecooper wants to merge 7 commits intomasterfrom
Conversation
Contributor
|
@graciecooper I agree with your idea of creating a tech debt to deal with the CI pipeline |
Contributor
Author
|
Collaborator
|
@graciecooper A couple of questions on potential edge cases on region absence:
|
…ntial init When using Builder(FeatureSet) without a region, urlBuilder stays null until setCredentials() is called. All 6 direct urlBuilder access sites could NPE before the existing PartialInitialisationException safety net was reached. This adds a null-safe accessor (Optimobile.urlForService) that throws PartialInitialisationException when urlBuilder is null, channeling the error into existing catch paths so events are persisted and flushed later, in-app fetch retries, and deep links are cached. Made-with: Cursor
Move the urlForService call above dialog.show() so that if credentials aren't available yet, the PartialInitialisationException is thrown before any UI is displayed. Previously the dialog and spinner were shown first, leaving the user stuck with a permanent spinner overlay. Made-with: Cursor
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of Changes
When using delayed initialisation,
Optimobile.urlBuilderwas created during the initialOptimobile.initialize()call using the Region enum provided to the Builder. When credentials arrived later viasetCredentials(), the config's region and baseUrlMap were updated, butOptimobile.urlBuilderwas never rebuilt — meaning all API calls continued using the initial enum-based URLs.This PR creates a new
Builder(FeatureSet)constructor for delayed initialisation that doesn't need region. Region and base URL mapping remain null until credentials are provided via setCredentials(). It also of deprecates the old constructor, sort of...our beloved CI runs JDK 8, but we have a transistive dependency (looks like kotlin-stdlib via OkHttp ? ) containing Java 9 module-info classfile attributes. If we add the actual
@Deprecatedannotation, it triggers the javac type annotation scanner and that hits theMODULEattribute, which causes a pipeline failure as it doesn't recognise it. I've worked around this by adding a deprecated as Javadoc but ideally we'd want this to be communicated via annotations so it shows compiler warnings to clients? Upgrading the CI pipeline would involve a fair wee bit of work though as it involves updating all the mockito tests, not just the.yaml😔I think the solution would be to ship this so Bet365 issue is fixed ASAP, then upgrade CI pipeline as tech debt?
Breaking Changes
Release Checklist
Prepare:
Bump versions in:
Integration tests
T&T Only
Mobile Only
Deferred Deep Links
Combined
Release Procedure
devtomaster