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

Latest issued Vaccination DCC 2/2 does not have highest priority #3838

Closed
2 of 3 tasks
vaubaehn opened this issue Jul 29, 2021 · 22 comments · Fixed by #4123
Closed
2 of 3 tasks

Latest issued Vaccination DCC 2/2 does not have highest priority #3838

vaubaehn opened this issue Jul 29, 2021 · 22 comments · Fixed by #4123
Assignees
Labels
bug Something isn't working Fix 2.11 Fix is planned for 2.11 mirrored-to-jira This item is also tracked internally in JIRA

Comments

@vaubaehn
Copy link
Contributor

vaubaehn commented Jul 29, 2021

Avoid duplicates

  • Bug is not mentioned in the FAQ
  • Bug is specific for Android only, for general issues / questions that apply to iOS and Android please raise them in the documentation repository - I'm not sure how sorting is done in iOS. If the issue is similar, this issue could be transferred to documention repo.
  • Bug is not already reported in another issue

Technical details

  • Device name: LG G4 (LGH-815)
  • Android version: Android 6.0
  • App version: 2.6.1

Describe the bug

Most users will only import 1 or 2 vaccination certificates into CWA, with either dose 2/2, or dose 1/2 together with dose 2/2.
But there are some use cases, when users are importing additionally a re-issued VaccDCC (2/2) as a 3rd certificate at a later point in time. Either just because they can do it 🤷‍♂️ , or they need to: when the Netherlands decided to apply business rules schema v1.3.0 to their validation checks, but German VaccDCCs were issued compliant to schema v1.0.0 before July 1st, and users want to be on safe side in border control and get their 2nd VacDCC re-issued (online, pharmacy). See corona-warn-app/cwa-documentation#671 .
The 3rd VaccDCC (2/2) with a newer date of issue and later date of expiry can be imported into CWA without problems, but at least in my case, the 3rd VaccDCC is not taken into account as certificate with highest priority, but sorted as the 2nd VaccDCC in the list of all DCCs. This may lead to confusion upon presenting QR code to gate keepers, even more as there is currently no date of issue/expiry date displayed (you need to check certID by comparing with paper printouts/PDFs to find out...)

Steps to reproduce the issue

  1. Import a vaccination DCC (2/2) that was issued before July 1st, 2021.
  2. Import another vaccination DCC (2/2) that was issued somewhat after July 1st, 2021.
  3. Check validity of both certificates against Netherland's business rules.
  4. Find that the one with highest priority (automatically activated) in the list fails, while the second one passes - at least in my case. So, the first one is the old one, and the second one the new one.

Expected behaviour

The (vacc)DCC (2/2) with latest date of issue/the longest expiry date should be the vaccDCC with the highest priority.

Possible Fix

Apply another rule 3a that accounts for expiry date here: #3811

Additional context

I don't know if this would also be necessary for recovery certificates...


Internal Tracking ID: EXPOSUREAPP-8756

@vaubaehn vaubaehn added the bug Something isn't working label Jul 29, 2021
@fynngodau
Copy link
Contributor

no date of issue/expiry date displayed

Expiry date will be displayed in CWA 2.7.

@MikeMcC399
Copy link
Contributor

MikeMcC399 commented Jul 29, 2021

@vaubaehn

I am also seeing CWA 2.6.1 selecting an older 2/2 certificate. They are both from PDFs and the older one has the QR code on the left of the text, the newer one has the QR code on the right of the text. They were both issued in July and they both pass the Netherlands test, so the impact of selecting the older certificate is not significant for me. Nevertheless it would be good to find out why the older one is being preferred.

@MikeMcC399
Copy link
Contributor

MikeMcC399 commented Jul 29, 2021

@vaubaehn

I did some more tests and both CWA and CovPass used the first scanned certificate as the default. If I scan the older one first, then that is default. If I scan the newer one first, then that becomes default.

D/PersonCertificatesExtensionsKt: Rule 3 match (Series-completing Vaccination Certificate > 14 days)

It seems that since the certificates have the same vaccination date, then the first one encountered in the list is preferred, with no other comparison taking place.

Perhaps there should be an FAQ article advising users to remove older certificates which have been reissued for correctional purposes? (See corona-warn-app/cwa-website#1555 for notes on terminology.)

@vaubaehn
Copy link
Contributor Author

@MikeMcC399

It seems that since the certificates have the same vaccination date, then the first one encountered in the list is preferred, with no other comparison taking place.

Yes, this seems to be the case according to comments in code:

/**
* 3
* Series-completing Vaccination Certificate > 14 days:
* Find Vaccination Certificates (i.e. DGC with v[0]) where v[0].dn equal to v[0].sd and the time difference
* between the time represented by v[0].dt and the current device time is > 14 days, sorted descending by v[0].dt
* (i.e. latest first).
* If there is one or more certificates matching these requirements,
* the first one is returned as a result of the operation.
*/
private fun Collection<CwaCovidCertificate>.rule3FindRecentLastShot(
nowUtc: Instant
): CwaCovidCertificate? = this
.filterIsInstance<VaccinationCertificate>()
.filter {
with(it.rawCertificate.vaccination) { doseNumber == totalSeriesOfDoses }
}
.filter {
Days.daysBetween(it.rawCertificate.vaccination.vaccinatedOn, nowUtc.toLocalDateUtc()).days > 14
}
.maxByOrNull { it.rawCertificate.vaccination.vaccinatedOn }

Perhaps there should be an FAQ article advising users to remove older certificates which have been reissued for correctional purposes? (See corona-warn-app/cwa-website#1555 for notes on terminology.)

This is a good idea. Ideally, the sorting algorithm could be enhanced in a future release. I guess there are already many people out there who have a re-issued 2/2 VaccDCC.

@MikeMcC399
Copy link
Contributor

@vaubaehn
This issue should also be reported to https://github.com/Digitaler-Impfnachweis/covpass-android/issues, since the CovPass App has exactly the same issue. Perhaps we should wait first for CWA dev feedback?

@vaubaehn
Copy link
Contributor Author

@MikeMcC399
Or we could ask @wkornewald whether we open an issue in https://github.com/Digitaler-Impfnachweis/covpass-android/issues or if they want to observe progress from here?

@MikeMcC399
Copy link
Contributor

MikeMcC399 commented Jul 29, 2021

@vaubaehn
I wouldn't want to assume that a particular person responds for CovPass App so I just went ahead and opened Digitaler-Impfnachweis/covpass-android#57. The two issues, for the two different apps, can be processed in parallel now.

Edit: The CovPass team has acknowledged the issue in Digitaler-Impfnachweis/covpass-android#57 (comment).

Now waiting for bug acknowledgement from the CWA Open Source Team and assignment of an internal ID.

@MikeMcC399
Copy link
Contributor

MikeMcC399 commented Jul 29, 2021

I/DccQrCodeValidator in the censored error report log file will give you the certificate's version number
I/DccQrCodeValidator: Extracted data from QR code is VaccinationCertificateQRCode(qrCode=<censor-collision/>"},"ver":"1.3.0"}, kid=xxxxxxxxxx)) when it is added into the app.

If you connect via Android Studio, then Logcat will show the full parsing of the certificate data under
I/DccQrCodeValidator when the certificate is added, including version, issuedAt, expiresAt and dt.

The schema version is defined in https://github.com/ehn-dcc-development/ehn-dcc-schema.

@Jo-Achim
Copy link

[OT]
Perhaps on this occasion, if vaccination certificates of several people have been scanned into CWA, their sorting / order could be revised. Thanks.
Please refer: corona-warn-app/cwa-wishlist#598 (comment)
[/OT]

@dsarkar dsarkar added the mirrored-to-jira This item is also tracked internally in JIRA label Jul 30, 2021
@dsarkar
Copy link
Member

dsarkar commented Jul 30, 2021

@MikeMcC399 @vaubaehn Thanks. Internal Tracking ID: EXPOSUREAPP-8756


Corona-Warn-App Open Source Team

@Jo-Achim
Copy link

Jo-Achim commented Aug 9, 2021

The sorting order of the scanned vaccination certificates is unfortunate even if there are multiple complete vaccination certificates:

Screenshot_20210809-102339_Corona-Warn_2

See: corona-warn-app/cwa-documentation#679 (comment).

"Last scanned = highest priority" would be an idea.

@Jo-Achim
Copy link

Workaround for sorting EU-Certificates (CWA 2.6.1 and CovPass 1.28.7): corona-warn-app/cwa-documentation#679 (comment).

@MikeMcC399
Copy link
Contributor

@Jo-Achim
If you have scanned in multiple certificates for a single vaccination, I would simply remove any certificate which you do not want used. I can't see any advantage to storing more than one certificate for the same person and the same vaccination.

@Jo-Achim
Copy link

@MikeMcC399
yes, I understand that and it is certainly a simple possibility.

But the initial issue here, for example, was "Latest issued Vaccination DCC 2/2 does not have highest priority" - the disadvantages have already been mentioned - against the background that, for whatever reasons, users scanned several EU certificates for the same person and vaccination.

A "for whatever reason" shouldn't be, but it could be that with a further development of the certificates not every country always keeps pace, the downward compatibility might not be given, ... - and therefore an older certificate could be helpful.

In this sense, I also understood your post https://github.com/Digitaler-Impfweise/covpass-android/issues/57#issue-956015931 under "Possible Fix":

2. If both the older and the re-issued certificate are stored, then when deciding on the "Aktuell verwendetes Zertifikat" the newer vaccination certificate should be chosen.

@MikeMcC399
Copy link
Contributor

@Jo-Achim
Keeping multiple certificates for the same person / vaccination and with different schema versions for the purpose of providing a workaround for schema compatibility issues with other country's business rules is an aspect that I had not considered.

I don't think that would be a workaround though, because only one of the certificates for a given person is chosen as "Currently used certificate" at any time. The business rule check only runs against that one certificate.

Your apparent desire to check against different schema versions stored for the same person / vaccination would need to be a separate enhancement request.

@Jo-Achim
Copy link

@MikeMcC399

I don't think that would be a workaround though, because only one of the certificates for a given person is chosen as "Currently used certificate" at any time. The business rule check only runs against that one certificate.

Well, just in case, there is still the option of selecting the certificate to be used.
Because after selecting the certificate for the person, you can scroll down to select another vaccination certificate and "check validity" in the CWA.

Your apparent desire to check against different schema versions stored for the same person / vaccination would need to be a separate enhancement request.

Done in: corona-warn-app/cwa-wishlist#611 (comment).

@MikeMcC399
Copy link
Contributor

@Jo-Achim

Well, just in case, there is still the option of selecting the certificate to be used.
Because after selecting the certificate for the person, you can scroll down to select another vaccination certificate and "check validity" in the CWA.

Thanks for pointing that out. I hadn't noticed that ability!

@MikeMcC399
Copy link
Contributor

Link back to related issue #4000.

@dsarkar dsarkar added the Fix 2.11 Fix is planned for 2.11 label Sep 27, 2021
@MikeMcC399
Copy link
Contributor

With PR #4123 merged into release/2.11.x this should be fixed in the next release. 👍🏻

@marcauberer
Copy link
Member

@vaubaehn can you confirm the fix? Can we close this one?

@MikeMcC399
Copy link
Contributor

@vaubaehn
I can't exactly carry out your repro steps with 2/2 certificates because I only have 2/2 certificates and they both use schema 1.3.0. Also, due to the fact that NL rules have been fixed (as you yourself confirmed in corona-warn-app/cwa-documentation#671 (comment)) the repro steps would not actually show up the problem regarding the choice of certificate any more.

I can however scan these two different 1.3.0 2/2 certificates in CWA Android 2.11.2 in the order older then newer certificate, and the newer certificate is selected as "Currently used certificate".

So I do believe that this issue is resolved.

@vaubaehn
Copy link
Contributor Author

Hi @MikeMcC399 and @marcauberer ,
meanwhile I have a phone at hand where CWA 2.11.2 arrived, and where I could check schema 1.0.0 and 1.3.0 certificates.
CWA now sorts certificates like intended, and I'm closing here.

Thanks to everyone involved! 🍻

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working Fix 2.11 Fix is planned for 2.11 mirrored-to-jira This item is also tracked internally in JIRA
Projects
None yet
7 participants