akvo / akvo-flow

A data collection and monitoring tool that works anywhere.
http://akvo.org/products/akvoflow/
GNU Affero General Public License v3.0
65 stars 31 forks source link

Add potassium strip test #1895

Closed nhternup closed 7 years ago

nhternup commented 7 years ago

Add potassium (Merck) test type

More info: https://github.com/akvo/akvo-caddisfly/issues/81

nhternup commented 7 years ago

Tests are defined in tests.json file https://github.com/akvo/akvo-caddisfly/blob/develop/caddisfly-app/app/src/main/assets/tests.json

valllllll2000 commented 7 years ago

@nhternup I am a bit confused about this one. So I looked in https://github.com/akvo/akvo-caddisfly/blob/develop/caddisfly-app/app/src/main/assets/tests.json and found:

{
      "name": "Potassium",
      "description": "Potassium, 0 - 1000 ppm.",
      "subtype": "striptest",
      "tags": ["water quality", "soil quality"],
      "uuid": "a18118e7-6382-4d16-b5fb-55f97cd99083",
      "calibration": "measured on 18-11-2016 using X-Rite iPro2",
      "brand": "MERCK 117985",
      "groupingType": "INDIVIDUAL",
      "illuminant": "D65-2deg",
      "length": 74,
      "height": 6,
      "unit": "mm",
      "hasImage": true,
      "results": [
        {
          "id": 1,
          "name": "Potassium",
          "unit": "ppm",
          "patchPos": 4.5,
          "patchWidth": 7,
          "timeLapse": 0,
          "colors":[
            {
              "value": 0,
              "lab": [78.90,12.62,47.41]
            },
            {
              "value": 250,
              "lab": [73.59,24.18,53.73]
            },
            {
              "value": 450,
              "lab": [65.05,41.19,55.50]
            },
            {
              "value": 700,
              "lab": [59.62,50.94,54.60]
            },
            {
              "value": 1000,
              "lab": [53.77,58.94,52.64]
            }
          ]
        }
      ]
    }

but all the other sensors are listed in this format in https://github.com/akvo/akvo-flow/blob/develop/GAE/src/resources/caddisfly/caddisfly-tests.json like for example:

{
            "subtype": "striptest",
            "uuid": "beca9731-63f4-434f-a3c2-ac33b44f9992",
            "tags": ["water quality"],
            "name": "EZ Arsenic High Range Test Kit (0-4000 range)",
            "brand": "HACH 2822800, 50 pcs",
            "description": "Measures arsenic levels. This is a dual range test kit - choose this test when the 0-4000 ppm range is used.",
            "numResults": 1,
            "hasImage":true,
            "results": [{
                "name": "Arsenic",
                "id": 1,
                "unit": "ppm"
            }]
        }

Obviously I will reorder the fields to match the order in akvo-flow file but also there are all these extra fields, are they all necessary? Also numResults seems to be missing from the original caddisfly file but is present for all the sensors.

nhternup commented 7 years ago

@valllllll2000 The idea is that the entire caddisfly-tests.json in Flow can be just overwritten with tests.json from the Caddisfly app repo. So the easy way would be to copy the tests.json contents and paste into caddisfly-tests.json. The extra fields are not necessary and it is assumed that they will be ignored by Flow. But this has to be tested.

Going forward this would be implemented hopefully as an automated script to copy the file there and any other places as required.(e.g. web service). This way we hope to avoid creating different versions of the file and keep the file in Caddisfly app repo as the master.

Also numResults is not required as the size of the results array should provide that number. It is unlikely that Flow is actually using this field but this has to be reviewed.

janagombitova commented 7 years ago

@nhternup, @valllllll2000 and @muloem I do not want to get in the middle of this, but @nhternup isn't your answer connected to planned change in the implementation? The idea with adding these tests for this release was to follow the original implementation first and then later in time refactor the set up. Please do correct me if I am mistaken.

nhternup commented 7 years ago

@valllllll2000 @muloem @janagombitova yes but I guess this can be copied across manually for now. The extra fields should still be ignored.

valllllll2000 commented 7 years ago

this has been handled by issue #1915