tom-and-the-toothfairies / pathways

:older_woman: :hospital: :pill: :pill:
https://tomandthetoothfairies.info/
5 stars 0 forks source link

DDI enrichment #179

Closed c-brenn closed 7 years ago

c-brenn commented 7 years ago

Adds the line number and label for each drug to the /api/ddis response.

We will need the line numbers for highlighting the drugs in the file and returning the label simplifies the frontend logic a bit.

As per this section of the updated README, the /apis/ddis response now has this structure:

{
  "ddis": [
    {
      "drug_a": {
        "uri": "http://purl.obolibrary.org/obo/DINTO_DB00214",
        "label": "torasemide",
        "line": 3
      },
      "drug_b": {
        "uri": "http://purl.obolibrary.org/obo/DINTO_DB00519",
        "label": "trandolapril",
        "line": 6
      },
      "label": "torasemide/trandolapril DDI",
      "uri": "http://purl.obolibrary.org/obo/DINTO_11031",
      "harmful": false,
      "spacing": 3,
      "category": "sequential",
      "enclosing_constructs": [
        {
          "type": "sequence",
          "line": 6
        }
      ]
    }
  ]
}

Previously, drug_a and drug_b were just strings (the DINTO uri for the drug).

c-brenn commented 7 years ago

There's no visible change in behaviour here - I haven't added the line highlighting for the participating drugs.

That change can be included in our UI overhaul - #177

c-brenn commented 7 years ago

closes #178

houli commented 7 years ago

I would just wonder about all the bits of the test that's no longer tested On Wed 29 Mar 2017 at 20:02, Peter Meehan notifications@github.com wrote:

@22a approved this pull request.

Looks great ! another one for the front end UI pile

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/tom-and-the-toothfairies/pathways/pull/179#pullrequestreview-29813444, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIx_A6tiq7IabPIc4gm-GRbXxvK3KlIks5rqqqfgaJpZM4MtWJa .

c-brenn commented 7 years ago

I removed a lot of the deep pattern matching from the tests as I felt they were overlapping a lot. I can add a few more tests if you think what we have isn't thorough enough.

houli commented 7 years ago

Maybe we can just add a few more assertions to cover the removed bits? On Wed 29 Mar 2017 at 20:10, Conor Brennan notifications@github.com wrote:

I removed a lot of the deep pattern matching from the tests as I felt they were overlapping a lot. I can add a few more tests if you think what we have isn't thorough enough.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/tom-and-the-toothfairies/pathways/pull/179#issuecomment-290194403, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIx_OvoUBgSqFRNEzwNlNe63ih96Y6eks5rqqxwgaJpZM4MtWJa .

c-brenn commented 7 years ago

I felt having the same structural assertion in all the test made them brittle. Changing the DDI information being returned was breaking tests that were about the category for example.

I moved that structural assertion into a new test.

houli commented 7 years ago

Yeah I guess that's ok to have in the controller test. The only thing we're not really checking anymore outside the controller test is the enclosing constructs which we might want to add. Probably just as more assertions on what's there

On Wed 29 Mar 2017 at 20:13, Conor Brennan notifications@github.com wrote:

I felt having the same structural assertion in all the test made them brittle. Changing the DDI information being returned was breaking tests that were about the category for example.

I moved that structural assertion into a new test.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/tom-and-the-toothfairies/pathways/pull/179#issuecomment-290195518, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIx_PwDPiLxBnK9NhW684cdpp5DSzDMks5rqq1xgaJpZM4MtWJa .

c-brenn commented 7 years ago

I'll add that to the repeated_alternative test