ehn-dcc-development / eu-dcc-schema

Schema for the ehn DCC payload
Apache License 2.0
165 stars 59 forks source link

Update vaccine-medicinal-product.json #109

Closed kruzikh closed 1 year ago

kruzikh commented 3 years ago

To allow validation of the relation between product and product manufacturer in the vaccination certificate a new element "manf-code" has been added into the "vaccine-medication-product" value set. Each vaccine is identified using its EMA registration number or - for vaccines that do not have EMA registration yet and has be registered only by national authorities - a vaccine name. Each registration is linked to one marketing authorization holder or manufacturer entry from the "vaccine-mah-manf" value set but information about this relation is currently missing in the value sets distributed by DCCG and cannot be used for certificate validation. This change overcomes this problem.

SchulzeStTSI commented 3 years ago

Can we maybe change the "manf" field to a more generic one that it fits to other valuesets too? Something like "linkedSet" and "linkedValue" that it can be used in other sets as well:)

kruzikh commented 3 years ago

Thanks for adding a more complete description for the PR, that helps a lot.

Minor changes - but changes nevertheless. Is there an official approval statement from Semantic SG for these changes? There should be if we are to follow correct change management procedure: there needs to be some kind of formal sign-off for the changes. Where can I find it?

I don't think that we need approval from the SgS. This is a purely technical change according to my opinion. We are not going to modify the value set itself (that would need SgS agreement) - we are adding a relationship information between existing value sets.

kruzikh commented 3 years ago

Can we maybe change the "manf" field to a more generic one that it fits to other valuesets too? Something like "linkedSet" and "linkedValue" that it can be used in other sets as well:)

Stephen, good suggestion. If I understood well, you propose to add two fields - linked valueset and linked value. Is that right?It make sense, however I don't see any other example of two value sets that need to be linked. At least not in current 3 types of the certificates.

ryanbnl commented 3 years ago

Thanks for the detailed description, that's perfect :-)

The actual changes themselves look good, I've double checked all of the codes and they're all correct.

I think that Steffen is suggesting that you rename the field? I would prefer to see the manf_code and manf_name in different fields. This is more explicit for implementers. You could even do something like this:

{ 
  "linked_manufactorer":  {
    "manf_code": "xxx",
    "manf_name": "xxx"
  }
}
gabywh commented 3 years ago

Thanks for adding a more complete description for the PR, that helps a lot. Minor changes - but changes nevertheless. Is there an official approval statement from Semantic SG for these changes? There should be if we are to follow correct change management procedure: there needs to be some kind of formal sign-off for the changes. Where can I find it?

I don't think that we need approval from the SgS. This is a purely technical change according to my opinion. We are not going to modify the value set itself (that would need SgS agreement) - we are adding a relationship information between existing value sets.

If I look at the file, it's changed. That means it comes under change management process. If you wish to keep to procedure (which I think is a good thing) then we need to keep to procedure. Can you please send me a link to the change procedure process for Semantic SG as I am currently getting rather mixed messages and it the choices about when to apply process and what is required for process seem somewhat arbitrary. If the process does exist that you refer to then (1) all of these points would be addressed in it, and (2) please send a me a link so I can see the process for myself and what my contact points with it should be.

gabywh commented 3 years ago

Can we maybe change the "manf" field to a more generic one that it fits to other valuesets too? Something like "linkedSet" and "linkedValue" that it can be used in other sets as well:)

Flexible, generic but also meaningless.

I do not know simply from looking at this what you mean by a linked set / value? This could either be a straight 1:1 mapping, in which case over time you end with a spaghetti mess of cross-coupling, or it could be a new value/link that is derived in some way, probably from the values it is linking.

Once you have tjhe name "linkedValue" in there, then you can only have that one linked value. Further linked values must then necessarily be named something different than linked value, even though they are also linked values. So then you end up having to name them something other than linked value for all but the first one.

What you are attempting to do here is to hack together a relational model of the data in the valuesets. The relational data modelling approach has been known for decades, is mature and well-documented and used the world over. If there is a need to have a relational model for the data in the valuesets, then it should be generated.

gabywh commented 3 years ago

I would prefer to see the manf_code and manf_name in different fields. This is more explicit for implementers. You could even do something like this:

Agreed. The field name "linked value":

  1. tells me nothing about the data, except that it is linked (but do I care?), and
  2. describes how the field is implemented, when the field name should be telling me something about the kind of data in it, and
  3. can only be used once - if you have multiple linked values then you have at least two problems:
    1. one of the fields will be called "linked value" and the other not, so you have a self-contradiction in the same document
    2. you start to build up a rat's nest of high coupling and low cohesion
{ 
  "linked_manufactorer":  {
    "manf_code": "xxx",
    "manf_name": "xxx"
  }
}

+1 This approach is much better IMHO - clear and tells me what I need to know. Analogous to a linking table in entity-relationship (ER) data modelling - worked for people all over the world for decades already, I guess it could also work for us ;)

SchulzeStTSI commented 3 years ago

I meant it more generic. Something like "linked_sets":[{ "name": "(setid)", values:["..."], "version":??,...}] to be generic across all valuesets. Maybe we should consider 1:n relations:


"valueSetValues": {
   "EU/1/20/1528": {
    "display": "Comirnaty",
     "linked_sets":[
                            {
                                setId:"vaccines-covid-19-auth-holders",
                                valueIds:["ORG-100001699",...]
                             },
                            {
                                setId:"Another-valueSet",
                               valueIds:["Value1","Value23"]
                            }
                           .....
                         ]
     "lang": "en",
     "active": true,
     "system": "https://ec.europa.eu/health/documents/community-register/html/",
     "version": ""
   }
gabywh commented 3 years ago

Any update? Over 7 days old and I believe Semantic SG met yesterday. I will close PR shortly if no progress here.

ryanbnl commented 1 year ago

Closing this as it's covered by the new vaccine-encoding-instructions.json, so there's no need to change the structure (and risk breaking things).