MS-479 age group selection screen by meladRaouf · Pull Request #749 · Simprints/Android-Simprints-ID

meladRaouf · 2024-06-06T08:35:10Z

Introduced the new age group selection screen and the associated event.
Retrieve the age limits from the fingerprint configuration.
The retrieval of age limits from the face configuration will be implemented in a separate PR.

# Conflicts: # feature/orchestrator/build.gradle.kts # feature/orchestrator/src/main/java/com/simprints/feature/orchestrator/steps/StepId.kt # feature/orchestrator/src/main/java/com/simprints/feature/orchestrator/usecases/steps/BuildStepsUseCase.kt # infra/resources/src/main/res/values/strings.xml # settings.gradle.kts

BurningAXE · 2024-06-06T11:20:27Z

@@ -5,7 +5,7 @@ import com.simprints.infra.config.store.models.TokenKeyType
 import io.mockk.mockk
 import org.junit.Test

-class ApiGuidSelectionPayloadTest {


Hm, is it intentional that you've renamed the ApiGuidSelectionPayloadTest?

BurningAXE · 2024-06-06T12:12:53Z

+    private fun formatAgeInMonthsForDisplay(ageInMonths: Int): String {
+        return when {
+            ageInMonths < 12 -> "$ageInMonths ${getString(IDR.age_group_selection_months)}"
+            ageInMonths < 24 -> "1 ${getString(IDR.age_group_selection_year)}"


Shouldn't it also display the months? Otherwise there would be no difference between 13-23 months and there might be an SDK that works only from say 15 months.

ybourgery · 2024-06-17T06:23:14Z

-     * Dev version >= 2023.4.0 is required for receiving new fingerprint configurations  [CORE-3033]
+     * Dev version >= 2024.2.1 is required for receiving biometric sdk age restrictions
     */
    set("VERSION_NAME", "2024.1.1")


You wrote that the dev version should be >= 2024.2.1, but you set 2024.1.1 you will not get the age configuration

ybourgery · 2024-06-17T06:25:13Z

+fun ProjectConfiguration.allowedAgeRanges(): List<AgeGroup> {
+    return listOf(
+        //Todo add face roc sdk ,
+        fingerprint?.secugenSimMatcher?.allowedAgeRange,
+        fingerprint?.nec?.allowedAgeRange
+    ).mapNotNull { it }
+}


If you look at the API definition the allowedAgeRange field is always present, but it's empty when both start and end are equal to null

ybourgery · 2024-06-17T06:25:57Z

+
+@Keep
+internal data class ApiAgeGroup(
+    val startInclusive: Int,


startInclusive is not mandatory, so it should be a nullable int

ybourgery · 2024-06-17T06:39:00Z

+
+    private fun generateSortedUniqueAgeGroups(ageGroups: List<AgeGroup>): List<AgeGroup> {
+        // Handle empty list case by returning a single age group starting at 0 and ending with null
+        if (ageGroups.isEmpty()) return listOf(AgeGroup(0, null))


This should not happen as you are only displaying the age selection if there are some age groups no?

luhmirin-s

The lack of plurals in the range name creation is the dealbreaker for me.

luhmirin-s · 2024-06-18T09:23:53Z

+            fillRecyclerView(ageGroupsList)
+        }
+
+        viewModel.showExitForm.observe(viewLifecycleOwner) { exitFormConfig ->


Could be using the custom observer instance to avoid checking "ifNotHandled" manually.

I just copied the code from the consent fragment

Then consent fragment could also be improved :)

…d-sdk-selection # Conflicts: # infra/events/src/main/java/com/simprints/infra/events/event/domain/models/EventPayload.kt

sonarqubecloud · 2024-06-20T15:51:25Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
82.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

BurningAXE · 2024-06-24T11:52:34Z

+    @Test
+    fun `should map correctly the allowedAgeRange`() {
+        val mapping = mapOf(
+            ProtoFingerprintConfiguration.VeroGeneration.VERO_1 to FingerprintConfiguration.VeroGeneration.VERO_1,


Looks like this is not testing what it should.

BurningAXE · 2024-06-24T12:25:31Z

Should this test be a new file instead of renaming ApiGuidSelectionPayloadTest?

meladRaouf added 5 commits June 4, 2024 00:22

Add Age group selection screen

ee89069

Add Age group selection event

dab1da7

Merge remote-tracking branch 'origin/main' into age-based-sdk-selection

b2e4dd8

[MS-479] handle selection result

ad312ec

cla-bot Bot added the ... label Jun 6, 2024

meladRaouf force-pushed the MS-479-age-group-selection-screen branch from afe58b0 to 30a4a7e Compare June 6, 2024 08:53

[MS-479] Use the new fingerprint sdk config

f6937bd

meladRaouf force-pushed the MS-479-age-group-selection-screen branch from 30a4a7e to f6937bd Compare June 6, 2024 09:31

BurningAXE reviewed Jun 6, 2024

View reviewed changes

meladRaouf added 2 commits June 6, 2024 18:01

[MS-479] Handle age groups overlapping

2f7ec38

[MS-479] add SelectSubjectAgeGroupViewModelTest

ca12fa8

meladRaouf force-pushed the MS-479-age-group-selection-screen branch from 42baee0 to ca12fa8 Compare June 6, 2024 15:45

meladRaouf added 2 commits June 16, 2024 08:43

[MS-479] add ApiAgeGroup.kt to ApiFingerprintConfiguration

b060479

Merge remote-tracking branch 'origin/main' into age-based-sdk-selection

9bd05c4

meladRaouf changed the title ~~Ms 479 age group selection screen~~ MS-479 age group selection screen Jun 16, 2024

meladRaouf added 2 commits June 16, 2024 09:13

[MS-479] Code reformat

7f13b41

[MS-479] Add tests to SelectSubjectAgeGroupViewModelTest

f34cbc8

meladRaouf marked this pull request as ready for review June 16, 2024 06:59

meladRaouf requested review from a team, BurningAXE, TristramN, alex-vt, alexandr-simprints, luhmirin-s and ybourgery and removed request for a team June 16, 2024 07:00

[MS-479] support years fraction groups

33a6887

ybourgery requested changes Jun 17, 2024

View reviewed changes

[MS-479] retrieve null startInclusive from the api

0c67b09

meladRaouf requested a review from ybourgery June 18, 2024 02:49

ybourgery approved these changes Jun 18, 2024

View reviewed changes

luhmirin-s requested changes Jun 18, 2024

View reviewed changes

[MS-479] Apply code review comments

7bd6974

meladRaouf requested a review from luhmirin-s June 19, 2024 15:01

meladRaouf force-pushed the MS-479-age-group-selection-screen branch 3 times, most recently from 4a428b8 to 481c6aa Compare June 19, 2024 15:24

luhmirin-s reviewed Jun 20, 2024

View reviewed changes

meladRaouf force-pushed the MS-479-age-group-selection-screen branch from 481c6aa to 501deb7 Compare June 20, 2024 09:53

luhmirin-s approved these changes Jun 20, 2024

View reviewed changes

Merge remote-tracking branch 'refs/remotes/origin/main' into age-base…

266b985

…d-sdk-selection # Conflicts: # infra/events/src/main/java/com/simprints/infra/events/event/domain/models/EventPayload.kt

meladRaouf force-pushed the MS-479-age-group-selection-screen branch from 501deb7 to 266b985 Compare June 20, 2024 15:37

meladRaouf merged commit dad1bab into main Jun 23, 2024

meladRaouf deleted the MS-479-age-group-selection-screen branch June 23, 2024 08:45

BurningAXE reviewed Jun 24, 2024

View reviewed changes

Conversation

meladRaouf commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

luhmirin-s left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud Bot commented Jun 20, 2024

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

meladRaouf commented Jun 6, 2024 •

edited

Loading