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

First vaccination dose shows up as complete for recovered people (EXPOSUREAPP-8896) #3928

Conversation

SamuraiKek
Copy link
Contributor

We already had the logic to show the vaccination as complete if the doseNumber == totalSeriesOfDoses, what was left was to hide the Vaccination Info box that shows up if the vaccination is incomplete or it's complete but the number of days until immunity hasn't been reached.

To test, scan one of the presets from this PR. The vaccination card should have the complete icon (the one with the chekmark) and it should say "Vaccination 1 of 1". Also the vaccination hint is not shown.

@SamuraiKek SamuraiKek added the maintainers Tag pull requests created by maintainers label Aug 16, 2021
@SamuraiKek SamuraiKek added this to the 2.9.0 milestone Aug 16, 2021
@SamuraiKek SamuraiKek requested a review from a team August 16, 2021 14:44
@mtwalli mtwalli self-assigned this Aug 16, 2021
Copy link
Contributor

@mtwalli mtwalli 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, please add unit tests covering different vaccine does?

@sonarcloud
Copy link

sonarcloud bot commented Aug 17, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

80.6% 80.6% Coverage
0.0% 0.0% Duplication

@mtwalli
Copy link
Contributor

mtwalli commented Aug 18, 2021

Presets old/new look good, but despite that 1:1 has full immunity when validating the rules it displays that it is not valid in the selected country and have to wait 14 days. Can you please check with Max if we need to change anything?

Screenshot 2021-08-18 at 11 36 11

@SamuraiKek
Copy link
Contributor Author

Presets old/new look good, but despite that 1:1 has full immunity when validating the rules it displays that it is not valid in the selected country and have to wait 14 days. Can you please check with Max if we need to change anything?

Alright, I'll talk to him, I don't think the tech spec mentions this https://github.com/corona-warn-app/cwa-app-tech-spec/pull/91/files

@mlenkeit
Copy link
Member

@mtwalli @SamuraiKek thanks for checking and raising this. It's a problem with the rule definition and not directly related to this change. I'll bring it up with the responsible people to adjust the rule definition. But you can proceed and merge the PR (if that's the only thing that was preventing the merge) 👍

Copy link
Contributor

@mtwalli mtwalli left a comment

Choose a reason for hiding this comment

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

Lgtm

@axelherbstreith axelherbstreith self-assigned this Aug 18, 2021
Copy link
Contributor

@BMItr BMItr left a comment

Choose a reason for hiding this comment

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

good tests.
lgtm

@BMItr BMItr self-assigned this Aug 18, 2021
@SamuraiKek SamuraiKek merged commit 4fb6581 into release/2.9.x Aug 18, 2021
@SamuraiKek SamuraiKek deleted the feature/8896-first-vaccination-complete-for-recovered-people branch August 18, 2021 13:47
@vaubaehn
Copy link
Contributor

Hi @rgrenz (and @mlenkeit after your return to work) @mtwalli and @SamuraiKek

@mtwalli @SamuraiKek thanks for checking and raising this. It's a problem with the rule definition and not directly related to this change. I'll bring it up with the responsible people to adjust the rule definition. But you can proceed and merge the PR (if that's the only thing that was preventing the merge) 👍

unfortunately this is not only an issue with the business rules definitions alone, but also with the DCC schema (1.3.0).
To validate a single vaccination shot after a recovery as valid immediately, it is necessary to validate the combination of two separate events: vaccination AND recovery. Unfortunately, current DCC spec does not allow for multiple health events in a single certificate, but only vaccination OR recovery (oneOf):
https://github.com/ehn-dcc-development/ehn-dcc-schema/blob/a603410d760fefc9073931c8c807759d9714c136/DCC.combined-schema.json#L8-L33
This is also expressed through the General Rules in the business rules of some countries,

e.g., Ireland
{
      "Type":"Acceptance",
      "SchemaVersion":"1.0.0",
      "Engine":"CERTLOGIC",
      "EngineVersion":"0.7.5",
      "CertificateType":"General",
      "Description":[
         {
            "lang":"en",
            "desc":"Certificate relates to more than one health event"
         }
      ],
      "ValidFrom":"2021-07-12T00:00:00Z",
      "ValidTo":"2030-06-01T00:00:00Z",
      "AffectedFields":[
         "r",
         "t",
         "v"
      ],
      "Logic":{
         "===":[
            {
               "reduce":[
                  [
                     {
                        "var":"payload.r"
                     },
                     {
                        "var":"payload.t"
                     },
                     {
                        "var":"payload.v"
                     }
                  ],
                  {
                     "+":[
                        {
                           "var":"accumulator"
                        },
                        {
                           "if":[
                              {
                                 "var":"current.0"
                              },
                              1,
                              -1
                           ]
                        }
                     ]
                  },
                  -1
               ]
            },
            1
         ]
      },
      "Identifier":"GR-IE-0000",
      "Version":"1.0.1",
      "Country":"IE"
   }

So, to get such a vaccination (after recovery) successfully validated with business rules, it would also require an extension of the DCC schema (and new business rules after).

Either the oneOf constraint would need to be relaxed, or a new health event (vaccination after recovery) would need to be introduced.

Are there other member countries that also accept vaccinations without 14-day waiting period, when a person was recovered? Then there is a good chance to get an agreement on EU level to enhance the DCC schema accordingly.
I don't know how are your connection to the institutions in charge for this... If there is no better connection, then I guess the flow to get it implemented would be like
SAP -> RKI -> BMG -> EU -> national semantic subgroups/TSI -> national IOP subgroups

@vaubaehn
Copy link
Contributor

Additional thought:

Maybe it's possible to treat 'Vaccination for Recovered' as 'Booster Vaccination'? At least the same problems will arise technically: a 3/3 (or similar) for non-recovered should also be 'valid immediately', like the 1/1 for recovered...
How will Business Rule validation be solved for boostered people?

@vaubaehn
Copy link
Contributor

vaubaehn commented Aug 27, 2021

Some additional hints:

In general it would be possible to add additional/customized fields to the DCC schema for 'Vaccination after Recovery' or even boosters, see: ehn-dcc-development/eu-dcc-schema#123. But this would stay a Germany-only solution, and not get a "pass" in business rule validation for other countries.

To understand why only one health event is allowed currently, you may read here:
ehn-dcc-development/eu-dcc-schema#110
So it is open for a future change in general, but EU Legal Dep. would need to be convinced to change legislation. see ehn-dcc-development/eu-dcc-schema#110 (comment) and the two following comments. So it's probably more easy to add a new health event than to allow for vacc AND recovery in one single cert.

@vaubaehn
Copy link
Contributor

Last notes before the week-end:
There is now some valuable additional information regarding multiple health event in one single cert, provided by "the" central person involved in developing the DCC schema:
ehn-dcc-development/eu-dcc-schema#110 (comment)

In short:

  • Having multiple health events in one cert requires change in EU legislation
  • after a change, business rules may need adjustment
  • this was/is a long lasting discussion, and member states somehow missed to take influence while the schema was in draft and before legislation was made
  • it will take good arguments to achieve a change
  • the use case, why it is necessary to allow multiple health events could e.g. be presented by the (our German) representative in charge at the eHN Coordinated Actions meeting

From all other comments in that issue I conclude that it would be good to involve BMG in any case.

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.

6 participants