Skip to content

SDK selection based on subject's age#774

Merged
BurningAXE merged 11 commits into
mainfrom
MS-464-Enrolling-an-entirely-new-patient
Jul 9, 2024
Merged

SDK selection based on subject's age#774
BurningAXE merged 11 commits into
mainfrom
MS-464-Enrolling-an-entirely-new-patient

Conversation

@BurningAXE
Copy link
Copy Markdown
Contributor

@BurningAXE BurningAXE commented Jun 26, 2024

These are the core changes to allow dynamic SDK selection based on age restriction of the SDKs and age of the subject.

Notable parts:

  • Use the new project configuration which includes allowedAgeRange for each SDK
  • Parse subjectAge from Intent metadata (if present)
  • Update StepBuilder to add capture and match steps for appropriate SDKs if they are age restricted and subjectAge is present
  • Update StepBuilder to not add capture and match steps for appropriate SDKs if they are age restricted and subjectAge is not present
  • Update StepBuilder to add age group selection step if project is age restricted but subjectAge is not present
  • Update Orchestrator to add appropriate capture and match steps after age group selection step
  • Send 'age group not supported' error if no SDK supports the subject's age
  • Update all components that relied on bioSdkConfiguration

Tried to keep commits thematic so the PR can be reviewed commit by commit if you want to follow the logic.

@cla-bot cla-bot Bot added the ... label Jun 26, 2024
@BurningAXE BurningAXE force-pushed the MS-464-Enrolling-an-entirely-new-patient branch 2 times, most recently from 831408b to e6bf57b Compare June 27, 2024 10:00
@BurningAXE BurningAXE self-assigned this Jun 27, 2024
@BurningAXE BurningAXE force-pushed the MS-464-Enrolling-an-entirely-new-patient branch 6 times, most recently from a73a4ff to ae316e8 Compare July 1, 2024 16:31
@BurningAXE BurningAXE force-pushed the MS-464-Enrolling-an-entirely-new-patient branch from ae316e8 to 3e87009 Compare July 1, 2024 17:43
@BurningAXE BurningAXE changed the title Ms 464 enrolling an entirely new patient SDK selection based on subject's age Jul 1, 2024
@BurningAXE BurningAXE marked this pull request as ready for review July 1, 2024 18:13
@BurningAXE BurningAXE requested review from a team, TristramN, alex-vt, alexandr-simprints, luhmirin-s, meladRaouf and ybourgery and removed request for a team July 1, 2024 18:13
@BurningAXE BurningAXE force-pushed the MS-464-Enrolling-an-entirely-new-patient branch from 3e87009 to 57a2d7b Compare July 2, 2024 13:12
)
val fingerSelections = mutableListOf<FingerSelectionSection>()
configManager.getProjectConfiguration().fingerprint?.secugenSimMatcher?.fingersToCapture?.let {
fingerSelections.add(FingerSelectionSection("SimMatcher", it.toFingerSelectionItems()))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a reasonable way to avoid hardcoding names?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We don't have anything like a name parameter for SDK config. I can put the names in the resources so they can be reused but this is the first time they appear in UI.

Comment thread feature/dashboard/src/main/res/layout/header_sdk_name.xml Outdated
projectConfiguration: ProjectConfiguration,
ageGroup: AgeGroup? = null,
): List<Step> {
val resolvedAgeGroup = ageGroup ?: ageGroupFromSubjectAge(action, projectConfiguration)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This line seems to be identical for all flows, could it be moved up the call stack and pre-resolved age be passed in?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The problem is that all 3 functions with repeated code can be called from 2 places (which is why they were extracted in the first place). Was thinking of extracting this part in a helper function but IMO it would hardly improve things - less repetition but more lines of code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That said - if you can think of a good solution - it's more than welcome!

@BurningAXE BurningAXE force-pushed the MS-464-Enrolling-an-entirely-new-patient branch from 57a2d7b to 35c6bac Compare July 3, 2024 14:51
Copy link
Copy Markdown
Collaborator

@ybourgery ybourgery left a comment

Choose a reason for hiding this comment

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

Something that I don't think was discussed in the design document, but with the current implementation the field modalities in the Session is based on the configuration. I think with this work it should be based on the configuration and the actual modalities that we are capturing based on the age.

@BurningAXE
Copy link
Copy Markdown
Contributor Author

Something that I don't think was discussed in the design document, but with the current implementation the field modalities in the Session is based on the configuration. I think with this work it should be based on the configuration and the actual modalities that we are capturing based on the age.

Hm, what is this field used for in the backend?

@ybourgery
Copy link
Copy Markdown
Collaborator

Something that I don't think was discussed in the design document, but with the current implementation the field modalities in the Session is based on the configuration. I think with this work it should be based on the configuration and the actual modalities that we are capturing based on the age.

Hm, what is this field used for in the backend?

We use this to know what are the different modalities that are present in the sessions to compute the different metrics/models in Clio

@BurningAXE BurningAXE requested a review from ybourgery July 4, 2024 16:59
@BurningAXE BurningAXE force-pushed the MS-464-Enrolling-an-entirely-new-patient branch from 39cbdc5 to cb51d22 Compare July 4, 2024 17:14
@BurningAXE BurningAXE force-pushed the MS-464-Enrolling-an-entirely-new-patient branch from cb51d22 to 8d3ac7d Compare July 4, 2024 17:21
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jul 4, 2024

@BurningAXE BurningAXE merged commit 41dcbfa into main Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants