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

_patient_info CDT pagination breaks the client #55

Open
edcohen08 opened this issue Nov 29, 2022 · 7 comments
Open

_patient_info CDT pagination breaks the client #55

edcohen08 opened this issue Nov 29, 2022 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@edcohen08
Copy link
Contributor

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

@edcohen08 edcohen08 self-assigned this Nov 29, 2022
@samamorgan
Copy link
Collaborator

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

@samamorgan samamorgan added the bug Something isn't working label Sep 14, 2023
@edcohen08
Copy link
Contributor Author

  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
Copy link
Collaborator

samamorgan commented Sep 19, 2023

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
Copy link
Collaborator

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
Copy link
Contributor Author

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

@samamorgan
Copy link
Collaborator

samamorgan commented Sep 20, 2023

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants