Skip to content
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

fix(ATS): send an internal notification when project config is updated #448

Merged
merged 2 commits into from
Feb 16, 2023

Conversation

jaeopt
Copy link
Contributor

@jaeopt jaeopt commented Feb 9, 2023

Summary

  • For ODPConfig update, send an internal notification when project config is updated (in addition to public NotificationCenter).
  • A few more unit test cases are added for OPD integration tests.

Test plan

  • Unit test validating ODPConfig updated when a new datafile is downloaded.

Issues

Copy link
Contributor

@mnoman09 mnoman09 left a comment

Choose a reason for hiding this comment

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

Added few questions. Otherwise changes LGTM.

return;
}

String sdkKey = config.getSdkKey();
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a corner case in which this will cause a problem. That when sdkKey is not in the datafile but is used to fetch datafile from a custom URL?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if this is a valid scenario for android or not but we handled it in java-sdk.

Copy link
Contributor Author

@jaeopt jaeopt Feb 15, 2023

Choose a reason for hiding this comment

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

@mnoman09 A good point. This line is for after fetching remote datafile. Some CDN (or proxy server) datafiles of old projects may miss sdkKeys but those are obviously not using ODP integrations, so must be safe. I mean if they want to integrate ODP then it's expected to have sdkKey in the datafile too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I got your point but I think even if the sdkKey exist in the datafile it can still cause the problem when the sdkKey used to download the datafile is different then what is in the datafile (Like we are doing in FSC using proxy server and using custom temporary sdkKeys). Notification will not trigger in that scenario.
In order to keep it consistent we need to somehow use the sdkKey which is used in NotificationRegistry (which is passed by user).

@@ -946,4 +947,34 @@ NotificationCenter getNotificationCenter() {

return null;
}

void sendUpdateConfigNotification() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the function that we are going to expose to public or not? If yes then we should add public.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not need to open to public. This one includes internal notification handling in addition to already-existing public notification interface.

@jaeopt jaeopt requested a review from mnoman09 February 15, 2023 22:30
Copy link
Contributor

@mnoman09 mnoman09 left a comment

Choose a reason for hiding this comment

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

I am approving the PR. you check the reply on the comment if we believe it is a valid scenario then we need to handle it, otherwise we can merge the PR.

@jaeopt jaeopt merged commit 184f426 into master Feb 16, 2023
@jaeopt jaeopt deleted the jae/odp-notif-registry branch February 16, 2023 21:18
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.

3 participants