MS-448 Ad-hoc tokenisation improvements#1520
Conversation
| realmToRoomMigrationScheduler.scheduleMigrationWorkerIfNeeded() | ||
|
|
||
| // Running tokenization after potential reset and room migration to avoid unnecessary work | ||
| enrolmentRecordRepository.tokenizeExistingRecords(project) |
There was a problem hiding this comment.
Can we also have a flag in the receiver so this doesn't unnecessarily run in 99.9% of cases?
There was a problem hiding this comment.
Passing the flag around is too much hustle with little ROI. I have added a check to only run the ad-hoc tokenisation when there were no keys before refresh. This should prevent extra queries at a cost of a single if. :)
| try { | ||
| val query = EnrolmentRecordQuery(projectId = project.id, hasUntokenizedFields = true) | ||
| val tokenizedSubjectsCreateAction = selectEnrolmentRecordLocalDataSource() | ||
| val tokenizedRecordsCreateAction = selectEnrolmentRecordLocalDataSource() |
There was a problem hiding this comment.
Shouldn't this be untokenizedRecordsCreateAction?
There was a problem hiding this comment.
By the end of the call chain the final value is indeed tokenized.
luhmirin-s
force-pushed
the
feature/MS-448-tokenisation-improvements
branch
from
b289c83 to
bf1afa9
Compare
|
@BurningAXE I have reworked the approach after yesterday's conversation. This is an intervention on a similar scale to my previous approach, but it happens much sooner in the execution flow, so it should cover your concerns. |
luhmirin-s
requested review from
a team,
BurningAXE,
TristramN,
alex-vt,
alexandr-simprints,
meladRaouf and
ybourgery
and removed request for
a team
| import com.simprints.infra.orchestration.data.ActionRequest | ||
| import javax.inject.Inject | ||
|
|
||
| class EnsureActionFieldsTokenisedUseCase @Inject constructor( |
There was a problem hiding this comment.
nitpick: we use tokenized in other places so I suggest we keep it consistent.
There was a problem hiding this comment.
Good catch, I am always leaning towards "s" and have to continuously correct myself.
| oldProject: Project?, | ||
| newProject: Project, | ||
| ) { | ||
| if (oldProject?.tokenizationKeys?.isEmpty() == true && newProject.tokenizationKeys.isNotEmpty()) { |
There was a problem hiding this comment.
Shouldn't this trigger also if oldProject?.tokenizationKeys are null?
There was a problem hiding this comment.
Hmm, technically, keys cannot be null, only the whole "oldProject". I will add an explicit check for that just in case.
| import com.simprints.infra.enrolment.records.repository.EnrolmentRecordRepository | ||
| import javax.inject.Inject | ||
|
|
||
| class TokenizeRecordsIfProjectChangedUseCase @Inject constructor( |
There was a problem hiding this comment.
nitpick: I found this name misleading - thought it meant one project changed to another. Maybe TokenizeRecordsIfKeysChangedUseCase?
| ProjectState.PROJECT_PAUSED -> _showAlert.send(LoginCheckError.PROJECT_PAUSED) | ||
| ProjectState.PROJECT_ENDED -> startSignInAttempt(actionRequest) | ||
| ProjectState.RUNNING -> proceedWithAction(actionRequest) | ||
| ProjectState.RUNNING -> proceedWithAction(ensureActionFieldsTokenisedUseCase(actionRequest)) |
luhmirin-s
force-pushed
the
feature/MS-448-tokenisation-improvements
branch
from
bf1afa9 to
d2cd3e7
Compare
|
2 participants
JIRA ticket
Will be released in: 2026.1.0
Notable changes
ActionRequestare not tokenised), we can do a double-check right before returning the action fromLoginCheckViewModeland provide the updated value to the orchestrator. This means that values in the action request are processed untokenised at most during the intent parsing and login flow.Testing guidance
Additional work checklist