-
Notifications
You must be signed in to change notification settings - Fork 118
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
Refactor: add unzipping downloaded key data files but only for iOS. #157
Conversation
src/services/ExposureNotificationService/ExposureNotificationService.ts
Outdated
Show resolved
Hide resolved
if (diagnosisKeysURLs.length === 0) { | ||
throw new Error('Attempt to call detectExposure with empty list if downloaded files'); | ||
} | ||
const keysZipUrl = diagnosisKeysURLs[0]; |
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.
Should we add a check first to make sure that there's only one diagnosis key URL? What would it mean for there to be more than 1?
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.
As it is now, the rest will be ignored.
I guess, the best thing we can do is to change type signature, to take string
instead of string[]
.
Native APIs concern me a little though, both iOS and Android leave the room for interpretation. iOS takes array of at least to files (bin and sig) whereas android needs just one file (but is typed such that it can take several files at once).
I'm not sure whats the best way to handle this
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.
Right now I think we should loop over the array and handle each entry appropriately, but log when we get more than 1, since it would be a surprise at this point.
@henrytao-me what do you think?
src/services/ExposureNotificationService/ExposureNotificationService.ts
Outdated
Show resolved
Hide resolved
@henrytao-me this will affect android I believe, please take a look when you have a moment. |
@honkfestival addressed all your comments, I believe. |
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.
Looks good to me!
`${unzippedLocation}/export.sig`, | ||
]); | ||
// first detected exposure is enough | ||
if (summary.matchedKeyCount > 0) break; |
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.
👍
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.
Should the unzip part be done in iOS layer instead of share layer?
@henrytao-me does this work on Android? |
f12a76d
to
5bfb3d8
Compare
…s-ios Refactor: add unzipping downloaded key data files but only for iOS.
I added adapter around ExposureNotification bridge so that it adds additional platform-specific behaviour (and unzipping downloaded files is such a behaviour on iOS)