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

Trace Location URL and ID (EXPOSUREAPP-6137, EXPOSUREAPP-6182) #2737

Merged
merged 48 commits into from
Apr 6, 2021

Conversation

mtwalli
Copy link
Contributor

@mtwalli mtwalli commented Apr 4, 2021

New

  • Generate Trace Location Url specs
  • Calculate Trace Location Id specs

Change

  • Updated deep link intent filter to work with the new url requirements
  • Updated QrCode Validation to use qrCodeDescriptors from configs and support Base64 and Base32 encoding validation
  • Updated Unit Tests and clean them up

Testing

  • Test against mock server
  • Create Trace location
  • Go to test menu -> Event Registration screen
  • Scroll to the card Last Organiser Location
  • You should see location, locationUrl and locationId data displayed for the last created Location
  • If you click on the link the App should respond to and open to ConfirmCheckIn
  • After confirming a CheckIn is added to the list
  • Now go back again to the test menu of the event registration , a card should be displayed for the Last Attendee Location (ChecKIn) with the same data above , since Location is the same of organiser and attendee , the data should be exactly the same except the id which is Table specific. Data should varies in case you scanned another location which will come next by QR Code Poster (EXPOSUREAPP-5962) #2727 .

Please note that url validation supports both Base32 and Base64 ,but the not the creation which supports only Base64

@mtwalli mtwalli added the maintainers Tag pull requests created by maintainers label Apr 4, 2021
@mtwalli mtwalli added this to the 2.0.0 milestone Apr 4, 2021
@mtwalli mtwalli requested a review from a team April 4, 2021 12:27
@mtwalli mtwalli changed the title Feature/6137 qr code content Trace Location URL and ID (EXPOSUREAPP-6137) Apr 4, 2021
@mtwalli mtwalli mentioned this pull request Apr 4, 2021
3 tasks
LukasLechnerDev
LukasLechnerDev previously approved these changes Apr 6, 2021
Copy link
Contributor

@LukasLechnerDev LukasLechnerDev left a comment

Choose a reason for hiding this comment

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

Good work, nice tests, thumbs up :)

@d4rken d4rken self-assigned this Apr 6, 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.

Looks good, nice tests, few comments/questions, no blockers.

@@ -95,7 +91,8 @@ class PresenceTracingConfigMapper @Inject constructor() : PresenceTracingConfig.
revokedTraceLocationVersions = revokedTraceLocationVersionsList.orEmpty(),
riskCalculationParameters = riskCalculationParameters,
submissionParameters = submissionParameters,
plausibleDeniabilityParameters = plausibleDeniabilityParameters
plausibleDeniabilityParameters = plausibleDeniabilityParameters,
qrCodeDescriptors = qrCodeDescriptorsOrBuilderList
Copy link
Member

Choose a reason for hiding this comment

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

Should we evaluate here whether the qrCodeDescribtor is actually valid regex?, and if it is not valid regex, throw an exception to fallback to a different config?

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 did not see any specs for that. Can you align with Max first? The regex in this case is dynamic I don't know how do you want to check if valid or not ?

Copy link
Member

Choose a reason for hiding this comment

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

Just compile it, if it is not valid regex, it would throw an exception? /cc @mlenkeit

Copy link
Member

Choose a reason for hiding this comment

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

Let's merge it as is and do a follow up if necessary.

Corona-Warn-App/src/main/AndroidManifest.xml Show resolved Hide resolved
@harambasicluka harambasicluka changed the title Trace Location URL and ID (EXPOSUREAPP-6137) Trace Location URL and ID (EXPOSUREAPP-6137, EXPOSUREAPP-6182) Apr 6, 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, thanks!

@sonarcloud
Copy link

sonarcloud bot commented Apr 6, 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

74.2% 74.2% Coverage
0.0% 0.0% Duplication

@mtwalli mtwalli merged commit dd3ab20 into release/2.0.x Apr 6, 2021
@mtwalli mtwalli deleted the feature/6137-qr-code-content branch April 6, 2021 15:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
maintainers Tag pull requests created by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants