Skip to content

Conversation

toddkazakov
Copy link
Contributor

No description provided.

Copy link

github-actions bot commented Jul 21, 2025

PR Preview Action v1.6.2

🚀 View preview at
https://developer.tidepool.org/TidepoolApi/pr-preview/pr-165/

Built to branch gh-pages at 2025-08-20 12:01 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

properties:
type:
description: Test
type: string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider externalizing this to a separate model that can be re-used in the URL paths and here as well.

Since this is basically an enum, consider also adding a regex pattern of allowable characters and/or min/max length.

parameters: []
'/v1/consents/{type}':
parameters:
- schema:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider externalizing this as it's shared by many of the APIs

application/json:
schema:
$ref: ./auth/models/consent/consents.v1.yaml
operationId: GetConentVersions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo :)

description: Returns a list of all consent versions for a given type
'/v1/users/{userId}/consents':
parameters:
- schema:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ref: './common/models/tidepooluserid.yaml'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be something here? Or is this now record.v1.yaml?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was renamed to record.v1.yaml

x-stoplight:
id: iepviv3jm8m9m
type: array
items:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uniqueItems: true?

id: iew30v83r2ce5
type: string
enum:
- ADCES Foundation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the intent here that the UX could use these enums directly as human-readable strings? Do we envision them ever to be localizable?

What should happen if the org changes their displayable name (e.g. JDRF -> Breakthrough T1D), or an org gets removed? This has happened at least 3 times thus far (https://tidepool.atlassian.net/browse/WEB-736, https://tidepool.atlassian.net/browse/WEB-3447, https://tidepool.atlassian.net/browse/WEB-824)

Copy link
Contributor Author

@toddkazakov toddkazakov Jul 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This enum represents all valid values that are accepted and returned in API requests. If we rename an organization, we ought to migrate all records. It's the responsibility of the frontend to maintain a list of all valid organizations for initial selection.

type: object
properties:
id:
type: string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this type is referenced as a URL path component, it'd be good to define as a separate model, re-usable in both places.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

type: object
properties:
data:
type: array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uniqueItems: true?

x-stoplight:
id: c8oq1dn4m4uak
type: object
properties:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this include metadata as well? Otherwise, how will one update their supportedOrganizations?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, then the required properties could be either status or metadata - or maybe we need an explicit API for revocation, and another for updating metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Need to think about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metadata is the only updatable attribute now. Consent can be revoked by sending a DELETE request.

id: 8ctwq6xjg496g
type: object
properties:
supportedOrganizations:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we envision other types of consents having attached metadata?

If so, consider making this specific to TBDDP, and perhaps define a top-level metadata that uses it as oneOf, keyed on type.

description: Test
type: string
version:
type: integer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest checking with Jake on whether we can rely on this version being a simple integer. We may need to accommodate an arbitrary string.

I can see the value of an integer version for sorting, but we could accomplish that using createdTime as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked to Jake - using an integer is ok

schema:
$ref: ./auth/models/consent/consents.v1.yaml
operationId: ListConsents
description: Retrieves the list of all different consent types and their content. By default returns only the latest version for each consent type. This can be override by setting the `version` query parameter to `all`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these returned in any predictable / stable sort order (e.g. version or createdTime)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related question: is the latest by the version or by the createdTime? Is it possible for a lower version to have a later createdTime?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By version. createdTime and modifiedTime are only added for consistency and easier data syncs.

@@ -680,6 +680,159 @@ paths:
- sessionToken: []
tags:
- Internal
/v1/consents:
get:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to be able to restrict the list of consent types that could be accepted by a user? Or can everyone consent to everything? Put another way: does this API require a valid access token?

I could see a potential use-case for restricting this due to kind of an invitation-only or restricted clinical trial. However, this creates a problem with defining who can see which consent types, and how does that work with people who do not yet have a user account. Similar to invitation workflows.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest checking in with Jake and Product team for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The list endpoint is for internal/admin use only. We could make this endpoint public and add restrictions in the future. The frontend should be responsible for fetching a particular consent by type so it can be rendered.

in: query
name: type
post:
summary: Create User Consent Record
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a user grant consent only the latest version of a consent? What happens if they try to grant consent to older version?
Can a user grant consent to something they already consented to (i.e. duplicate record)?

This may need a bit more description of the various workflows - are these correct interpretations of the proposed API?

  1. Granting consent for the first time: CreateUserConsentRecord with latest version
  2. Updating a consent to agree to a new version: CreateUserConsentRecord with a newer latest version
  3. Updating a consent with new metadata: UpdateUserConsentRecord with new metadata only
  4. Updating a consent to revoke it: UpdateUserConsentRecord with status = revoked only

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yes
  2. Yes, but it revokes previous consents for the same type
  3. Need to think some more about the metadata
  4. Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added DELETE /v1/users/{userId}/consents/{recordId} for 4. (revoking a consent record)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if by definition any new version of consent in case #2 is a superset of the original consent? If we revoke the past consent, does that mean we need to purge content they shared while that previous consent was in effect?

IANAL so might be best to check with Jake.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How we interpret revocation dates and whether we purge the data depends on the type of the consent and the actual content. Currently, I believe we state that date shared with BDDP accounts is not purged even after the consent is revoked.

$ref: ./auth/models/consent/consent.v1.yaml
operationId: GetLatestConsentByType
description: Returns the latest consent version for the given type
'/v1/consent/{consentType}/versions':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit pick, but now is the time...

Usually these endpoints use a plural form, like /users even if we're retrieving something for only a single user. Or like the /users/{userid}/consents endpoint that you add later in this doc.

I'm fine either way, I don't think it's a big deal, but if you wanted to maintain more consistency, you might consider the change.

Suggested change
'/v1/consent/{consentType}/versions':
'/v1/consents/{consentType}/versions':

'/v1/users/{userId}/consents/{recordId}':
parameters:
- $ref: '#/components/parameters/userId'
- schema:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this should be better specified as either a UUID or a mongo objectid or... something?

Consider adding at least a max length here? Even just something like 1024.

description: Retrieves a consent record by ID for a given user.
patch:
summary: Update User Consent Record
operationId: UpdateUserConsentRecord
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In most places we use Update<X> with PUT requests, which may or may not obey PUT semantics. Now we're adding PATCH endpoints called Update<X> (which will hopefully only implement PATCH semantics). I'm worried this will get confusing when implementing the handler functions.

Ideally (IMO), PATCH => Update<X> and PUT => Replace<X>, but that's too big of a change for no really good reason.

Is it clearer if we have PATCH => Patch<X> or just something other than Update<X>, so it's clear what the handler's expected semantics are?

Mostly just thinking out loud here, I don't think I feel strongly enough to ask for anything here to be changed. Do you have thoughts?

To summarize, I would like it if I knew that a handler named Update<X> was supposed to behave in a certain way, and if we have PUT Update<X> that does multiple different things already (some obey traditional PUT some don't), and then we add PATCH Update<X> it just all becomes more confusing than it needs to be.

type: string
minLength: 1
readOnly: true
status:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than having a grab bag status field, which can be abused in future changes to the model, consider simply having a "revoked" field that's a timestamp. If the timestamp is a non-zero value, then it has been revoked.

x-stoplight:
id: s4pt6ptt7ez4n
minimum: 1
grantTime:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than repeating Time in each of these fields, consider just naming them granted, revoked, created, and updated. The time is specified in the type of the field, so there's no need to repeat it in the field's name.

Is grantTime useful?

Assuming that revocationTime is non-zero (or the status == granted, but I'd remove the status field, personally), then I can assume it's been granted. Specifically at the createdTime, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Time suffix is consistent with the rest of the platform services. Same goes with createdTime and updatedTime. It makes querying the database a little easier.

I think both grant time and status are useful for downstream consumers because it takes off the responsibility from explaining the implied meaning of the fields of the model. This will be useful to the data science team.

schema:
$ref: ./auth/models/consent/consents.v1.yaml
operationId: GetConsentVersions
description: Returns a list of all consent versions for a given type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this endpoint? Given that version is an integer, it feels safe to assume that if the latest version is 4, then the list of versions is 4, 3, 2, and 1, right?

If the latest version of a consent is 5, can a user consent to version 3? I think the API as it is right now would allow this, but why should the system allow that?

Passing a version with a CreateRecord request makes sense, so we can double check that if they're creating a record for version 4, but version 5 is the newest, we can refuse and return an error. Is there some reason I don't understand for letting someone consent to an older version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This endpoint is only for admin purposes. I believe it will be useful down the road to display all consents versions of a given type e.g. in Orca.

parameters:
- schema:
type: string
enum:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What other values would ever be added to this enum? Would this be better as a boolean flag? That is, ?allVersions=true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was changed to latest=true but that change was not published to GH, apologies.

$ref: ./record.v1.yaml
x-stoplight:
id: u1b4u8fpl1ed6
count:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this useful? Isn't it just a data.length? Do we really need to put this in the response when it's so easy for a client to calculate it itself?

PS- I believe a count appears in the list of consent types too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the length of the data array. Count is the total number of documents matching the query.

id: o02joytb2fxay
type: object
properties:
data:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
data:
consents:

type: object
properties:
id:
type: string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

@toddkazakov toddkazakov requested a review from ewollesen August 20, 2025 11:58
enum:
- <12
- 13-17
- 18+
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the API actually expects "<13", "13-17", ">=18"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

content:
application/json:
schema:
$ref: ./auth/models/consent/consent.v1.yaml
Copy link
Member

@ginnyyadav ginnyyadav Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GET /v1/consents/big_data_donation_project results in this response:

{
"data": [
{
"type": "big_data_donation_project",
"version": 1,
"content": "### Tidepool Big Data Donation Project ....",
"contentType": "markdown",
"createdTime": "2025-09-03T15:46:08.398Z"
}
],
"count": 1
}

which is not a single object as referenced here, but an array of objects. we should return the latest consent object directly I think or switch to the consents model (though the latter makes less sense for what I think this endpoint is intended for)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

- $ref: '#/components/parameters/userId'
get:
summary: Get User Consent Records
tags: []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add Consent tag

- $ref: '#/components/parameters/recordId'
get:
summary: Get User Consent Record
tags: []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add Consent tag

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of the requests also don't have consent tags so they're popped out of the consent folder. I would add those too if you want them all under the "Consent" folder in the API docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

get:
summary: Get Latest Consent By Type
tags:
- Consent
Copy link
Member

@ginnyyadav ginnyyadav Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this and all of these requests below we should add

security:
- sessionToken: []

as we have done in the rest of the docs.

edit: For this particular one /v1/consents ooks like the security there might be a server token but the rest seem to work with a session token

edit 2: Somehow the sessionToken authorization is indeed showing up on redocly. Either sorcery or I'm looking at something outdated? You may be able to disregard this...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's likely a global security setting. Added it explicitly.

content:
application/json:
schema:
$ref: ./auth/models/consent/record.v1.yaml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

   '403':
      description: Forbidden
      content:
        application/json:
          schema:
            $ref: ./common/models/error.v1.yaml
            
            
            ect ect....

description: 'Retrieves the list of all different consent types and their content. '
requestBody:
content: {}
parameters: []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like type is indeed a parameter.

also type and latest together can be used but not latest by itself

also page (integer)

also size (integer)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and type and version

id: r7ypd0pfkbhkv
format: date-time
readOnly: true
updatedTime:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did updatedTime change to modifiedTime?

current response looks like this:

{
"id": "53b6f61168ea8914fe0a83c417e3f033",
"userId": "fbfb1adf-4bba-47a2-850d-285cd23ad644",
"status": "active",
"ageGroup": "<13",
"ownerName": "Mufasa Lion",
"parentGuardianName": "Ahadi Lion",
"grantorType": "parent/guardian",
"type": "big_data_donation_project",
"version": 1,
"metadata": {
"supportedOrganizations": []
},
"grantTime": "2025-09-04T17:58:29.787Z",
"createdTime": "2025-09-04T17:58:29.787Z",
"modifiedTime": "2025-09-04T17:58:29.795Z"
}

there is also a userId being sent back as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modifiedTime is correct and consistent with the rest of the platform models

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants