Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Cannot obtain test certificate after deleting pending/failed test certificate (EXPOSUREAPP-7763) #3474

Merged

Conversation

kolyaopahle
Copy link
Contributor

Description

This PR changes a bit of logic during certifiacte creation where currently one failed certificate creation would result in the flow completing and no more certs being created untill the app is restarted.

Steps to reproduce

  1. this is kind of hard to reproduce as de.rki.coronawarnapp.covidcertificate.test.core.TestCertificateRepository#requestCertificate would have to throw an error
  2. as this should not happen during normal operation i recommend forcing an exception throw in that function
  3. register a test
  4. check the certificate screen for a failed certificate
  5. check if the flow is still active when scanning new certificates (note that they will never appear due to the thrown error)

Remove catch clause from flow as this would cancel it and replace it with a try catch in the on each callback
@kolyaopahle kolyaopahle added this to the 2.4.0 milestone Jun 16, 2021
@kolyaopahle kolyaopahle requested a review from a team June 16, 2021 14:11
@kolyaopahle kolyaopahle added backend Issues related to internal work not directly correlated to UI interaction bug Something isn't working maintainers Tag pull requests created by maintainers labels Jun 16, 2021
@kolyaopahle kolyaopahle changed the title TestCertificateRetrievalScheduler: Cannot obtain test certificate after deleting pending/failed test certificate (EXPOSUREAPP-7763) Jun 16, 2021
@jurajkusnier jurajkusnier self-assigned this Jun 16, 2021
Copy link
Member

@d4rken d4rken left a comment

Choose a reason for hiding this comment

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

lgtm

if (checkCerts) scheduleWorker()
try {
Timber.tag(TAG).d("State change: checkCerts=$checkCerts")
if (checkCerts) scheduleWorker()
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this could reasonably throw an exception, but catching it doesn't hurt either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i agree, i did it here for consistency with the other occasions, but we could remove it as well.

@d4rken d4rken self-assigned this Jun 16, 2021
Copy link
Contributor

@jurajkusnier jurajkusnier left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarcloud
Copy link

sonarcloud bot commented Jun 16, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@harambasicluka harambasicluka merged commit dd04ef0 into release/2.4.x Jun 16, 2021
@harambasicluka harambasicluka deleted the fix/7763-certificate-creation-no-cancel-flow branch June 16, 2021 14:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backend Issues related to internal work not directly correlated to UI interaction bug Something isn't working maintainers Tag pull requests created by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants