-
Notifications
You must be signed in to change notification settings - Fork 243
Fixed bug with Advanced Compliance and Integration Validator #1495
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
Fixed bug with Advanced Compliance and Integration Validator #1495
Conversation
…or whether or not it should use the custom endpoint. Updated getBaseURLForLinkingEndpoints() to take in a bool parameter for whether or not it should use the custom endpoint.
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 PR addresses a bug with the Advanced Compliance and Integration Validator by adding a parameter to control when custom API URLs should be used. The implementation looks good, but I have a few suggestions to improve code clarity and maintainability.
Sources/BranchSDK/BNCServerAPI.m
Outdated
- (NSString *)getBaseURLForLinkingEndpoints: (BOOL)useCustom { | ||
if (useCustom && self.customAPIURL) { | ||
return self.customAPIURL; | ||
} |
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.
💡 Optional Recommendation
Issue: The code doesn't have any logging when switching between custom and standard URLs.
Fix: Consider adding debug logging to help with troubleshooting.
Impact: Would make it easier to debug URL-related issues in production environments.
- (NSString *)getBaseURLForLinkingEndpoints: (BOOL)useCustom { | |
if (useCustom && self.customAPIURL) { | |
return self.customAPIURL; | |
} | |
- (NSString *)getBaseURLForLinkingEndpoints: (BOOL)useCustom { | |
if (useCustom && self.customAPIURL) { | |
BNCLogDebug(@"Using custom API URL: %@", self.customAPIURL); | |
return self.customAPIURL; | |
} | |
BNCLogDebug(@"Using standard API URL"); |
Co-authored-by: matter-code-review[bot] <150888575+matter-code-review[bot]@users.noreply.github.com>
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
Updated getBaseURLForLinkingEndpoints() to take in a bool parameter for whether or not it should use the custom endpoint.
Reference
INTENG-22685
Summary
If a customer is using Advanced Compliance and then runs the Integration Validator code, the Integration Validator will not work. This is because the protected-api endpoint gets used for the requests, and there is no app-settings protected equivalent for that endpoint. Since the Integration Validator code is never released into the wild, but only used for testing, we can safely keep that endpoint going to the regular Branch endpoint, with all other calls like opens, installs, etc... still going through the protected endpoint for Advanced Compliance customers. To address this, I've added a boolean parameter to the getBaseURLForLinkingEndpoints() function, which can take in false in the case of any test code, like the integration validator, so that the regular endpoint (instead of protected-api) is used.
Motivation
To fix the bug caused when the protected API URL is set before the integration validator is run:
[Branch setAPIUrl:@"https://protected-api.branch.io"];
[branch validateSDKIntegration];
Type Of Change
Testing Instructions
[Branch setAPIUrl:@"https://protected-api.branch.io"];
[branch validateSDKIntegration];
cc @BranchMetrics/saas-sdk-devs for visibility.