cph-cachet / carp.core-kotlin

Infrastructure-agnostic framework for distributed data collection.
https://carp.cachet.dk/core/
MIT License
21 stars 3 forks source link

Should `DeviceRegistration` include freeform specification data? #431

Closed Whathecode closed 1 month ago

Whathecode commented 1 year ago

There is a current request to include the following information for smartphone registrations:

        val platform: String? = null,
        val hardware: String? = null,
        val deviceName: String? = null,
        val deviceManufacturer: String? = null,
        val deviceModel: String? = null,
        val operatingSystem: String? = null,
        val sdk: String? = null,
        val release: String? = null,

But, as stated previously, such fields weren't added yet to the corresponding registration of Smartphone since the use cases are unclear:

So far, it hasn't been included as separate more formalized fields since I did not make time to determine what these should be or where they should be defined. Are these additional fields on the abstract DeviceRegistration or on extending classes? E.g., for smartphones we want to know the device model and OS. But, is "device model" generic enough to be applied to the base class, and does it add value over the current deviceDisplayName?

Further questions I raised on Slack:

As is, the current underlying requirement seems "I want to log any device specifications", and this doesn't seem particularly related to something CARP-specific. If there are no CARP use cases acting on this data, I so no real need to define it statically (which also requires supporting backwards compatibility, etc...). To give an example, "I want to configure CARP deployments to only accept iOS phones", would be a CARP-specific requirement warranting such a static field.

Regardless of whether or not adding some of this data at some point in the future as static fields makes sense, I believe "logging any device specification" is a fair requirement. This is similar to how we already allow appending any extra information to StudyInvitation. By using ApplicationDataSerializer on a String? field, this also supports full JSON objects to be assigned to this field by the application.

I have the impression that adding an additionalSpecifications: String? field to DeviceRegistration would satisfy the use case of the current PR. @bardram? To such a field, you can add your own JSON string, and it won't be escaped during serialization, making it trivial to append any rich key/value data.

bardram commented 1 year ago

Here is a link to the Flutter Device Info package which has a lot of inspiration for cross-platform device in formation.

bardram commented 1 year ago

The use case(s) I expects are the following - we should be able to know:

Useful overviews are:

I agree that;

Whathecode commented 1 year ago

we should be able to know ...

I agree this is relevant info, but as it stands now, I understand "we" is the researcher. Any of the solutions suggested above work, from a single additionalSpecifications, to a platform String, to an operatingSystem String which includes the platform, to a strongly typed Platform (which can still include Other for forward compatibility) and OS Version.

The question is which use case you would need the gradually stronger contracts for.

bardram commented 1 year ago

The question is which use case you would need the gradually stronger contracts for.

I think it's important to know the type of the device - I need to know that it's a smartphone, or an ECG device, etc. This is useful e.g. for showing the right icon in the web portal. More detailed information on the tech specs of the device might also be useful in a scenario where a study would not be able to run on an old Android phone, for example. I don't know what the normal way to model this in carp core is - would it be having a type attribute, by having a class hierarchy (SmartphoneRegistration, ECGDeviceRegistration), or an enum with possible options (which can later be extended as new devices are know and used)?

Any of the solutions suggested above work, from a single additionalSpecifications, ...

Yes - we should definitely add this.

bardram commented 1 year ago

Another question - how do I get the DeviceRegistration back to show it in the client (smartphone or web portal)?

When looking at StudyDeploymentStatus.kt is has a list of DeviceDeploymentStatus. But this does not seems to contain the device registration -- so I don't know anything about the registred devices.

Whathecode commented 1 year ago

I think it's important to know the type of the device - I need to know that it's a smartphone, or an ECG device, etc.

This, you already know due to the type of DeviceConfiguration. Not only is this useful to determine icons, etc., it should be what determines the device/data collector runtime. A distinct implementation for each type of DeviceConfiguration is expected, which is one of the reasons to model them as such. If there are too many conditional execution paths depending on internal information contained therein, it may be better to model things as two separate concrete DeviceConfigurations.

More detailed information on the tech specs of the device might also be useful in a scenario where a study would not be able to run on an old Android phone, for example.

Agreed, This was part of the reason for registering devices in a deployment, and separating out a configuration from a registration. As you can see in the implementation, the backend already calls a validity check on the registration.

Thus, for concrete types of DeviceConfiguration (e.g., Smartphone), the backend will be able to execute such minimum requirement checks. It will make perfect sense for these to operate on strongly typed fields, which would be a reason for adding them to a custom DeviceConfiguration.

But, adding these fields now without including that functionality in CARP is premature. This is what I hinted at before:

If there are no CARP use cases acting on this data, I so no real need to define it statically (which also requires supporting backwards compatibility, etc...). To give an example, "I want to configure CARP deployments to only accept iOS phones", would be a CARP-specific requirement warranting such a static field.

If the current requirement is simply to be able to see the specifications as a researcher, I think I will do a separate PR implementing the additionalSpecifications first, since this is not only useful to Smartphone, but any DeviceConfiguration, and overall a fairly clear requirement. As far as I can tell, this would also already cover your current use cases.

how do I get the DeviceRegistration back to show it in the client (smartphone or web portal)?

That seems to be an omission. 😮 You can access them through PrimaryDeviceDeployment returned by getDeviceDeploymentFor, but it is unclear why I would have decided against also including registration inside the status. 🤔 I seem to vaguely recall this may be related to privacy, since only people responsible for deploying a device (being invited to do so) should have access to what may be PII. 🤔 But, I need to look at this in detail again.

Whathecode commented 8 months ago

I seem to vaguely recall this may be related to privacy, since only people responsible for deploying a device (being invited to do so) should have access to what may be PII. 🤔 But, I need to look at this in detail again.

@bardram I wrote some detailed thoughts on this in a separate issue. I think preventing PII data from living in the deployments subsystem, or not exposing it to everyone, was the primary driving factor. However, upon reflecting on this further this can't really be avoided. As is, it is fair to assume that anyone who has access to a deployment, will also have access to the study of that deployment.

In terms of this issue, I think the solution is thus simply to include DeviceRegistration information in DeviceDeploymentStatus (which is contained within StudyDeploymentStatus). Either way, with the current recommended claim-based access documentation, the researcher who set up the deployment would have access to device registrations by calling getDeviceDeploymentFor() (which returns PrimaryDeviceDeployment and contains the registrations), since they have access to all deployment endpoints.

I removed the "needs discussion" tag, as I think there are no further questions on what is needed here.

Whathecode commented 1 month ago

The main request, an additionalSpecifications field, was implemented. I created a separate issue for the request to expose DeviceRegistration in study status.

Therefore, I'm closing this as completed.