Lightmatter / welkin-health

A Python wrapper of the Welkin Health API
https://welkin.readthedocs.io
Other
4 stars 3 forks source link

_patient_info CDT pagination breaks the client #55

Open edcohen08 opened 1 year ago

edcohen08 commented 1 year ago

The _patient_info CDT meta_key value is just a string 'INSTANCE', we expect a dictionary

samamorgan commented 11 months ago

@edcohen08 Can you illustrate the steps to reproduce this bug?

edcohen08 commented 11 months ago
  1. In the welkin designer created a cdtf under 'Patient Info'
  2. Populate that field for a patient
  3. Make a call to welkin.Patient(id=...).CDTs.get(cdt_name="_patient_info") @samamorgan
samamorgan commented 11 months ago

Discovered another bug in relation to this.

If you call patient.CDTs().get() with no arguments, welkin.models.cdt.CDTs.get calls the patient endpoint ({instance}/patients/{patient_id}).

Created #85 to track this.

samamorgan commented 11 months ago

The data returned by this specific CDT is pretty different than others. Example:

{
  "name": "_patient_info",
  "data": {
    "content": [],
    "pageable": "INSTANCE",
    "last": true,
    "totalPages": 1,
    "totalElements": 1,
    "first": true,
    "number": 0,
    "sort": {
      "sorted": false,
      "unsorted": true,
      "empty": true
    },
    "numberOfElements": 1,
    "size": 1,
    "empty": false
  }
}

What we expect:

{
  "name": "allergies",
  "data": {
    "content": [],
    "pageable": {
      "sort": {
        "sorted": false,
        "unsorted": true,
        "empty": true
      },
      "pageNumber": 0,
      "pageSize": 20,
      "offset": 0,
      "unpaged": false,
      "paged": true
    },
    "last": true,
    "totalElements": 0,
    "totalPages": 0,
    "first": true,
    "sort": {
      "sorted": false,
      "unsorted": true,
      "empty": true
    },
    "number": 0,
    "numberOfElements": 0,
    "size": 20,
    "empty": true
  }
}

@edcohen08 Is this a paging strategy you recognize?

edcohen08 commented 11 months ago

@samamorgan this specific call is the only time I've seen that where pageable is a string rather than a dict

samamorgan commented 11 months ago

The code around pagination is pretty bloated, and we pepper a lot of logic to figure out pagination in request. I think this is due for an overhaul.

We shouldn't have to determine what paginator class to use as we currently do (setting the class on the model). Instead, we should sort out the pagination strategy in a method based on the structure of the response, then assign that iterator to the underlying collection during the request.

@edcohen08 I assume given the age of this ticket that you already have a workaround in place for this issue. How critical is this fix for your work?

edcohen08 commented 11 months ago

It turned out that we actually weren't using that CDT so I never fixed it. What I was working on was just checking whether the pageable was a string and handling it then. That said I definitely agree it's at the point it should be broken out @samamorgan