Hi,

Good afternoon,

A suggestion that comes to me to improve the code cleanliness and how to work with it.

    def get_vaccine_by_identifier(self, identifier):
        """Gets a vaccine by the identifier in the JSON file"""
        for vaccine in self._known_vaccine_list:
            if vaccine.vaccine_identifier == identifier:
                return vaccine

        raise ValueError("Unknown vaccine")

If we pass self._known_vaccine_list as a dict() we could just use

    def get_vaccine_by_identifier(self, identifier):
        """Gets a vaccine by the identifier in the JSON file"""
        if  vaccine.vaccine_identifier in VACCINE_DICTIONARY:
                return True, vaccine
        else:
                raise ValueError("Unknown vaccine")

And dictionaries can be changed to JSON whenever we want, but we gain speed and stability when working with them.

It's a suggestion, maybe it has a dark side that I don't see.

Let's not forget that dictionaries can also be serialised, stored and loaded from main memory to persistent memory.

Regards

They have been corrected again, adapting the following to the new code generated by @NComannder:

• Markdown syntax violations.
• Python 3 syntax violations
  • In addition, the imports have been tidied up
  • The code, without altering its functionality, has been formatted for better readability, as this is the philosophical purpose of Python 3.
• XML syntax violations
• JSON syntax-structural violations

The problem in merging the branches was the absence of a file that has been deleted. Now it's fixed.

Reverts NCommander/vaksina#13

Github displays a very ugly vertical table with this landed. Maybe we need to generate an AUTHORS file, but this isn't a solution.

Include metadata in documentation

On this occasion, if it seems relevant, I have added a YAML header (it is part of the accepted Markdown) in order to better format the document and add metadata to the README.md

Also, whether you want to do the binding or not, at least it is now signed as requested. :)

Regards.

The result of running test.py:

{
   "cards":{
      "card0":{
         "card_type":"smart_health_card",
         "issued_by":"https://spec.smarthealth.cards/examples/issuer",
         "persons":{
            "person0":{
               "name":[
                  "Jane C. Anyperson"
               ],
               "dob":"1961-01-20",
               "immunizations":[
                  {
                     "vaccine_administered":"PFIZER_COMIRNATY",
                     "date_given":"2021-01-01",
                     "lot_number":"0000002"
                  },
                  {
                     "vaccine_administered":"PFIZER_COMIRNATY",
                     "date_given":"2021-01-29",
                     "lot_number":"0000008"
                  }
               ]
            }
         }
      }
   },
   "validations":[
      {
         "validation_method":"osha_1910_501",
         "results":{
            "Jane C. Anyperson":"success"
         }
      }
   ]
}

Note:

In your example in #16 in results you refered to the person using their reference id (person0), however assuming several cards get passed to the validator, wouldn't this id be duplicated in every card? Since the id is assigned locally separately for each card:

class Card(object):
    def __init__(self):
        ...
        self.persons = {}
        self._person_count = 0

    def add_person(self, card):
        person_key = "person" + str(self._person_count)
        self.persons[person_key] = card
        self._person_count = self._person_count + 1

Thus, for now I am using the persons name(s) instead. However, this of course has the potential for trouble if several people share the same card and name (parent and child for example).

• fixed typos in code
• cleaned up some code (PyCharm linter errors/warnings)
• fixed parse_immunization_record?

On the last one (parse_immunization_record), the comment inside it states:

It's possible that multiple vaccines can be given in single day. This isn't done for COVID per se, but because FHIR is a general purpose specification, we should handle this, especially if there are future multishot COVID vaccinations that are given at later point, because data structures are important

If I understand this correctly, the method should handle multiple immunization records, store them in an list and return them all. If that is the intended behaviour, then I fixed it. If not then this should be reverted, and the method refactored, since before it created a list of imunizations that was filled but never used, and the method only ever returned the last parsed immunization, and if the received dictionarry was empty/had no immunizations, the method would return a variable before initialized (so my PyCharm linter was freaking out on that :P)

• typos in docs
• added some links (Alpine, Ubuntu Core and etc.) to the docs
• typos in tests
• fixed duplicate test names
• changed file paths from relative to absolute. Otherwise, tests would raise FileNotFound in PyCharm (on Windows)

So as of right now, Vaksina is using a very simplistic design which has a JSON file with all know keys from the VCI issuers list. This doesn't handle key revocation (which is a custom rolled thing in SHC and is semi-complex), but essentially, we need to do the following.

Each SHC has a specific signing key in the iss field, but that doesn't specifically denote who actually issued a given card, and that should be available in plain text if possible (this will also be true for other card types later) through the API. For fully offline operation, we need to be able to generate a datafile that has all the information in a single go, and then load it as needed.

In practice, the key management tool needs to do the following:

• Download all known VCI key signers from (https://github.com/the-commons-project/vci-directory)'s metadata
  • The keys are specifically at VCI base path + /.well-known/jwks.json, and are represented as a JSON Web Keyset format
  • However, the CRL system is unique
• Merge VCI metadata with the VCI signers in such a way that all key information is available in a single go
• If CRL support is defined (aka crlRevision: 1 is present in the JWK object), then we need to do additional steps
  • CRLs are defined on a per key basis, and refer to the rid object on a given card.
  • We need to download `/.well-known/crls/kid.json to get a list of revoked keys
  • This file needs a validation check before incorporated in the dataset.
• Have a local database of keys. As of last run, the key database is 210 kb JSON file. That may or may not be acceptable to load at library instance.

While we could dynamically fetch a pubkey for an unknown issuer, I question if that's really a door I want to open ...

The following tools need to be implemented:

• Define a serialization type for the key signer database
• Implement a tool to download the VCI list and serialize it
• Tools to create a local test signing CA for development purposes

We need to test the following scenarios to make sure we're handling this properly

• Download and validate a JWS claim for a given key (this is coded, but not unit tested)
• Ensure expiration and NBF (not valid before) is handled
• Check revocation status (example03 of SHC data is a revoked example)
• Model the issuer somewhere in Card information properly

There's probably more I'm forgetting, but this is a relatively good baseline in which to start

While I was beginning to refactor code to handle vaccinationresults, I had a thought. Currently, in API.md, we define an example result as such:

{
    "card_validation_status": "good",
    "card_decoded_content": {
        "card_type": "smart_health_card",
        "persons": [
            {
                "names" : [
                    "John A. Person"
                ],
                "dob": "01-05-1950",
                "immunizations": [
                    {
                        "vaccine": "MODERNA",
                        "given_on": "01-01-2021",
                        "lot_number": "1"
                    },
                    {
                        "vaccine": "MODERNA",
                        "given_on": "01-29-2021",
                        "lot_number": "20"
                    }
                ]
            }
        ],
        "issued_by": "Example Issuer"
    },
    "validation_error": ""
}

However, this doesn't work properly because a validation result needs to handle a person, and it is legal per FIRS/SHC for multiple persons/patients to be within one dataset, and when I brought this up on SHC (https://github.com/smart-on-fhir/health-cards/issues/223), this seems to be by design.

As such, we refactored the card representation/results to look like this:

{
  "card_type": "smart_health_card",
  "issued_by": "https://spec.smarthealth.cards/examples/issuer",
  "persons": {
    "person0": {
      "name": [
        "John B. Anyperson"
      ],
      "dob": "1951-01-20",
      "immunizations": [
        {
          "vaccine_administered": "MODERNA",
          "date_given": "2021-01-01",
          "lot_number": "0000001"
        },
        {
          "vaccine_administered": "MODERNA",
          "date_given": "2021-01-29",
          "lot_number": "0000007"
        }
      ]
    }
  }
}

with the expectation that the end result would look like this (abbreviated):

{
    "cards": {
        "card0": {
            "persons": {
                "person0": {
                    "person_object": "data goes here"
                },
            }
        }
    },
    "validation": {
        "person0": "good"
    }
}  

This seemed fine at the time, but I don't think its flexible enough. For one, it makes the assumption that a given vaksina instance only has a single validator, and well, I can envision user stories where I can see multiple validators may need to be run (i.e., someone having to be checked for multiple criteria for different locations).

I'm thinking we need to model this list so:

{
    "cards": {
        "card0": {
            "persons": {
                "person0": {
                    "person_object": "data goes here"
                }
            }
        }
    },
    "validations": [
        {
            "validation_method": "osha2022",
            "results": {
                "person0": "good"
            }
        }
    ]
}

This might be more a job for the API than the library itself, but I don't want to make this difficult to implement either. It may be worth having REST endpoints for different validations, but that doesn't handle cases where something is interfacing w/ the library directly. I'm undecided on how best to represent the validation results as of right now, or if I should handle it in the core library at all ...