hypothesis / lms

LTI app for integrating with learning management systems
BSD 2-Clause "Simplified" License
45 stars 14 forks source link

Add new API endpoint to list students #6409

Closed marcospri closed 1 week ago

marcospri commented 1 week ago

For: https://github.com/hypothesis/lms/issues/6393

Example response

{
  "students": [
    {
      "id": "2a8a99bc095a8438a594cd57c1f4f0341951e360",
      "display_name": "Hypothesis 101 Student"
    }
  ],
  "pagination": {
    "next": "http://localhost:8001/api/dashboard/students?limit=1&cursor=%5B%22Hypothesis+101+Student%22%2C+71%5D"
  }
}

Testing

robertknight commented 1 week ago

Running this query locally I get:

{
  "students": [
    {
      "id": "2a8a99bc095a8438a594cd57c1f4f0341951e360",
      "display_name": "Hypothesis 101 Student"
    },
    {
      "id": "748fc7f5-7ad7-4f7a-8cbf-49b5bc19dfec_355",
      "display_name": null
    },
    {
      "id": "6",
      "display_name": null
    },
    {
      "id": "075f3bec5d684e59acb882a12b9bed2c",
      "display_name": null
    },
    {
      "id": "6",
      "display_name": null
    }
  ],
  "pagination": {
    "next": null
  }
}

These are LTI user IDs, right? They aren't unique across LMSes, so we can't use them in API calls where we need this to be the case.

marcospri commented 1 week ago

These are LTI user IDs, right? They aren't unique across LMSes, so we can't use them in API calls where we need this to be the case.

Yes, we still don't use these but it makes sense to think ahead and expose IDs we can actually use :+1:

I went from exposing both the id of the user in the LMS (maybe useful to display it at some point?) and the h_userid which is a better ID than the primary key in "User" as it shared in H and LMS.

robertknight commented 1 week ago

I went from exposing both the id of the user in the LMS (maybe useful to display it at some point?)

It might be useful for debugging, although in the context of an assignment launch you have both the H user ID and the LMS user ID.

robertknight commented 1 week ago

When accessing http://localhost:8001/api/dashboard/students?limit=3 I get:

{
  "students": [
    {
      "h_userid": "acct:f6c3ed0edd8a4013bfc8d0c22b9eca@lms.hypothes.is",
      "lms_id": "2a8a99bc095a8438a594cd57c1f4f0341951e360",
      "display_name": "Hypothesis 101 Student"
    },
    {
      "h_userid": "acct:fc0c4acac9dfab3f4b3fb29b0d4bdd@lms.hypothes.is",
      "lms_id": "748fc7f5-7ad7-4f7a-8cbf-49b5bc19dfec_355",
      "display_name": null
    },
    {
      "h_userid": "acct:b327e72eafa04519c72dc5ea0835bd@lms.hypothes.is",
      "lms_id": "6",
      "display_name": null
    }
  ],
  "pagination": {
    "next": "http://localhost:8001/api/dashboard/students?limit=3&cursor=%5Bnull%2C+14%5D"
  }
}

Fetching the next link returns:

{
  "message": "Unable to process the contained instructions",
  "details": {
    "query": {
      "_schema": [
        "Invalid value for pagination cursor. Cursor must be a [str,int] list."
      ]
    }
  }
}
marcospri commented 1 week ago

I encountered an issue with the next link when testing this - see my most recent comment. If the h user ID is the unique key for these items, I presume the cursor should use that?

Not necessarily, the cursor has to convey order and we want to use name for that in the dropdowns. I didn't account for names being empty, I'll check that now.

robertknight commented 1 week ago

Not necessarily, the cursor has to convey order and we want to use name for that in the dropdowns.

The cursor needs to tell you unambiguously where in the list to start from. Using a person's name is problematic as it is possible, in principle, that multiple users have the same displayed name. This is hopefully not too common within a single class, as it would make navigating in the UI confusing, but it shouldn't break the system.

marcospri commented 1 week ago

Not necessarily, the cursor has to convey order and we want to use name for that in the dropdowns.

We already use name and ID... The problem is the validation not checking for nullable values.

robertknight commented 1 week ago

We already use name and ID... The problem is the validation not checking for nullable values.

Should the ID be the h user ID? If I remove the limit parameter I get back results where there are multiple entities with the same lms_id but different h_userid fields. Or is the issue that we shouldn't ever return multiple items with the same lms_id?

{
  "students": [
    {
      "h_userid": "acct:f6c3ed0edd8a4013bfc8d0c22b9eca@lms.hypothes.is",
      "lms_id": "2a8a99bc095a8438a594cd57c1f4f0341951e360",
      "display_name": "Hypothesis 101 Student"
    },
    {
      "h_userid": "acct:fc0c4acac9dfab3f4b3fb29b0d4bdd@lms.hypothes.is",
      "lms_id": "748fc7f5-7ad7-4f7a-8cbf-49b5bc19dfec_355",
      "display_name": null
    },
    {
      "h_userid": "acct:b327e72eafa04519c72dc5ea0835bd@lms.hypothes.is",
      "lms_id": "6",
      "display_name": null
    },
    {
      "h_userid": "acct:a818dcfcacdee12dacfbf4b607be81@lms.hypothes.is",
      "lms_id": "075f3bec5d684e59acb882a12b9bed2c",
      "display_name": null
    },
    {
      "h_userid": "acct:525d674dcde8cf25a32713290db13b@lms.hypothes.is",
      "lms_id": "6",
      "display_name": null
    }
  ],
  "pagination": {
    "next": null
  }
}
marcospri commented 1 week ago

@robertknight made some changes here to fix the pagination for None values in names. The same problem would happen in assignments.

Should the ID be the h user ID? [the second value of the cursor]

It could but right now it's the table's PK. That's generally a sorter value and works across all models (as in course, assignments and user do have the same Table.id column).

robertknight commented 1 week ago

Gah. Please ignore my comment about the ID. I mistook it for being the LTI user ID because some of the LTI user IDs I see are integers. The revised version works as expected.