488 include deviceregistration in devicedeploymentstatus

[yuanchen233]

DeviceRegistration is added to DeviceDeploymentStatus, which is part of StudyDeploymentStatus. A new interface HasDeviceRegistration is introduced to keep this field since Unregistered would not need this field.

A regression test case was added in a separate commit as I'm not sure if it is desired. The test case amis to test GetStudyDeploymentStatusList endpoint with registered devices in one of the deployments. This is not covered since DeploymentServiceTest tests minimum usage for GetStudyDeploymentStatusList, as it is just a list representation of GetDeploymentStatus. I added it since json migration for this endpoint works differently.

[Whathecode]

Smaller comments I fixed in a fixup commit.

Some bigger observations:

• I would have expected a migration to be set up for RecruitmentService in the studies subsystem as well, which returns ParticipantGroupStatus, which contains StudyDeploymentStatus once "invited". It's odd there are no failing tests on this. 🤔
• Extra test cases seem relevant (also see my comment on a bug which wasn't caught due to lacking coverage of DeploymentService). But, if you add them for past versions (p.s. you added it to 1.0 and not 1.1, was that intentional?), make sure they were generated with code for that version. You may have to branch out from an old release, write the test there, and then possibly do a cherry-pick on this branch.

[Whathecode]

This should be DeviceDeployed. 🤔 Why does it work? I'm not a big TDD fan, but there is something to seeing a test fail before making it pass.

Seems like luck would have it that's exactly where coverage is missing. 😅 Could you add a preceding commit which provides coverage for DeploymentService.deviceDeployed()? And, while at it, verify that all the other endpoints of this service are actually called in the current regression tests.

[yuanchen233]

We only have coverage for CreateStudyDeployment, if I remember correctly that was the only failing test, as Actual testing of the status responses should already be covered adequately in StudyDeployment tests., but the regression test case are generated from DeploymentServiceTest.

Extra test cases seem relevant

Do we only want the generated json request files, or we also want those tests in DeploymentServiceTest? I think I'll include them first anyways to agree on the test cases, and they can be removed later if not desired.

I added it to 1.0 because I felt the test case was needed from the start of this major version, not due to some changes introduced in 1.1. Then simlly forget to add them 1.1 😛. As I generated those json files in ver. 1.0 and see a list of diff with origional ones, wandering what I breaked and realized those are the generated UUIDs difference...

[Whathecode]

Do we only want the generated json request files, or we also want those tests in DeploymentServiceTest?

I'd always keep them in sync, so that it's just a matter of copying the output of all of them over.

But, obviously the comment I left in code there didn't consider the implications on reduced regression testing of the infrastructure layer (JSON) by not covering all potential input/output. 😅 So albeit valid—there is plenty of functional coverage in StudDeploymentTest—, that doesn't cover serialization and JSON schema definitions.

I think it would make sense to write test cases that try to cover as much as possible, rather than writing them as unit tests. These are more or less integration tests after all. So, I'd focus on longer scenarios with multiple calls reflecting fuller study lifetime to get more coverage for the infrastructure layer.

As I generated those json files in ver. 1.0 and see a list of diff with origional ones, wandering what I breaked and realized those are the generated UUIDs difference...

I actually spotted that. 🤔. I thought I injected some more deterministic GUID and timestamp generator for the creation of these tests. I saw it in some test resources, but not others. I don't remember whether that's expected/normal, or oversight.