Skip to content

[MS-887] Fix Filtering Logic in CommCareIdentityDataSource #1113

Merged
meladRaouf merged 1 commit into
release/2025.1.0from
bugfix/MS-887-commcare-id
Feb 24, 2025
Merged

[MS-887] Fix Filtering Logic in CommCareIdentityDataSource #1113
meladRaouf merged 1 commit into
release/2025.1.0from
bugfix/MS-887-commcare-id

Conversation

@meladRaouf
Copy link
Copy Markdown
Collaborator

@meladRaouf meladRaouf commented Feb 24, 2025

There was a bug in the identification callouts from CommCare due to incorrect filtering logic in CommCareIdentityDataSource. The filtering logic was unintentionally excluding all records, causing identification failures.

The issue arose when building the subjectQuery in BuildMatcherSubjectQueryUseCase, where the filtering logic failed under different partitionType conditions:

partitionType = PROJECT → Both module ID and attendant ID are null.
partitionType = USER → The module ID in the query is null.
partitionType = MODULE → The attendant ID is null in the query.

Due to this, the filtering logic always failed, as it did not account for null values correctly.

I updated the filtering logic to correctly handle null values to ensure that valid records are no longer filtered out incorrectly.

project: Project,
): Boolean = when {
s1 == null -> false
s1 == null -> true // ignore null values as they are not tokenized
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.

Doesn't this break the usual comparison/equals contract in kotlin? Usually, when nulls are involved, comparisons return false, as there is nothing to compare.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, you are right, I reverted it back to false.

TokenKeyType.ModuleId,
project,
)
isSubjectIdMatching(query, event) &&
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 is very counter-intuitive since it reads as "filter items that match by all 3 fields".
I get the logic behind it, but the naming should be clearer, maybe at least something like "isNullOrMatching".

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

@meladRaouf meladRaouf force-pushed the bugfix/MS-887-commcare-id branch 3 times, most recently from 599a179 to 1796fb5 Compare February 24, 2025 08:37
}.orEmpty()
}

fun isSubjectIdNullOrMatching(
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.

Do the functions need to be public?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

…eationEvent` to resolve issues when either the module ID, attendant ID, or both are null.
@meladRaouf meladRaouf force-pushed the bugfix/MS-887-commcare-id branch from 1796fb5 to 6f77dba Compare February 24, 2025 09:03
@sonarqubecloud
Copy link
Copy Markdown

)
val templateFormat = "ROC_1_23"
val query = SubjectQuery(faceSampleFormat = templateFormat)
val query = SubjectQuery(
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.

Are the tests failing with the old implementation now?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

No Tests were OK.
I just added more parameters to one of the queries so that the tests covers all the flows in the filtering method

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.

My point is if they didn't catch the bug maybe they weren't :D

@meladRaouf meladRaouf merged commit a8fd21e into release/2025.1.0 Feb 24, 2025
@meladRaouf meladRaouf deleted the bugfix/MS-887-commcare-id branch February 24, 2025 10:09
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