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

feat: prefill ICAO fields. #185

Closed
ascheibal opened this issue Jun 7, 2021 · 33 comments · Fixed by #296
Closed

feat: prefill ICAO fields. #185

ascheibal opened this issue Jun 7, 2021 · 33 comments · Fixed by #296
Assignees
Labels
enhancement New feature or request Investigate

Comments

@ascheibal
Copy link
Contributor

Feature description

The standardized name fields are neccessary for DCC conformity.

Problem and motivation

documentation: 9303_p3_cons_en.pdf

example implementation in python for substitution:
https://github.com/Arg0s1080/mrz/blob/master/mrz/generator/dictionaries/latin_based.py

@vaubaehn
Copy link

vaubaehn commented Jul 6, 2021

Hi @ascheibal ,
I think implementing that feature is quite urgent, see for example
#191 (comment)
corona-warn-app/cwa-app-android#3645
corona-warn-app/cwa-app-ios#2975

Although the referenced issues deal with vaccination certificates, they show the kind of trouble that manually entering of standardised names for RATs will introduce.
Wallet apps could/should use standardised names for matching different kind of certificates to single persons.

Is there any progress in the implementation of this important feature?

@ascheibal
Copy link
Contributor Author

Hi @vaubaehn,
this is currently not selected by the stakeholders and was postponed due to other important stuff.
Be also aware that most of test certificates that are issued in Germany come from other partners doing the RAT (Quicktest). It's up to their implementation of validating/correcting the input data.
Same to vaccination certificates, where the issue is more severe/can cause more trouble, and where the referenced issues point to - we do not implement the issuance service for them.
In general, I totally agree with you, but currently cannot say whether this feature will be included in the cwa-quicktest soon.

@janhoffmann
Copy link
Contributor

janhoffmann commented Jul 7, 2021

@ascheibal @ggrund-tsi I'm looking into this right now.

I think this issue needs to be taken care on 2 points:

  • input via qr code
  • changes via form input should be auto-translated

The solution for the second part would be to update via change events on the name fields. I would like to test my ideas before I commit anything or submit pullrequest.

Is there a way to start a local (working) dev environment?

The webapplication relies on an Identity and Access Management (IAM) Component used to manage the user accounts and to authorize the login. This has currently not been configured to run on local environment.

Maybe there is some kind of dev guide to get a local, rudimentary version of keycloak running for this? I installed a docker version for this and patched the keycloak.json call, but configuration on keycloak seems to be a intense task from my point.

For this issue there is no need to have all api calls present, I only need to get into the interface and open the registration/qr-code forms. Maybe another staging env, like i had on onboarding, would do the job?

@vaubaehn
Copy link

vaubaehn commented Jul 8, 2021

@janhoffmann
Great you're looking into this!

  • input via qr code
  • changes via form input should be auto-translated

The solution for the second part would be to update via change events on the name fields.

Not 100% sure, but wouldn't change events also work for QR code input? I assume same events are triggered when fields are filled out by QR module?

I hope they can provide you with a testing environment.

@vaubaehn
Copy link

vaubaehn commented Jul 8, 2021

Hi @ascheibal , thanks a lot for your kind reply.

this is currently not selected by the stakeholders and was postponed due to other important stuff.

Full understanding, but I see it's in your mind, and I hope it will get into your discussions soon.

Be also aware that most of test certificates that are issued in Germany come from other partners doing the RAT (Quicktest). It's up to their implementation of validating/correcting the input data.

Yes, I see. There are also different issues that can arise from insufficient 3rd-party-implementations that 'we' came across lately (and I hope you don't mind my pings too much in that context).

But there could be solutions regarding 3rd-party implementations: the transmission of the standardised names could be made optional. If fields are missing, the backend could generate them automatically before further processing. If fields are present, they could be used as transmitted (with or without validation from backend, whether they're correct. I'd prefer with validation...). In a strict approach, an error could be responded when the transmitted standardised names are not validated as correct against the regular names.

My concerns are, that wallet apps (CWA and others) began to implement person certificate containers that hold all kind of certificates (v,t,r). Correct matching of new certificates of a single person to the correct (pre-)existing container becomes crucial. From a pragmatic point of view you may say, it's a 'nice to have' when certificates are matched correctly to corresponding containers. From the 'common user' perspective I expect a high number of complaints, when some certificates are matched correctly, and for others (name mismatch) new containers are created.
There is a certain likelihood that containers may become more important in the future, when a sequence of tests is necessary to prove a regular testing effort. Regular tests were already necessary in the 'tourist opening model' of Schleswig-Holstein (i.e., negative tests results every 72 hours), and they're currently introduced in other countries (Thailand for example: https://reisetopia.de/news/ankuenfte-phuket-sandbox-projekt). If something like this would be (re-)introduced later on EU level (or single regions) due to newly increasing incidence rates, it would be good to be able to proof a series of tests via the person certificate containers in the wallet apps. Hence, a fail-safe matching algorithm is quite important.
Practically we already see how many problems arise with the vaccination certificates, when pharmacy staff accidentally enter superfluous spaces in one of the certificates, or caps are different between first and second cert.

Matching by standardised names that are automatically generated by input of regular names may not solve all problems but mitigate the most. A good algorithm to create standardised names can sanitize flawed input from manual entry of regular names.

Manually entering standardised names introduces the same problems like any manual input.

Beside this, it will help testing staff, doctors etc. to save some time in data entry.

Same to vaccination certificates, where the issue is more severe/can cause more trouble, and where the referenced issues point to - we do not implement the issuance service for them.

True. Probably it could be a good idea to involve IBM/UBirch into this discussion/process. Their apps seem also to make use of a container model, and from reviews in app stores it looks like they're already using standardised names for matching ('Name mismatch in CWA, but no issue in CovPass').

In general, I totally agree with you, but currently cannot say whether this feature will be included in the cwa-quicktest soon.

Yes, and I assume you're seeing similar reasons like me, why it could be a good idea to implement it. Thus, my long text above is only to structure some arguments if you want to use them to discuss in an upcoming meeting.

@janhoffmann
Copy link
Contributor

Not 100% sure, but wouldn't change events also work for QR code input? I assume same events are triggered when fields are filled out by QR module?

That's why I need to test this locally :-)

@vaubaehn
Copy link

vaubaehn commented Jul 8, 2021

@janhoffmann
Thanks for your effort!
Feel free to ping me when your PR is ready. My coding skills are limited but fair enough to check the algorithm.

@vaubaehn
Copy link

vaubaehn commented Jul 9, 2021

@ascheibal
In case there is a decision to implement ICAO conform autogeneration rather sooner than later, please check with @janhoffmann who will do it concretely.

@BrittaTSI
Copy link

Possibly reference implementation by T-Systems? (to be discussed)

@vaubaehn
Copy link

Hi @BrittaTSI ,
could you kindly share the current state of discussion? Is there already a roadmap of implementation?
Have a nice afternoon

@BrittaTSI
Copy link

Hi @vaubaehn - the issue is still in our backlog. There has been not decision yet by the development team on when to implement it.

@vaubaehn
Copy link

vaubaehn commented Jan 25, 2022

Hi @kerstin-oppermann-tsi and @BrittaTSI ,

may I kindly ask to re-raise following issues internally (again):

  • Implementation of automatic pre-filling of ICAO fields in the RAT portal (quicktest-frontend)
  • adjustments/improvements of quality assurance when checking for ICAO transformed standardized names during the "Abnahmetest" of the API partners

Why:
There are cases, when wallet apps (CWA/CovPass) are not able to group a set of certificates to the corresponding person (i.e., grouping vaccination/recovery/test certificates to the right person correctly). The grouping (=matching) works by comparing standardized forename + standardized surname + birthdate. When they match, they're grouped together to a person.
Grouping fails for two different reasons:
a) the (quantity of) information present in one certificate does not correspond with the (quantity of) information in another certificate. This is the case, when for example one certificate holds the "DR." title but the others don't. Or, one certificate holds a middle name, while the others don't. Or, someone married and changed the surname.
b) the information of normal forename and normal surname is the same between certificates, but the transformation (algorithm, manual entry) to standardized names following the ICAO rules was different for some certificates.
Until now, it's only a cosmetic problem, when wallet apps are not able to group.
But you will have noticed, that soon (ETA mid February) at least CWA but probably also CovPass/CovPassCheck will be able to detect the admission status (i.e., 2G +/B, 2G, ...) of a holder, depending on the stored certificates, and present a corresponding badge (CWA/CovPass) or even validate the status for admission to a venue (i.e., for 2G +/B, CovPassCheck will be able to scan a sequence of certificates). For this application it's a must, that certificates are grouped correctly. DCC ticketing via a validation service will also be affected.

When looking to error source a), TSI can't do much about it - except implementing a visual trigger to the RAT portal (display a text), that staff needs to ensure that the RAT certificate to be issued should exactly correspond to other already stored certificates (vaccination, recovery) in terms of forename, middlename surname, titles and birthdate. For API partners connected to the backend, it would be good to have a mailing to inform them, that it is recommended to implement a staff-client-feedback-loop to ensure that these data match.

When looking to error source b), TSI should now urgently implement the great suggestion of @ascheibal : the ICAO fields in the RAT portal should be pre-filled to avoid typos/wrong transformation to ICAO standard when entering data (still manually) thus failing grouping of certificates. The implementation of ICAO transformation for the RAT portal should exactly produce the results that would result from the algorithm for our Bundespersonalausweis.
To have the best quality for 3rd-party-implementations of API partners, the ICAO algorithm that you implement for the RAT portal must be mandatory for them. They should be informed to change/adapt/enhance their ICAO implementation according to the (error free) algorithm that you use.
For newly applying API partners, the quality assurance via the "Abnahmetest" needs to be enhanced, that wrong application of ICAO transformation can be detected from your side, before the partner is going into production.

Some sources where these issues had been discussed lately:
API partners with flaws in ICAO algorithm: corona-warn-app/cwa-documentation#810 & corona-warn-app/cwa-quick-test-backend#213
Long discussion about problems with grouping certificates: corona-warn-app/cwa-wishlist#705

Inviting some people into this issue to keep track or to facilitate internal discussion:
IBM: @oliver-steinbrecher , @molk-ibm
SAP: @mlenkeit , @thomasaugsten

For me it's not clear whether the issuance portal for issuing certificates via pharmacies already uses an automatted ICAO transformation. In this case it would be good to align IBM's and TSI's implementations. Same is true for GEMATIK applications/issuance of certificates via doctor's offices.

Please leave us a line, how you will pick up this issue (again).
If there is no possibility for implementation due to contracts, please leave a discrete sign so that user community can ask BMG to guarantee a funding for this.
Thank you!

FYI: @dsarkar @Ein-Tim @timokoenig @alexcimander

@vaubaehn
Copy link

Hi @thinkberg, unfortunately forgot to ping you. Please see #185 (comment)

@janhoffmann
Copy link
Contributor

@vaubaehn I totally agree. Correctly typing the standardized name is the most time consuming part in the hole registration process. On the other hand this is a guarantee for disaster, converting strings is a job for code, not for humans.

I want to stress one more point:
I was able to use the Portal used by our state vaccination teams, some special chars where stripped right away. In my opinion it's really important to do a reference implementation, share it and make it mandatory for all sides.

@thinkberg
Copy link

The name is standardized using ICAO rules. However, Sometimes name components are taken from different (automated) sources and may include parts not relevant or missing parts of the name. So its not the transliteration, its the data sources that make it hard. And that is nothing easily fixed.

@vaubaehn
Copy link

vaubaehn commented Jan 25, 2022

Hi @thinkberg , thanks for your comment.
I assume you're relating to Ubirch's parts of ICAO transliteration in the process?
The reason why I pinged you is, that there is still a heterogenity of current ICAO implementations from other stakeholders or there is no automated transliteration at all (standardized names are entered manually).

@janhoffmann pinned it to the point in one sentence:

it's really important to do a reference implementation, share it and make it mandatory for all sides.

This is why I (or we) kindly ask you, all stakeholders, to find and agree on one ICAO algorithm reference implementation, and make it mandatory for all connected parties.
This includes TSI's RAT portal and their API partners.

It is forseeable that a reasonable portion of the public will run into problems to prove their admission status (i.e., 2G +/B) when two certificates needs to be validated in a sequence and they can't be matched.
There are many reasons why that could fail, heterogenity in ICAO transliteration one important among them. But this is an issue that can be addressed well in a joint effort.

Maybe Ubirch's implementation could serve as the reference implementation for all?
Please discuss this internally.
Thanks a lot.

Edit: what I mean with ICAO transliteration heterogenity: for some API partners ICAO means, spelling everything in capital letters and replace white space with '<', while others implemented the full algorithm.

@thomasaugsten
Copy link
Member

Here you can find the ICAO Conversion guidelines for the DCC partners
https://github.com/corona-warn-app/cwa-quicktest-onboarding/wiki/Anbindung-an-CWA-mit-Verwendung-von-DCCs#icao-conversion

janhoffmann added a commit to janhoffmann/cwa-quick-test-frontend that referenced this issue Feb 3, 2022
The code implements a change detection on the name input fields and prefills the standardized name when reading the QR-Code

see corona-warn-app#185
@janhoffmann
Copy link
Contributor

@thomasaugsten thanks for the reference implementation.

I implemented the algorithm and used it on qr-code reading and added a change detection on name.

@kerstin-oppermann-tsi @ggrund-tsi @ascheibal please organize a review and test on my implementation, as I wrote the code without test env.

I really would appreciate if "my" instance would be updated with the change asap :-)

@janhoffmann
Copy link
Contributor

@ggrund-tsi let's have the discussion here:

I implemented the algorithm as stated in the wiki. will add missing chars according to pdf.

@janhoffmann
Copy link
Contributor

@ggrund-tsi i checked SpeakingUrl, i think we don't gain much profit by adding it and add the cost of an external lib we don't control.

The new MR has the cyrillic and arabic translations as defined in the standard.

One thing we all should agree on:
The solution here will never be perfect, because the correct (mathematical) solution depends on more information (f.e. the language) and some translations are not 1:1, but 1:n.
I believe this is the best we can do, without being the issuing government.

@vaubaehn
Copy link

vaubaehn commented Feb 4, 2022

@janhoffmann I hope you're fine with the ping: #290 (comment) ? If we could get a review from Ubirch, that would be the best outcome the implementation can have - not only for the RAT portal itself, but all connected workflows, thus more happy users (because the likelihood of correctly grouped recovery/vaccination & RAT test DCCs is inecreased) and more happy stakeholders (because less user complaints about ungrouped DCCs that bind much resources on SAP's/IBM's side)...

@janhoffmann
Copy link
Contributor

@vaubaehn of cause.

One more thought:
Maybe it would be wise to start with "my" proposed changes, even if we could not do a full sync with other stakeholders. The situation right now is, that most of the persons entering those values don't realize the effect and therefore might not care to do it right at all. Adding the autofill will help those persons a lot to gain better results.
Of cause we should aim to have all the same implementation, but we should not wait until we have all stakeholders on board to do a implementation here.

To be clear: I'm fine with any solution and it doesn't need to be my MR, but it needs to be done.

@vaubaehn
Copy link

vaubaehn commented Feb 4, 2022

@janhoffmann

but it needs to be done

I fully agree.

Of cause we should aim to have all the same implementation, but we should not wait until we have all stakeholders on board to do a implementation here.

The two goals are: getting better data quality into the RAT portal & providing the best possible reference implementation for future use. Logically, for me the reference implementation would be first step, then implementing it into the RAT portal as the second step. Screening/Reviewing your PR shouldn't be much effort, and just by having a fast look to yours, likely there won't be any changes nescessary.
Because that's not too complicated, my hope is that Ubirch could do this in a matter of days, when the value of their review is well understood there.

But in any case I'd agree that their review shouldn't be a long lasting blocker. Maybe waiting one week at most, and then move on anyway?

@janhoffmann
Copy link
Contributor

@vaubaehn Did you hear anything about this?

@vaubaehn
Copy link

vaubaehn commented Feb 14, 2022

Good morning, @janhoffmann , unfortunately I didn't hear anything until now. Due to acute disease I also can't do much currently, so please go ahead anyways.

@janhoffmann
Copy link
Contributor

@vaubaehn @ggrund-tsi any news on this topic?

@kerstin-oppermann-tsi
Copy link
Contributor

kerstin-oppermann-tsi commented Feb 22, 2022

As we have illness and vacations in the team, I tried to check the PR with cyrillic and arabic letters, but for me it didn't work. If I insert cyrillic or arabic letters into the name field they will not automatically transformed and inserted into the ICAO fields.For latin letters it works. But as @ggrund-tsi mentioned cyrillic and arabic letters are the important fields that have to fill. So we decided to wait for developers are back.

@vaubaehn
Copy link

@kerstin-oppermann-tsi
Thanks for your feedback! Great that you had a look into it, and that it's in your backlog.
I favor with @janhoffmann that highest priority is to get it implemented in a working version. However, as even the ICAO guideline shows some blurring with regard to a few special cases of transliteration, it would be great to align with Ubirch's algorithm. Do you, Kerstin, have a direct contact to Matthias Jugel from Ubirch to ask whether they can have a look to the here proposed algorithm, either before or after merging? Alternatively, Ubirch could publish their algorithm via their certification-api repository on GitHub, so that we can take care of the alignment.

@kerstin-oppermann-tsi
Copy link
Contributor

@vaubaehn no I don't unfortunatly. And as I said we had to wait for our main developers and scoping to look and decide what to do.

@janhoffmann
Copy link
Contributor

@kerstin-oppermann-tsi which version did you check?
Is there a way for me to get a running env to test the proposed changes?

@vaubaehn
Copy link

Hi @kerstin-oppermann-tsi ,
thanks for your reply!

And as I said we had to wait for our main developers and scoping to look and decide what to do.

Yes, for the technical aspects, that's the best.

However, I will open an issue at Ubirch's repo, and again try to establish a collaboration on the ICAO algorithm itself, as soon as I find some time. They should either have a look to the algorithm before or after TSI merged the working version of @janhoffmann 's PR to the RAT portal, or they could opernsource their algorithm, so we can adapt in at a later point in time just in case there are some unexpected deviations for some transliteration.

@vaubaehn
Copy link

vaubaehn commented Mar 3, 2022

Hi all,
I hope TSI devs have recovered and can have a look to @janhoffmann 's PR soon.
Independently, I openend an issue at Ubirch's repo: Digitaler-Impfnachweis/certification-apis#232
There are some examples that illustrate why it was good to have one single reference implementation (fuzzy transliteration even suggested in the ICAO guideline) and that Ubirch could provide their solution.

@ascheibal ascheibal linked a pull request Mar 27, 2022 that will close this issue
@BrittaTSI
Copy link

@ggrund-tsi @fOppenheimer - das Issue können wir schließen, richtig?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request Investigate
Projects
None yet
Development

Successfully merging a pull request may close this issue.