Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename configuration fields to be consistent with DHIS2 and make names localizable #6457

Closed
michaelkohn opened this issue Jun 1, 2020 · 20 comments
Assignees
Labels
Priority: 3 - Low Can be bumped from the release Type: Improvement Make something better
Projects

Comments

@michaelkohn
Copy link
Member

michaelkohn commented Jun 1, 2020

Current CHT/DHIS2 integration configuration uses terminology that is not entirely consistent with DHIS2. Excerpts from the documentation are below and the full documentation is here.

A few suggestions:

  • In app_settings, rename guid to id
  • In app_settings, rename label to translation_key
  • Make translation_key translatable

We might also consider changing dhis to dhis2, though who knows if there will ever be a DHIS3.

For reference, here is DHIS2 documentation for sending data sets.

App Settings

CHT Core....

{
  "dhisDataSets": [
    {
      "guid": "VMuFODsyWaO",
      "label": "MoH Monthly Report"
    }
  ],
}

From DHIS2, if you download metadata for Data Sets, it looks like this:

"dataSets": [
    {
      "lastUpdated": "2020-06-01T17:48:22.355",
      "id": "Xa2mJ5ExpMP",
      "created": "2020-06-01T15:58:56.545",
      "name": "AAA-data set1",
      "validCompleteOnly": false,
      "dataElementDecoration": false,
      "publicAccess": "rw------",
....

Contact document

{
  "_id": "my_place",
  "type": "health_center",
  "dhis": {
    "orgUnit": "HJiPOcmziQA",
  }
}

Targets.js

module.export = [
  {
    id: 'births-this-month',
    type: 'count',
    icon: 'icon-infant',
    translation_key: 'targets.births.title',
    subtitle_translation_key: 'targets.this_month.subtitle',
    goal: -1,
    appliesTo: 'contacts',
    appliesToType: ['person'],
    appliesIf: contact => !!contact,
    date: (contact) => contact.contact.date_of_birth,
    dhis: {
      dataElement: 'kB0ZBFisE0e',
    }
  },
]
;
@michaelkohn michaelkohn added the Type: Bug Fix something that isn't working as intended label Jun 1, 2020
@garethbowen
Copy link
Member

Instead of name can we use translation_key? That would make it consistent with other places we have translatable strings.

@michaelkohn
Copy link
Member Author

Sure

@garethbowen
Copy link
Member

Although I see the name is exported as well. What is the name used for in the export? Maybe we should have both an export name and a translation key?

@michaelkohn
Copy link
Member Author

DHIS2 does not need the name of the Data Set when you are importing it, so I'm not 100% sure why name would be included in the export.

@garethbowen
Copy link
Member

@kennsippell Do you know? Is it just a human readable string to make it easier to scan the file?

@kennsippell
Copy link
Member

kennsippell commented Jun 1, 2020

@garethbowen Where do you see name being exported? The contact.name is used in the admin/src/js/controller/export-dhis.js to create a list of "human readable" orgUnits. But these aren't translatable.

I don't expect to see a name in the exported payload. Here is an example payload.

Do you mean that the label is being exported? When using the "Human Readable" mode for DHIS2 exporting, the dataSet.guid is replaced with the dataSet.label. This label wasn't made to be a translation key in the MVP since 1) admin console doesn't support translations, and 2) it didn't seem necessary for the human readable export format (unsure if anybody even uses this). Seems like a positive change if Admin Console is going to support translations.

I did purposefully just use dhis instead of dhis2 incase there is eventually a dhis3 etc.

@garethbowen
Copy link
Member

  1. admin console doesn't support translations

Actually it does, for example, the DHIS2 export page is fully translated. We just don't necessarily ship all the admin console translations for all the supported languages out of the box.

Where do you see name being exported?

I saw it in the description of this issue: "From DHIS2, if you download metadata for Data Sets, it looks like this:". It could be that the documentation is wrong?

@kennsippell
Copy link
Member

kennsippell commented Jun 1, 2020

@michaelkohn can confirm, but I think he is exporting a dataset via DHIS2 - in this case the name is the value given as the name of the dataSet via DHIS2.

The CHT only supports the exporting of a dataValueSet which does not reference a name.

@michaelkohn
Copy link
Member Author

Correct. Currently the label field in the CHT is just what shows up in the dropdown of the Admin App. Likewise, the name field in DHIS2 is what shows up in the dropdown in the Data Entry app. So we would be translating the contents of a drop down, NOT the field label. This value does not get sent to DHIS2.

Screen Shot 2020-06-01 at 6 25 15 PM

Screen Shot 2020-06-01 at 6 26 42 PM

@michaelkohn
Copy link
Member Author

I saw it in the description of this issue: "From DHIS2, if you download metadata for Data Sets, it looks like this:". It could be that the documentation is wrong?

I included the DHIS2 metadata export so you could see what they call the id and name fields (what we are calling guid and label).

@garethbowen garethbowen self-assigned this Jun 2, 2020
garethbowen added a commit to medic/medic-docs that referenced this issue Jun 2, 2020
@garethbowen garethbowen added Type: Improvement Make something better Priority: 3 - Low Can be bumped from the release and removed Type: Bug Fix something that isn't working as intended labels Jun 2, 2020
@garethbowen garethbowen added this to To do in 3.9.0 via automation Jun 2, 2020
@garethbowen
Copy link
Member

I've put this in 3.9.0 because changing the configuration schema after release would break backwards compatibility.

@garethbowen garethbowen moved this from To do to In progress in 3.9.0 Jun 2, 2020
@SCdF SCdF moved this from In progress to In AT in 3.9.0 Jun 2, 2020
@SCdF
Copy link
Contributor

SCdF commented Jun 2, 2020

Ready for AT in 6457-fix-dhis-config-schema

@newtewt
Copy link
Contributor

newtewt commented Jun 2, 2020

@garethbowen The admin app doesn't seem to respect your selected language on your profile because none of it is translated or maybe I'm missing all the keys for Spanish. Adding a new translation_key for the DHIS dataset though shows the English translation in the drop down even though my users account is set to Spanish.

image

@newtewt
Copy link
Contributor

newtewt commented Jun 2, 2020

I'm also seeing an extra property when I download my app_settings.

    "dhisDataSets": [
        {
            "id": "test1test2test4",
            "translation_key": "data.dhis",
            "$$hashKey": "object:1244"
        }

@newtewt
Copy link
Contributor

newtewt commented Jun 2, 2020

Otherwise I think the changes for this are working ok. The export is occuring and using the updated values.

As an aside I am confused how this works in general. I used our births-this-month example and see the target as my offline user but when exporting the value is 0 in the dataValues

@garethbowen
Copy link
Member

I'm also seeing an extra property when I download my app_settings.

I think that'll be unrelated to this change. I think what's happening is when rendering the dhisDataSets angular adds the $$hashKey for internal use, then at some point you've saved the config anywhere in the app management app which has saved the property inadvertently. Please raise a separate issue for this.

@garethbowen
Copy link
Member

Merged and backported to 3.9.x. I've also merged the medic-docs PR but not the beta docs site one as it's waiting for review.

3.9.0 automation moved this from In AT to Done Jun 2, 2020
abbyad pushed a commit to medic/cht-docs that referenced this issue Jun 2, 2020
@michaelkohn
Copy link
Member Author

FWIW.... I'm seeing some inconsistencies in how we name properties and deal with translation keys.


In some of our documentation I notice that the property is a normal field name like "title" and the type is of translation key. See tasks.js

Screen Shot 2020-06-03 at 10 19 56 AM


Sometimes I see that the property is called "translation_key" with a type of translation key. See targets.js

Screen Shot 2020-06-03 at 10 21 38 AM


Sometimes the property is a normal field name but with no type column. See hierarchy.

Screen Shot 2020-06-03 at 10 22 35 AM

Sometimes the property is called "translation_key" with a type of string. See JSON forms

Screen Shot 2020-06-03 at 10 23 55 AM

@garethbowen
Copy link
Member

Thanks for flagging that! The property names are inconsistent for historical reasons and can't be changed without breaking backwards compatibility so I'm ignoring those for now. I think the type should be "string" because "translation_key" is a Medic convention. The fact that it's a translation key should be explained in the description. @abbyad What do you think?

@michaelkohn
Copy link
Member Author

FWIW... my preference is that it should be something like this:

Property Type Description Required
name string Translation key for the name of the data set as displayed in the UI. e.g. dhis.dataset.name.moh515 Yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 3 - Low Can be bumped from the release Type: Improvement Make something better
Projects
No open projects
3.9.0
  
Done
Development

No branches or pull requests

5 participants