RCF-1354: Implemented the Maximum number of days without running the sync and added GPS location validation #664
Conversation
…sync job and geo-location validation Signed-off-by: sachin.sp <sachin.sp@cyberpwn.com>
WalkthroughAdds a SyncStatusValidatorService (interface + implementation), integrates it into RegistrationService via DI, enforces pre-registration sync-timing and center-to-machine distance checks, updates AppModule and constants, adds unit tests, extends localized error messages, and adds an error string resource. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client App
participant RegService as RegistrationServiceImpl
participant SyncValidator as SyncStatusValidatorService
participant SyncJobRepo as SyncJobDefRepository
participant GlobalParam as GlobalParamRepository
participant LocationSvc as LocationValidationService
Client->>RegService: startRegistration()
activate RegService
RegService->>RegService: doPreChecksBeforeRegistration()
RegService->>SyncValidator: validateSyncStatus()
activate SyncValidator
SyncValidator->>SyncJobRepo: getActiveSyncJobs()
SyncJobRepo-->>SyncValidator: active jobs
loop per job
SyncValidator->>GlobalParam: get configured frequency
GlobalParam-->>SyncValidator: frequency value
SyncValidator->>SyncJobRepo: get last sync / history
SyncJobRepo-->>SyncValidator: last sync time
end
alt overdue detected
SyncValidator-->>RegService: throw ClientCheckedException
RegService-->>Client: return time-sync error
else passed
SyncValidator-->>RegService: validation passed
end
deactivate SyncValidator
alt GPS location available
RegService->>SyncValidator: validateCenterToMachineDistance(lat,long)
SyncValidator->>LocationSvc: compute distance / get center coords
LocationSvc-->>SyncValidator: distance
SyncValidator->>GlobalParam: get max allowed distance
GlobalParam-->>SyncValidator: threshold
alt distance > threshold
SyncValidator-->>RegService: throw ClientCheckedException
RegService-->>Client: return location error
else
SyncValidator-->>RegService: distance OK
end
end
RegService->>Client: proceed or error
deactivate RegService
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java:
- Around line 305-308: Replace the hardcoded English message in the
ClientCheckedException throw inside SyncStatusValidatorServiceImpl with a string
resource: add a new entry named err_outside_registration_center in strings.xml
containing the provided message, then call the existing throw new
ClientCheckedException(RegistrationConstants.OPT_TO_REG_OUTSIDE_LOCATION,
context.getString(R.string.err_outside_registration_center)) (or equivalent
using the available Context/Resources) so the exception uses
R.string.err_outside_registration_center instead of the literal string.
- Around line 223-231: getActualDays has an off-by-one: it adds +1 so a job
synced today yields 1, which with the existing comparison configuredFrequency <=
actualDays (used elsewhere) can mark non-overdue jobs as overdue; remove the +1
from getActualDays so it returns the true elapsed days ((int) daysDifference),
then run/adjust any tests or logic that rely on the old behavior (or
alternatively change the comparison to configuredFrequency < actualDays if you
prefer keeping the +1, but the simplest fix is to remove the +1 in
getActualDays).
In
@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/spi/SyncStatusValidatorService.java:
- Around line 17-24: Update the Javadoc for
SyncStatusValidatorService.validateCenterToMachineDistance to document null
parameter behavior: state that machineLongitude and machineLatitude will skip
validation if null (e.g., "@param machineLongitude Machine longitude (validation
skipped if null)" and "@param machineLatitude Machine latitude (validation
skipped if null)"), ensuring the comment reflects the intended behavior of
skipping validation rather than throwing an exception.
🧹 Nitpick comments (7)
assets/l10n/app_en.arb (1)
305-305: Minor spelling inconsistency in error message.The new error tokens
OPT_TO_REG_TIME_SYNC_EXCEEDandOPT_TO_REG_OUTSIDE_LOCATIONare clear and actionable. However, there's a minor spelling inconsistency in the second message: it uses both "center" and "centre" (British spelling). Consider using consistent spelling throughout.✏️ Suggested consistency fix
-"errors": "{messages, select, REG_TRY_AGAIN{Login Failed. Try Again!} REG_INVALID_REQUEST{Password incorrect!} REG_MACHINE_NOT_FOUND{This device has not been onboarded yet. Please reach out to your administrator for assistance!} REG_NETWORK_ERROR{Login failed. Check network connection!} REG_CRED_EXPIRED{Credentials not found or are expired. Please try online login!} REG_MACHINE_INACTIVE{Machine is not active!} REG_CENTER_INACTIVE{Center is not active!} REG_LOGIN_LOCKED{Maximum login attempt limit reached. Try again later!} KER_SYN_AUTH_001{Unable to get Authentication Token!} PAK_APPRVL_MAX_TIME{Action required: Pending registered packets have exceeded the allowed approval time. Please clear the backlog to continue.} PAK_UPLOAD_MAX_TIME{Action required: Upload or export the packets to the server before proceeding with registration.} PAK_UPLOAD_MAX_COUNT{Action required: Packet limit reached. Please export or upload existing packets before creating new registrations.} OPT_TO_REG_TIME_SYNC_EXCEED{Time since last sync exceeded maximum limit. Please sync from server before proceeding with this registration.} OPT_TO_REG_OUTSIDE_LOCATION{Your client machine location is outside the registration center. Please note that registration can be done only from within the registration centre} other{'Some error occurred!'}}", +"errors": "{messages, select, REG_TRY_AGAIN{Login Failed. Try Again!} REG_INVALID_REQUEST{Password incorrect!} REG_MACHINE_NOT_FOUND{This device has not been onboarded yet. Please reach out to your administrator for assistance!} REG_NETWORK_ERROR{Login failed. Check network connection!} REG_CRED_EXPIRED{Credentials not found or are expired. Please try online login!} REG_MACHINE_INACTIVE{Machine is not active!} REG_CENTER_INACTIVE{Center is not active!} REG_LOGIN_LOCKED{Maximum login attempt limit reached. Try again later!} KER_SYN_AUTH_001{Unable to get Authentication Token!} PAK_APPRVL_MAX_TIME{Action required: Pending registered packets have exceeded the allowed approval time. Please clear the backlog to continue.} PAK_UPLOAD_MAX_TIME{Action required: Upload or export the packets to the server before proceeding with registration.} PAK_UPLOAD_MAX_COUNT{Action required: Packet limit reached. Please export or upload existing packets before creating new registrations.} OPT_TO_REG_TIME_SYNC_EXCEED{Time since last sync exceeded maximum limit. Please sync from server before proceeding with this registration.} OPT_TO_REG_OUTSIDE_LOCATION{Your client machine location is outside the registration center. Please note that registration can be done only from within the registration center} other{'Some error occurred!'}}",android/clientmanager/src/main/java/io/mosip/registration/clientmanager/spi/SyncStatusValidatorService.java (1)
8-15: Consider narrowing exception types for better error handling.Both methods declare
throws Exception, which is overly broad and makes it difficult for callers to handle specific validation failures distinctly. Consider introducing specific exception types (e.g.,SyncStatusException,LocationValidationException) or using existing domain-specific exceptions from the codebase.💡 Refactoring suggestion
public interface SyncStatusValidatorService { /** * Validates sync job frequencies. * - * @throws Exception if validation fails + * @throws ClientCheckedException if validation fails */ - void validateSyncStatus() throws Exception; + void validateSyncStatus() throws ClientCheckedException;android/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/RegistrationConstants.java (1)
3-7: Duplicate import detected.
java.util.ArrayListis imported twice on lines 4 and 5. Also,java.sql.Arrayon line 3 appears unused in this file.🧹 Proposed fix
package io.mosip.registration.clientmanager.constant; -import java.sql.Array; -import java.util.ArrayList; -import java.util.ArrayList; import java.util.Arrays; import java.util.List;android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImplTest.java (3)
86-93: Redundant mock initialization.When using
@RunWith(MockitoJUnitRunner.class), mocks annotated with@Mockand@InjectMocksare automatically initialized. The explicitMockitoAnnotations.openMocks(this)call on line 88 is redundant and can be removed.🧹 Proposed fix
@Before public void setUp() { - MockitoAnnotations.openMocks(this); when(mockContext.getString(R.string.app_name)).thenReturn("RegistrationClient"); when(mockContext.getSharedPreferences(anyString(), anyInt())).thenReturn(mockSharedPreferences); when(mockSharedPreferences.edit()).thenReturn(mockEditor); when(mockEditor.putLong(anyString(), anyLong())).thenReturn(mockEditor); }
404-430: Inconsistent test pattern.This test uses
try-catch-fail()pattern while other similar tests use@Test(expected = ClientCheckedException.class). For consistency, consider using the same pattern across all exception tests.🧹 Proposed fix
- @Test - public void testValidateCenterToMachineDistance_InvalidCoordinateFormat_ThrowsException() throws Exception { + @Test(expected = ClientCheckedException.class) + public void testValidateCenterToMachineDistance_InvalidCoordinateFormat_ThrowsException() throws Exception { // Setup: GPS enabled when(mockGlobalParamRepository.getCachedStringGpsDeviceEnableFlag()).thenReturn("N"); // Setup: Center details CenterMachineDto centerMachineDto = new CenterMachineDto(); centerMachineDto.setCenterId(CENTER_ID); when(mockMasterDataService.getRegistrationCenterMachineDetails()).thenReturn(centerMachineDto); RegistrationCenter center = new RegistrationCenter(); center.setId(CENTER_ID); center.setLangCode("eng"); center.setLatitude("invalid"); // Invalid format center.setLongitude("77.5946"); List<RegistrationCenter> centers = new ArrayList<>(); centers.add(center); when(mockRegistrationCenterRepository.getRegistrationCenter(CENTER_ID)).thenReturn(centers); // Execute - should throw exception - try { - syncStatusValidatorService.validateCenterToMachineDistance(78.5946, 13.9716); - fail("Expected ClientCheckedException for invalid coordinate format"); - } catch (ClientCheckedException e) { - // Expected - } + syncStatusValidatorService.validateCenterToMachineDistance(78.5946, 13.9716); }
244-255: Test name and comment are misleading about the GPS_DEVICE_ENABLE_FLAG semantics.The test correctly validates that validation is skipped when the flag is
"Y", but the test nameGPSDisabled_Skippedand comment "GPS validation disabled" create confusion. WhenGPS_DEVICE_ENABLE_FLAGis"Y", it means the GPS device is enabled/present, not that GPS is disabled. The implementation intentionally skips distance validation when a GPS device is available, allowing users to manually verify location.Rename or clarify the test to reflect the actual flag semantics:
- Test should indicate that GPS device is enabled when the flag is
"Y"- Comment should explain that validation is skipped because GPS device is available for manual verification
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java (1)
159-163: Consider logging at appropriate level for swallowed exceptions.The exception handling here silently continues to the next job, which is appropriate for resilience. However, for
NumberFormatException(line 160),Log.e(error level) may be too severe for what is essentially a configuration issue. Consider usingLog.w(warning) instead.🧹 Proposed fix
} catch (NumberFormatException e) { - Log.e(TAG, "Invalid frequency value for job: " + jobId + " (" + apiName + "): " + configuredFrequencyStr, e); + Log.w(TAG, "Invalid frequency value for job: " + jobId + " (" + apiName + "): " + configuredFrequencyStr + " - skipping validation"); } catch (Exception e) { Log.e(TAG, "Error validating job: " + jobId + " (" + apiName + ")", e); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
android/app/src/main/java/io/mosip/registration_client/api_services/RegistrationApi.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/config/AppModule.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/RegistrationConstants.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/spi/SyncStatusValidatorService.javaandroid/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImplTest.javaassets/l10n/app_ar.arbassets/l10n/app_en.arbassets/l10n/app_fr.arbassets/l10n/app_hi.arbassets/l10n/app_kn.arbassets/l10n/app_ta.arb
🧰 Additional context used
🧬 Code graph analysis (3)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java (3)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/GlobalParamRepository.java (1)
GlobalParamRepository(21-254)android/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/RegistrationCenterRepository.java (1)
RegistrationCenterRepository(11-39)android/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/SyncJobDefRepository.java (1)
SyncJobDefRepository(18-39)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/config/AppModule.java (2)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java (1)
Singleton(96-771)android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java (1)
Singleton(35-318)
android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImplTest.java (4)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/RegistrationConstants.java (1)
RegistrationConstants(9-148)android/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/GlobalParamRepository.java (1)
GlobalParamRepository(21-254)android/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/RegistrationCenterRepository.java (1)
RegistrationCenterRepository(11-39)android/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/SyncJobDefRepository.java (1)
SyncJobDefRepository(18-39)
🔇 Additional comments (19)
android/app/src/main/java/io/mosip/registration_client/api_services/RegistrationApi.java (1)
27-27: LGTM! Clean error handling for validation exceptions.The addition of
ClientCheckedExceptionhandling enables structured validation error propagation. The catch block is correctly positioned before the genericExceptionhandler, and returninge.getErrorCode()allows the Flutter layer to receive specific error codes (likeOPT_TO_REG_TIME_SYNC_EXCEEDandOPT_TO_REG_OUTSIDE_LOCATION) for localized display.Also applies to: 56-57
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/config/AppModule.java (3)
51-51: LGTM! Proper service imports added.The imports for
SyncStatusValidatorServiceImpl(concrete implementation) andSyncStatusValidatorService(interface) are correctly added to support the new validation service.Also applies to: 65-65
211-212: LGTM! Dependency injection correctly updated.The
provideRegistrationServicemethod signature now includesSyncStatusValidatorServiceas a parameter, and the service is properly passed to theRegistrationServiceImplconstructor. This follows the existing DI patterns in the module.Also applies to: 215-215
316-328: LGTM! Well-structured provider method.The
provideSyncStatusValidatorServiceprovider method correctly:
- Returns the interface type (
SyncStatusValidatorService) rather than the implementation- Provides all 8 required dependencies for
SyncStatusValidatorServiceImpl- Is annotated as
@Singletonfor proper lifecycle management- Follows Dagger conventions used throughout this module
assets/l10n/app_kn.arb (1)
305-305: LGTM! Localization tokens added for Kannada.The two new error tokens (
OPT_TO_REG_TIME_SYNC_EXCEEDandOPT_TO_REG_OUTSIDE_LOCATION) have been added to the Kannada localization file, maintaining consistency with the English version.assets/l10n/app_hi.arb (1)
305-305: LGTM! Localization tokens added for Hindi.The two new error tokens (
OPT_TO_REG_TIME_SYNC_EXCEEDandOPT_TO_REG_OUTSIDE_LOCATION) have been added to the Hindi localization file, maintaining consistency with the English version.assets/l10n/app_ar.arb (1)
305-305: LGTM: Localization keys added for new validation features.The Arabic translations for the two new error messages (
OPT_TO_REG_TIME_SYNC_EXCEEDandOPT_TO_REG_OUTSIDE_LOCATION) have been added consistently with the existing error message pattern.assets/l10n/app_fr.arb (1)
305-305: LGTM: Localization keys added for new validation features.The French translations for the two new error messages (
OPT_TO_REG_TIME_SYNC_EXCEEDandOPT_TO_REG_OUTSIDE_LOCATION) have been added consistently with the existing error message pattern.assets/l10n/app_ta.arb (1)
314-314: LGTM: Localization keys added for new validation features.The Tamil translations for the two new error messages (
OPT_TO_REG_TIME_SYNC_EXCEEDandOPT_TO_REG_OUTSIDE_LOCATION) have been added consistently with the existing error message pattern.android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java (3)
71-71: LGTM: Dependency injection properly implemented.The
SyncStatusValidatorServicedependency has been correctly wired through constructor injection, following the existing pattern used for other services in this class. The field is properly declared, assigned, and ready for use.Also applies to: 118-118, 136-137, 153-153
575-578: LGTM: Sync status validation properly placed in pre-checks.The sync status validation is correctly positioned in the pre-registration checks sequence, ensuring validation occurs before any registration processing begins. The null-safe invocation pattern is consistent with other optional validations in this method.
200-210: Parameter order is correct and consistent with the method signature.The call at lines 203-205 passes
longitudethenlatitude, which matches thevalidateCenterToMachineDistance(Double machineLongitude, Double machineLatitude)method signature. Lines 500-503 only access the fields separately, not as method parameters, so they don't establish a different ordering convention. The code is internally consistent; parameter order is not an issue.Regarding timing: Validation immediately follows DTO initialization, which is appropriate since the geolocation data must be available to validate it. The DTO is a lightweight data container (initialized with the constructor parameters), not a resource-heavy object, so early vs. late validation has minimal practical impact.
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/RegistrationConstants.java (1)
121-148: LGTM!The new constants are appropriately named and follow existing conventions. They correctly support the new sync status and GPS location validation features.
android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImplTest.java (2)
269-300: LGTM!The test correctly validates the success scenario when the machine is within the allowed distance from the registration center. Mock setup is clean and assertions verify the expected behavior.
521-539: LGTM!The helper method is well-structured and promotes test code reuse across multiple test cases.
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java (4)
49-67: LGTM!Constructor properly injects all required dependencies for the validation service.
244-248: GPS flag logic may be inverted.The condition skips validation when
enableFlagis"Y"(GPS device enabled). This is counterintuitive—typically you would validate distance when GPS is available, not skip it.If
GPS_DEVICE_ENABLE_FLAG = "Y"means "GPS validation should be enabled," then the condition should be inverted:🔧 Potential fix if logic is inverted
String enableFlag = globalParamRepository.getCachedStringGpsDeviceEnableFlag(); - if (enableFlag == null || "Y".equalsIgnoreCase(enableFlag)) { - Log.d(TAG, "GPS distance validation disabled or not configured, skipping"); + if (enableFlag == null || "N".equalsIgnoreCase(enableFlag)) { + Log.d(TAG, "GPS device disabled or not configured, skipping distance validation"); return; }Please verify the intended semantics of
GPS_DEVICE_ENABLE_FLAG:
- Does
"Y"mean GPS device is enabled (and validation should run)?- Or does
"Y"mean GPS validation feature should be bypassed?
35-47: LGTM!The service is correctly annotated as
@Singletonand doesn't maintain shared mutable state. Thread safety depends on injected dependencies, which should be thread-safe by design.
165-170: LGTM!The error message construction clearly communicates the validation failure with actionable details about which jobs are overdue.
...rc/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java
Show resolved
Hide resolved
...rc/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java
Show resolved
Hide resolved
...anager/src/main/java/io/mosip/registration/clientmanager/spi/SyncStatusValidatorService.java
Show resolved
Hide resolved
Signed-off-by: sachin.sp <sachin.sp@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java:
- Around line 244-248: The GPS enable-flag check is inverted: currently it
returns (skips validation) when enableFlag is "Y"; change the condition so the
method only skips when the flag is absent or not set to "Y". Locate the
getCachedStringGpsDeviceEnableFlag() call and the surrounding if using the
enableFlag variable in SyncStatusValidatorServiceImpl, and replace the predicate
with one that returns early when enableFlag == null ||
!"Y".equalsIgnoreCase(enableFlag) so that validation runs only when the flag is
"Y".
In @android/clientmanager/src/main/res/values/strings.xml:
- Line 15: The message string named err_outside_registration_center uses both
"center" and "centre"; pick one spelling and make them consistent in the string
value (e.g., change both to "center" or both to "centre") so the displayed
message uses the same variant throughout while keeping the existing identifier
err_outside_registration_center unchanged.
🧹 Nitpick comments (3)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/spi/SyncStatusValidatorService.java (1)
10-24: Consider declaringClientCheckedExceptioninstead of genericException.Both methods throw
Exception, but the implementation only throwsClientCheckedException. Declaring the more specific exception type improves API clarity and enables callers to handle validation failures distinctly.♻️ Suggested refinement
- void validateSyncStatus() throws Exception; + void validateSyncStatus() throws ClientCheckedException; ... - void validateCenterToMachineDistance(Double machineLongitude, Double machineLatitude) throws Exception; + void validateCenterToMachineDistance(Double machineLongitude, Double machineLatitude) throws ClientCheckedException;android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java (2)
3-5: Remove unused import.
SharedPreferencesis imported but never used in this class.🧹 Suggested fix
import android.content.Context; -import android.content.SharedPreferences; import android.util.Log;
273-277: Inconsistent handling of missing data.When center details or center list is unavailable (lines 256-267), validation is skipped. However, when center coordinates are missing, an exception is thrown. Consider consistent behavior—either skip validation (if this is expected in some setups) or fail consistently for all missing center data.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/spi/SyncStatusValidatorService.javaandroid/clientmanager/src/main/res/values/strings.xml
🧰 Additional context used
🧬 Code graph analysis (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java (4)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/RegistrationConstants.java (1)
RegistrationConstants(9-148)android/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/GlobalParamRepository.java (1)
GlobalParamRepository(21-254)android/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/RegistrationCenterRepository.java (1)
RegistrationCenterRepository(11-39)android/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/SyncJobDefRepository.java (1)
SyncJobDefRepository(18-39)
🔇 Additional comments (3)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java (3)
74-91: LGTM!The exception handling properly distinguishes between expected validation failures and unexpected errors, with appropriate logging.
148-157: Verify the boundary condition for overdue detection.Using
<=means a job withconfiguredFrequency=7is marked overdue whenactualDays=7(exactly at the limit). If the intent is "must sync within N days," the job should be overdue only after exceeding the limit (i.e.,actualDays > configuredFrequency).Please confirm whether a sync performed exactly at the configured frequency limit should block registration. If "within N days" is the requirement, consider:
- if (configuredFrequency <= actualDays) { + if (actualDays > configuredFrequency) {
223-231: LGTM!The day calculation logic correctly uses day-level granularity with appropriate null handling.
...rc/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java
Show resolved
Hide resolved
Signed-off-by: sachin.sp <sachin.sp@cyberpwn.com>
…registration-client into RCF-1354 # Conflicts: # android/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/RegistrationConstants.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java:
- Around line 148-157: The condition treating syncs as overdue uses <= which
flags jobs that ran exactly at the configured interval; change the comparison in
the sync validation block that references configuredFrequency and actualDays to
use a strict greater-than (actualDays > configuredFrequency) so only jobs that
exceeded the configuredFrequency increment syncFailureCount, append to
errorDetails, and log the overdue message for jobId/apiName; update the
conditional around the existing syncFailureCount++, errorDetails.append(...),
and Log.w(...) accordingly.
🧹 Nitpick comments (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java (1)
75-75: Consider more specific exception declaration.The method signature declares
throws Exception, which is very broad. Consider declaringthrows ClientCheckedExceptionor a more specific exception type to improve type safety and make the contract clearer to callers.♻️ Suggested improvement
- public void validateSyncStatus() throws Exception { + public void validateSyncStatus() throws ClientCheckedException {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.javaandroid/clientmanager/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- android/clientmanager/src/main/res/values/strings.xml
🔇 Additional comments (5)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java (5)
35-47: LGTM: Clean dependency injection setup.The singleton service is properly structured with all dependencies declared as private fields and injected via constructor using Dagger.
49-67: LGTM: Constructor follows Dagger injection pattern.All dependencies are properly assigned. Dagger's DI contract ensures non-null values at runtime.
181-215: LGTM: Proper configuration retrieval with null safety.The method correctly builds property names from constants, retrieves configuration values, and filters out invalid entries (null, empty, or literal "null" strings). Good defensive programming.
223-231: LGTM: Day calculation is mathematically correct.The integer division at line 229 truncates partial days (e.g., 5 days 23 hours becomes 5 days), which is the expected behavior for calculating whole days since last sync.
241-316: LGTM: Robust GPS distance validation with proper error handling.The method correctly:
- Checks GPS enable flag and coordinates availability with early returns
- Retrieves center details with null safety
- Calculates and converts distance (km to meters)
- Uses strict
>comparison at line 301 (machines at exactly the max distance are allowed)- Handles parsing exceptions and missing configurations appropriately
...rc/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/RegistrationConstants.java (1)
3-7: Remove duplicate import statement.Line 5 contains a duplicate import of
java.util.ArrayList(also imported on Line 4).🧹 Proposed fix
import java.sql.Array; import java.util.ArrayList; -import java.util.ArrayList; import java.util.Arrays; import java.util.List;
🤖 Fix all issues with AI agents
In
@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java:
- Around line 200-210: The GPS distance validation is never reached because
registrationDto.getGeoLocationDto() is not populated before validation; move the
validateCenterToMachineDistance(...) call out of the constructor/init path and
execute it after the location is set (i.e., after setMachineLocation() in
RegistrationApi.java) or call registrationDto.setGeoLocation(...) prior to this
validation; specifically, ensure
syncStatusValidatorService.validateCenterToMachineDistance(...) is invoked in
the same flow/method that sets the geo location (or add a post-location-capture
hook) so validateCenterToMachineDistance(...) runs with a non-null
registrationDto.getGeoLocationDto().
🧹 Nitpick comments (1)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/RegistrationConstants.java (1)
148-152: Consider more specific names for generic string builder constants.The constants
DOTandFREQUENCYare very generic and could potentially be reused in unrelated contexts, making the code harder to maintain. If these are specifically for building sync validation configuration keys, consider more descriptive names likeSYNC_KEY_DOTandSYNC_KEY_FREQUENCY, or consolidate key construction into a helper method.Alternative approach
If these constants are only used to build configuration keys like
"mosip.registration.{jobId}.frequency", consider a helper method instead:// Instead of public generic constants, use a helper method public static String buildSyncFrequencyKey(String jobId) { return "mosip.registration." + jobId + ".frequency"; }This eliminates the need for public
DOTandFREQUENCYconstants while making the intent clearer.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/RegistrationConstants.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java
🔇 Additional comments (3)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java (2)
71-71: LGTM: Service wiring is correct.The SyncStatusValidatorService is properly imported, declared as a field, added to the constructor, and assigned. The dependency injection follows the established pattern in this class.
Also applies to: 118-118, 136-137, 153-153
576-579: LGTM: Sync status validation is properly integrated.The validation is correctly placed in the pre-check sequence before registration begins. The null check is defensive and consistent with other service checks in this method (Lines 582, 587, 592). The comment clearly explains the purpose of the validation.
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/constant/RegistrationConstants.java (1)
121-123: LGTM: GPS and registration option constants are well-defined.The new constants
GEO_CAP_FREQ,OPT_TO_REG_OUTSIDE_LOCATION, andOPT_TO_REG_LAST_CAPTURED_TIMEfollow the existing naming conventions and are appropriately placed near related GPS configuration constants.
| // Validate machine distance from registration center | ||
| if (syncStatusValidatorService != null && this.registrationDto.getGeoLocationDto() != null) { | ||
| try { | ||
| syncStatusValidatorService.validateCenterToMachineDistance( | ||
| this.registrationDto.getGeoLocationDto().getLongitude(), | ||
| this.registrationDto.getGeoLocationDto().getLatitude()); | ||
| } catch (ClientCheckedException e) { | ||
| Log.e(TAG, "Location validation failed", e); | ||
| throw e; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for setGeoLocationDto or setGeoLocation calls in the RegistrationServiceImpl file
rg -nP --type=java 'setGeoLocation(Dto)?\s*\(' android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java
# Also search more broadly for where GeoLocationDto is assigned/set
rg -nP --type=java 'geoLocationDto\s*=' android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.javaRepository: mosip/android-registration-client
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Get more context around lines 190-220 to see the full flow and where geoLocationDto comes from
sed -n '190,220p' android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java | cat -nRepository: mosip/android-registration-client
Length of output: 1793
🏁 Script executed:
#!/bin/bash
# Search for where GeoLocationDto is instantiated in this file
rg -nP --type=java 'new\s+GeoLocationDto\s*\(' android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.javaRepository: mosip/android-registration-client
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Find the RegistrationDto class
fd -t f -i 'RegistrationDto.java' android/clientmanagerRepository: mosip/android-registration-client
Length of output: 286
🏁 Script executed:
#!/bin/bash
# Search for RegistrationDto class definition across the codebase
fd -t f -i 'registrationdto.java'Repository: mosip/android-registration-client
Length of output: 286
🏁 Script executed:
#!/bin/bash
# Search for where geoLocationDto is set or populated in the entire project
rg -nP --type=java 'geoLocationDto\s*=' --max-count=20Repository: mosip/android-registration-client
Length of output: 837
🏁 Script executed:
#!/bin/bash
# Look for GeoLocationDto in the broader context to understand when/where it's captured
rg -nP --type=java 'GeoLocationDto' --max-count=30Repository: mosip/android-registration-client
Length of output: 4240
🏁 Script executed:
#!/bin/bash
# Look at RegistrationDto.java around line 488 to see what method sets geoLocationDto
sed -n '480,500p' android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dto/registration/RegistrationDto.java | cat -nRepository: mosip/android-registration-client
Length of output: 505
🏁 Script executed:
#!/bin/bash
# Search for the method that contains line 488 (the setGeoLocation or similar)
sed -n '480,510p' android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dto/registration/RegistrationDto.java | cat -nRepository: mosip/android-registration-client
Length of output: 505
🏁 Script executed:
#!/bin/bash
# Get the full structure around geoLocationDto in RegistrationDto to find the method signature
rg -nP --type=java -B5 -A5 'this\.geoLocationDto = new GeoLocationDto' android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dto/registration/RegistrationDto.javaRepository: mosip/android-registration-client
Length of output: 327
🏁 Script executed:
#!/bin/bash
# Search for where setGeoLocation (or any method that sets geoLocationDto) is called in RegistrationServiceImpl
rg -nP --type=java 'setGeoLocation|geoLocationDto' android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.javaRepository: mosip/android-registration-client
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Search for ALL calls to setGeoLocation in the entire codebase to understand when/where location is captured
rg -nP --type=java 'setGeoLocation' --max-count=20Repository: mosip/android-registration-client
Length of output: 422
🏁 Script executed:
#!/bin/bash
# Look at the full method around the validation check to see if there's missing location capture logic
sed -n '150,230p' android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java | cat -nRepository: mosip/android-registration-client
Length of output: 4095
🏁 Script executed:
#!/bin/bash
# Check the context around RegistrationApi.java:155 where setGeoLocation is called
sed -n '140,170p' android/app/src/main/java/io/mosip/registration_client/api_services/RegistrationApi.java | cat -nRepository: mosip/android-registration-client
Length of output: 1054
🏁 Script executed:
#!/bin/bash
# Get more context to understand the flow - search for the method name containing line 155
sed -n '100,180p' android/app/src/main/java/io/mosip/registration_client/api_services/RegistrationApi.java | cat -nRepository: mosip/android-registration-client
Length of output: 3364
GPS validation is unreachable: geoLocationDto is never populated before the validation check.
The validation block checks this.registrationDto.getGeoLocationDto() immediately after creating a new RegistrationDto instance (line 49), but location data is only set later via setMachineLocation() in RegistrationApi.java (line 54-60), which is a separate method call that occurs after registration is already initiated. Since setGeoLocation() is never called before line 52, the condition will always fail silently, causing the distance validation to never execute.
Either move the validation to occur after location capture, or restructure the flow so location is available before validation is attempted.
🤖 Prompt for AI Agents
In
@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java
around lines 200 - 210, The GPS distance validation is never reached because
registrationDto.getGeoLocationDto() is not populated before validation; move the
validateCenterToMachineDistance(...) call out of the constructor/init path and
execute it after the location is set (i.e., after setMachineLocation() in
RegistrationApi.java) or call registrationDto.setGeoLocation(...) prior to this
validation; specifically, ensure
syncStatusValidatorService.validateCenterToMachineDistance(...) is invoked in
the same flow/method that sets the geo location (or add a post-location-capture
hook) so validateCenterToMachineDistance(...) runs with a non-null
registrationDto.getGeoLocationDto().
Signed-off-by: sachin.sp <sachin.sp@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java:
- Around line 200-210: The distance validation block is dead because
registrationDto.getGeoLocationDto() is null at startRegistration(); move the
check out of startRegistration() and into a point after setMachineLocation() is
called (or into packet submission) so GPS is guaranteed to be present.
Specifically, remove or disable the validateCenterToMachineDistance call in
RegistrationServiceImpl.startRegistration(), and invoke
syncStatusValidatorService.validateCenterToMachineDistance(...) after
setMachineLocation(...) completes (or inside the packet creation/submission
flow), keeping the same exception handling (catch ClientCheckedException,
Log.e(TAG,...), rethrow) and using
registrationDto.getGeoLocationDto().getLongitude()/getLatitude() as before.
In
@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java:
- Around line 241-316: In validateCenterToMachineDistance, when
getCachedStringGpsDeviceEnableFlag() is "Y" do not silently skip if
machineLongitude or machineLatitude are null: replace the early return for null
machine coords with a thrown ClientCheckedException (use the same client error
pattern/context as other failures, e.g., new ClientCheckedException(context,
R.string.err_004)) so GPS-enabled mode enforces presence of device location;
also update the NumberFormatException log message in the catch block from
"Invalid center coordinates format" to "Invalid number format in center
coordinates or max distance configuration" and include the exception object in
the log to reflect that Double.parseDouble(maxDistanceStr) can fail.
🧹 Nitpick comments (4)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java (2)
118-154: MakeSyncStatusValidatorServicea required injected dependency (avoid silent validation bypass).Right now
syncStatusValidatorServiceis constructor-injected but all call sites guard it with!= null, which can hide DI misconfiguration and silently skip “must-run” validations (sync-status and distance). Prefer enforcing non-null via constructor and dropping the null checks (or failing fast).Proposed adjustment
@@ - private SyncStatusValidatorService syncStatusValidatorService; + private final SyncStatusValidatorService syncStatusValidatorService; @@ public RegistrationServiceImpl(Context context, PacketWriterService packetWriterService, @@ - PacketService packetService, - SyncStatusValidatorService syncStatusValidatorService) { + PacketService packetService, + SyncStatusValidatorService syncStatusValidatorService) { @@ this.packetService = packetService; - this.syncStatusValidatorService = syncStatusValidatorService; + if (syncStatusValidatorService == null) { + throw new IllegalArgumentException("syncStatusValidatorService must not be null"); + } + this.syncStatusValidatorService = syncStatusValidatorService; }
200-210: Redundant catch/rethrow (log once at the boundary).Catching
ClientCheckedException, logging, and rethrowing doesn’t change behavior. If you want centralized logging, prefer logging at the API boundary (where errors are presented to callers) to avoid double logs; otherwise remove the try/catch.android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java (2)
40-67: Preferfinalfields for injected dependencies.These are constructor-injected and never reassigned; marking them
finalreduces accidental mutation and improves thread-safety/readability.Proposed adjustment
@@ - private Context context; - private SyncJobDefRepository syncJobDefRepository; - private GlobalParamRepository globalParamRepository; - private JobManagerService jobManagerService; - private JobTransactionService jobTransactionService; - private LocationValidationService locationValidationService; - private MasterDataService masterDataService; - private RegistrationCenterRepository registrationCenterRepository; + private final Context context; + private final SyncJobDefRepository syncJobDefRepository; + private final GlobalParamRepository globalParamRepository; + private final JobManagerService jobManagerService; + private final JobTransactionService jobTransactionService; + private final LocationValidationService locationValidationService; + private final MasterDataService masterDataService; + private final RegistrationCenterRepository registrationCenterRepository;
223-231: Day calculation: considerjava.time/TimeUnitto avoid edge cases.The integer division is OK, but using
TimeUnit.MILLISECONDS.toDays(...)orjava.time.Instant+ChronoUnit.DAYSis clearer and avoids “magic numbers”.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java
🔇 Additional comments (2)
android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java (1)
576-579: Sync-status pre-check placement looks good; ensure it’s “fail-closed” if required.Calling
validateSyncStatus()insidedoPreChecksBeforeRegistration(...)is the right place to block early. As above, consider removing thesyncStatusValidatorService != nullguard if this validation is mandatory.android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java (1)
181-215: No issue: MOSIP_REGISTRATION constant already includes the required dot separator.The constant
MOSIP_REGISTRATION = "mosip.registration."already includes the trailing dot, so the propertyName is correctly formatted asmosip.registration.{apiName}.frequency. The key format is valid and will not cause silent lookup failures due to missing separators.Likely an incorrect or invalid review comment.
| // Validate machine distance from registration center if location is available | ||
| if (syncStatusValidatorService != null && this.registrationDto.getGeoLocationDto() != null) { | ||
| try { | ||
| syncStatusValidatorService.validateCenterToMachineDistance( | ||
| this.registrationDto.getGeoLocationDto().getLongitude(), | ||
| this.registrationDto.getGeoLocationDto().getLatitude()); | ||
| } catch (ClientCheckedException e) { | ||
| Log.e(TAG, "Location validation failed", e); | ||
| throw e; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Where is geo-location attached to RegistrationDto?
rg -n --hidden -S "\bsetGeoLocationDto\s*\(" .
# Any other patterns that mutate geo location?
rg -n --hidden -S "\bGeoLocationDto\b" android/clientmanager/src/main/java | head -n 200
# Where is validateCenterToMachineDistance invoked?
rg -n --hidden -S "\bvalidateCenterToMachineDistance\s*\(" android/clientmanager/src/main/javaRepository: mosip/android-registration-client
Length of output: 59
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check repository structure
echo "=== Repository root ==="
ls -la | head -20
# Find the target file
echo -e "\n=== Finding RegistrationServiceImpl.java ==="
find . -name "RegistrationServiceImpl.java" -type f 2>/dev/null || echo "Not found with find"
# Try locating android directory
echo -e "\n=== Android directories ==="
find . -type d -name "android" 2>/dev/null | head -10
# List files in repo root
echo -e "\n=== Git files ==="
git ls-files | grep -i registration | head -20Repository: mosip/android-registration-client
Length of output: 3314
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the target file around lines 200-210
echo "=== RegistrationServiceImpl.java context ==="
cat -n android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java | sed -n '190,220p'
echo -e "\n=== Search for setGeoLocationDto ==="
grep -rn "setGeoLocationDto" android/clientmanager/src/main/java/ 2>/dev/null || echo "Not found"
echo -e "\n=== Search for GeoLocationDto ==="
grep -rn "GeoLocationDto" android/clientmanager/src/main/java/ 2>/dev/null | head -30
echo -e "\n=== Search for validateCenterToMachineDistance ==="
grep -rn "validateCenterToMachineDistance" android/clientmanager/src/main/java/ 2>/dev/nullRepository: mosip/android-registration-client
Length of output: 4099
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check RegistrationDto constructor and geoLocationDto field
echo "=== RegistrationDto.java - field and constructor ==="
cat -n android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dto/registration/RegistrationDto.java | sed -n '80,110p'
echo -e "\n=== RegistrationDto.java - line 488 context ==="
cat -n android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dto/registration/RegistrationDto.java | sed -n '480,510p'
echo -e "\n=== RegistrationServiceImpl.java - line 500 context ==="
cat -n android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java | sed -n '495,520p'
echo -e "\n=== Check the constructor used at line 198 ==="
cat -n android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dto/registration/RegistrationDto.java | head -100 | tail -60Repository: mosip/android-registration-client
Length of output: 6811
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all calls to setGeoLocation
echo "=== Search for setGeoLocation calls ==="
grep -rn "setGeoLocation" android/clientmanager/src/ 2>/dev/null
# Get method signature and context of startRegistration/initialization methods
echo -e "\n=== Find methods that call startRegistration or initialize flow ==="
grep -rn "fun startRegistration\|void startRegistration\|public.*startRegistration" android/clientmanager/src/main/java/ 2>/dev/null | head -10
# Look at the full context of the initialization method
echo -e "\n=== Full context of RegistrationServiceImpl initialization ==="
cat -n android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java | sed -n '150,220p'Repository: mosip/android-registration-client
Length of output: 4278
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there are any other geo/location related operations
echo "=== All references to geoLocation ==="
grep -rn "geoLocation\|GeoLocation" android/clientmanager/src/main/java/ 2>/dev/null
# Check the GeoLocationDto class definition
echo -e "\n=== GeoLocationDto class ==="
cat -n android/clientmanager/src/main/java/io/mosip/registration/clientmanager/dto/registration/GeoLocationDto.java
# Check if there are any Location or GPS related services
echo -e "\n=== Search for Location/GPS services ==="
grep -rn "Location\|GPS\|location" android/clientmanager/src/main/java/ 2>/dev/null | grep -i "service\|manager\|provider" | head -15Repository: mosip/android-registration-client
Length of output: 5383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if setGeoLocation is called from the app layer
echo "=== Search in app directory for setGeoLocation or similar ==="
find android/app -type f \( -name "*.kt" -o -name "*.java" \) -exec grep -l "setGeoLocation\|geoLocation" {} \; 2>/dev/null
# Check the lib/flutter side for location handling
echo -e "\n=== Check Dart/Flutter lib for location handling ==="
find lib -type f -name "*.dart" -exec grep -l "location\|Location" {} \; 2>/dev/null | head -10
# Check if there's any usage in tests
echo -e "\n=== Check test files for setGeoLocation usage ==="
find android/clientmanager/src/test -type f -name "*.java" -exec grep -l "setGeoLocation\|geoLocation" {} \; 2>/dev/null
# Get file count and quick check
echo -e "\n=== Quick count of all files with setGeoLocation ==="
grep -r "setGeoLocation" . 2>/dev/null | wc -lRepository: mosip/android-registration-client
Length of output: 925
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check where setGeoLocation is called in RegistrationApi
echo "=== RegistrationApi.java - setGeoLocation usage ==="
grep -B 5 -A 5 "setGeoLocation" android/app/src/main/java/io/mosip/registration_client/api_services/RegistrationApi.java
# Check the test file
echo -e "\n=== RegistrationServiceImplTest.java - setGeoLocation usage ==="
grep -B 5 -A 5 "setGeoLocation" android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/RegistrationServiceImplTest.java
# Check the Flutter registration service
echo -e "\n=== registration_service_impl.dart - location handling ==="
grep -n "location\|Location" lib/platform_android/registration_service_impl.dart | head -20
# Check if location is passed to startRegistration
echo -e "\n=== Check method signatures in RegistrationApi ==="
cat -n android/app/src/main/java/io/mosip/registration_client/api_services/RegistrationApi.java | head -150 | tail -100Repository: mosip/android-registration-client
Length of output: 606
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where setMachineLocation is called
echo "=== Search for setMachineLocation calls ==="
grep -rn "setMachineLocation" . 2>/dev/null | grep -v ".git"
# Check the Flutter registration flow
echo -e "\n=== registration_service_impl.dart - full context ==="
cat -n lib/platform_android/registration_service_impl.dart | head -100
# Look for where location is captured or set
echo -e "\n=== Flutter registration_task_provider.dart ==="
grep -n "startRegistration\|setMachineLocation\|location" lib/provider/registration_task_provider.dart | head -30Repository: mosip/android-registration-client
Length of output: 4441
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the GPS location design document
echo "=== gps_location.md design document ==="
cat -n docs/design/gps_location.md
# Find where setMachineLocation is called in Flutter
echo -e "\n=== registration_task_provider.dart around line 358 ==="
cat -n lib/provider/registration_task_provider.dart | sed -n '350,370p'
# Check context around line 137-139
echo -e "\n=== registration_task_provider.dart around startRegistration call ==="
cat -n lib/provider/registration_task_provider.dart | sed -n '130,150p'Repository: mosip/android-registration-client
Length of output: 8211
Distance validation is dead code and will never execute—geoLocationDto is always null at this point in the flow.
The validation at lines 200-210 checks for a non-null geoLocationDto, but the object is only populated later via setMachineLocation() (called from the Flutter layer after startRegistration() completes). At the time this check runs, geoLocationDto is still null from the constructor, making the validation block unreachable.
To fix, either:
- Populate
geoLocationDtobeforestartRegistration()returns (requires GPS capture to complete before method exit), or - Move the distance validation to a point where GPS location is guaranteed to be available (e.g., during packet submission), or
- Restructure the flow so validation occurs after
setMachineLocation()is called.
The design document (docs/design/gps_location.md) indicates GPS should be captured at packet creation time, suggesting the validation should not run at startRegistration() at all—it should be deferred to when location is actually available.
🤖 Prompt for AI Agents
In
@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java
around lines 200 - 210, The distance validation block is dead because
registrationDto.getGeoLocationDto() is null at startRegistration(); move the
check out of startRegistration() and into a point after setMachineLocation() is
called (or into packet submission) so GPS is guaranteed to be present.
Specifically, remove or disable the validateCenterToMachineDistance call in
RegistrationServiceImpl.startRegistration(), and invoke
syncStatusValidatorService.validateCenterToMachineDistance(...) after
setMachineLocation(...) completes (or inside the packet creation/submission
flow), keeping the same exception handling (catch ClientCheckedException,
Log.e(TAG,...), rethrow) and using
registrationDto.getGeoLocationDto().getLongitude()/getLatitude() as before.
| public void validateSyncStatus() throws Exception { | ||
| Log.i(TAG, "Validating sync status started"); | ||
|
|
||
| try { | ||
| // Validate sync job frequencies | ||
| validatingSyncJobsConfig(); | ||
|
|
||
| Log.i(TAG, "Sync status validation completed successfully"); | ||
|
|
||
| } catch (ClientCheckedException e) { | ||
| Log.e(TAG, "Sync status validation failed", e); | ||
| throw e; | ||
| } catch (Exception e) { | ||
| Log.e(TAG, "Unexpected error during sync status validation", e); | ||
| throw new ClientCheckedException(context, R.string.err_004); | ||
| } | ||
| } |
There was a problem hiding this comment.
Don’t map “unexpected sync validation error” to err_004 unless it’s the correct UX.
err_004 is thrown for any unexpected exception, which can obscure the real failure mode and potentially show an unrelated message. Prefer a dedicated “sync validation failed” error token/string (or include a stable error code) while still logging the underlying exception.
| private void validatingSyncJobsConfig() throws Exception { | ||
| Log.i(TAG, "Validating sync jobs configuration started"); | ||
|
|
||
| List<SyncJobDef> activeJobs = syncJobDefRepository.getActiveSyncJobs(); | ||
| if (activeJobs == null || activeJobs.isEmpty()) { | ||
| Log.w(TAG, "No active sync jobs found, skipping validation"); | ||
| return; | ||
| } | ||
|
|
||
| Map<String, String> jobFrequencyMap = getSyncJobFrequencies(activeJobs); | ||
| if (jobFrequencyMap.isEmpty()) { | ||
| Log.w(TAG, "No sync job frequencies configured, skipping validation"); | ||
| return; | ||
| } | ||
| int syncFailureCount = 0; | ||
| StringBuilder errorDetails = new StringBuilder(); | ||
|
|
||
| for (SyncJobDef syncJobDef : activeJobs) { | ||
| String jobId = syncJobDef.getId(); | ||
| String apiName = syncJobDef.getApiName(); | ||
|
|
||
| if (jobId == null || apiName == null) { | ||
| Log.w(TAG, "Skipping job with null id or apiName"); | ||
| continue; | ||
| } | ||
|
|
||
| String configuredFrequencyStr = jobFrequencyMap.get(jobId); | ||
| if (configuredFrequencyStr == null || configuredFrequencyStr.trim().isEmpty()) { | ||
| Log.d(TAG, "No frequency configured for job: " + jobId + " (" + apiName + ")"); | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| int configuredFrequency = Integer.parseInt(configuredFrequencyStr.trim()); | ||
| int serviceJobId = jobManagerService.generateJobServiceId(jobId); | ||
| long lastSyncTimeMillis = jobTransactionService.getLastSyncTime(serviceJobId); | ||
|
|
||
| if (lastSyncTimeMillis == 0) { | ||
| Log.d(TAG, "No sync history for job: " + jobId + " (" + apiName + | ||
| ") - Skipping validation"); | ||
| continue; | ||
| } | ||
|
|
||
| Date lastSyncDate = new Date(lastSyncTimeMillis); | ||
| int actualDays = getActualDays(lastSyncDate); | ||
|
|
||
| Log.d(TAG, String.format( | ||
| "Job [%s] (%s): Configured frequency=%d days, Actual days since last sync=%d", | ||
| jobId, apiName, configuredFrequency, actualDays)); | ||
| if (actualDays > configuredFrequency) { | ||
| syncFailureCount++; | ||
| errorDetails.append("- ").append(apiName) | ||
| .append(": Last sync was ").append(actualDays) | ||
| .append(" days ago (limit: ").append(configuredFrequency).append(" days)\n"); | ||
|
|
||
| Log.w(TAG, String.format( | ||
| "Sync job [%s] (%s) is overdue. Configured: %d days, Actual: %d days since last sync", | ||
| jobId, apiName, configuredFrequency, actualDays)); | ||
| } | ||
|
|
||
| } catch (NumberFormatException e) { | ||
| Log.e(TAG, "Invalid frequency value for job: " + jobId + " (" + apiName + "): " + configuredFrequencyStr, e); | ||
| } catch (Exception e) { | ||
| Log.e(TAG, "Error validating job: " + jobId + " (" + apiName + ")", e); | ||
| } | ||
| } | ||
| if (syncFailureCount > 0) { | ||
| String errorMessage = "Registration blocked: " + syncFailureCount + | ||
| " sync job(s) are overdue:\n" + errorDetails.toString(); | ||
| Log.e(TAG, errorMessage); | ||
| throw new ClientCheckedException(RegistrationConstants.OPT_TO_REG_TIME_SYNC_EXCEED, errorMessage); | ||
| } | ||
|
|
||
| Log.i(TAG, "All sync jobs validated successfully"); | ||
| } |
There was a problem hiding this comment.
Current skip-on-misconfig behavior can bypass the “max days without sync” requirement.
These cases currently allow registration to proceed:
- job has no sync history (
lastSyncTimeMillis == 0) (Lines 136-140) - frequency is missing/blank (Lines 125-129)
- frequency is non-numeric (Lines 159-160)
- any per-job validation error (Lines 161-163)
If the intent is “block registration when overdue (or unknown)”, this should likely fail-closed (or at least treat these as validation failures once GPS/sync enforcement is enabled).
One possible approach (treat unknowns as failures)
@@
- if (configuredFrequencyStr == null || configuredFrequencyStr.trim().isEmpty()) {
- Log.d(TAG, "No frequency configured for job: " + jobId + " (" + apiName + ")");
- continue;
- }
+ if (configuredFrequencyStr == null || configuredFrequencyStr.trim().isEmpty()) {
+ syncFailureCount++;
+ errorDetails.append("- ").append(apiName)
+ .append(": Missing configured frequency\n");
+ continue;
+ }
@@
- if (lastSyncTimeMillis == 0) {
- Log.d(TAG, "No sync history for job: " + jobId + " (" + apiName +
- ") - Skipping validation");
- continue;
- }
+ if (lastSyncTimeMillis == 0) {
+ syncFailureCount++;
+ errorDetails.append("- ").append(apiName)
+ .append(": No sync history found\n");
+ continue;
+ }
@@
- } catch (NumberFormatException e) {
- Log.e(TAG, "Invalid frequency value for job: " + jobId + " (" + apiName + "): " + configuredFrequencyStr, e);
- } catch (Exception e) {
- Log.e(TAG, "Error validating job: " + jobId + " (" + apiName + ")", e);
- }
+ } catch (NumberFormatException e) {
+ syncFailureCount++;
+ errorDetails.append("- ").append(apiName)
+ .append(": Invalid frequency '").append(configuredFrequencyStr).append("'\n");
+ } catch (Exception e) {
+ syncFailureCount++;
+ errorDetails.append("- ").append(apiName)
+ .append(": Validation error (").append(e.getClass().getSimpleName()).append(")\n");
+ }| public void validateCenterToMachineDistance(Double machineLongitude, Double machineLatitude) throws Exception { | ||
| Log.i(TAG, "Validating center to machine distance started"); | ||
|
|
||
| String enableFlag = globalParamRepository.getCachedStringGpsDeviceEnableFlag(); | ||
| if (enableFlag == null || !"Y".equalsIgnoreCase(enableFlag)) { | ||
| Log.d(TAG, "GPS distance validation not enabled, skipping"); | ||
| return; | ||
| } | ||
|
|
||
| if (machineLongitude == null || machineLatitude == null) { | ||
| Log.d(TAG, "Machine GPS location not available, skipping validation"); | ||
| return; | ||
| } | ||
|
|
||
| CenterMachineDto centerMachineDto = masterDataService.getRegistrationCenterMachineDetails(); | ||
| if (centerMachineDto == null) { | ||
| Log.w(TAG, "Center details not found, skipping distance validation"); | ||
| return; | ||
| } | ||
|
|
||
| List<RegistrationCenter> centers = registrationCenterRepository.getRegistrationCenter( | ||
| centerMachineDto.getCenterId()); | ||
|
|
||
| if (centers == null || centers.isEmpty()) { | ||
| Log.w(TAG, "Center not found, skipping distance validation"); | ||
| return; | ||
| } | ||
|
|
||
| RegistrationCenter center = centers.get(0); | ||
| String centerLatStr = center.getLatitude(); | ||
| String centerLonStr = center.getLongitude(); | ||
|
|
||
| if (centerLatStr == null || centerLonStr == null || | ||
| centerLatStr.isEmpty() || centerLonStr.isEmpty()) { | ||
| Log.e(TAG, "Center coordinates not available"); | ||
| throw new ClientCheckedException(context, R.string.err_004); | ||
| } | ||
|
|
||
| try { | ||
| double centerLatitude = Double.parseDouble(centerLatStr); | ||
| double centerLongitude = Double.parseDouble(centerLonStr); | ||
|
|
||
| double distanceKm = locationValidationService.getDistance( | ||
| machineLongitude, machineLatitude, | ||
| centerLongitude, centerLatitude); | ||
|
|
||
| double distanceMeters = distanceKm * 1000; | ||
|
|
||
| String maxDistanceStr = globalParamRepository.getCachedStringMachineToCenterDistance(); | ||
| if (maxDistanceStr == null || maxDistanceStr.isEmpty()) { | ||
| Log.e(TAG, "Max allowed distance configuration not found"); | ||
| throw new ClientCheckedException(context, R.string.err_004); | ||
| } | ||
|
|
||
| double maxAllowedDistance = Double.parseDouble(maxDistanceStr); | ||
|
|
||
| Log.d(TAG, String.format( | ||
| "Distance validation: Calculated=%.2f meters, Max allowed=%.2f meters", | ||
| distanceMeters, maxAllowedDistance)); | ||
|
|
||
| if (distanceMeters > maxAllowedDistance) { | ||
| Log.e(TAG, String.format( | ||
| "Machine is outside allowed distance: %.2f meters (limit: %.2f meters)", | ||
| distanceMeters, maxAllowedDistance)); | ||
| throw new ClientCheckedException( | ||
| RegistrationConstants.OPT_TO_REG_OUTSIDE_LOCATION, | ||
| context.getString(R.string.err_outside_registration_center)); | ||
| } | ||
|
|
||
| Log.i(TAG, "Location validated successfully - machine is within allowed distance"); | ||
|
|
||
| } catch (NumberFormatException e) { | ||
| Log.e(TAG, "Invalid center coordinates format", e); | ||
| throw new ClientCheckedException(context, R.string.err_004); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find LocationValidationService interface and implementations
echo "=== LocationValidationService definitions ==="
rg -n --hidden -S "interface LocationValidationService|class.*LocationValidationService" android/clientmanager/src/main/java -A 5
# Find getDistance method signatures
echo -e "\n=== getDistance method signatures ==="
rg -n --hidden -S "def getDistance|getDistance\s*\(" android/clientmanager/src/main/java -B 2 -A 3 | head -n 100
# Find getCachedStringMachineToCenterDistance usage and documentation
echo -e "\n=== getCachedStringMachineToCenterDistance references ==="
rg -n --hidden -S "getCachedStringMachineToCenterDistance|MACHINE_TO_CENTER_DISTANCE|machineToCenterDistance" android/clientmanager/src/main/java -B 2 -A 2 | head -n 100
# Check for any constants or config that define the distance unit
echo -e "\n=== Distance unit references (km, meters, etc) ==="
rg -n --hidden -S "(kilometers?|meters?|km|distance.*unit)" android/clientmanager/src/main/java -i | head -n 50Repository: mosip/android-registration-client
Length of output: 10409
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find DIST_FRM_MACHINE_TO_CENTER constant definition
echo "=== DIST_FRM_MACHINE_TO_CENTER constant definition ==="
rg -n --hidden -S "DIST_FRM_MACHINE_TO_CENTER" android/clientmanager/src/main/java -B 2 -A 2
# Search for where this constant is set/used
echo -e "\n=== Where DIST_FRM_MACHINE_TO_CENTER is populated ==="
rg -n --hidden -S "DIST_FRM_MACHINE_TO_CENTER|machineToCenterDistance" android/clientmanager/src/main/java -B 2 -A 2 | head -n 150
# Check for any test files that might show expected values
echo -e "\n=== Test files related to distance validation ==="
find android/clientmanager -name "*Test*.java" -o -name "*test*.java" | head -n 20
# Look for any documentation or comments about distance units in GlobalParamRepository or related classes
echo -e "\n=== GlobalParamRepository javadoc and comments ==="
sed -n '1,50p' android/clientmanager/src/main/java/io/mosip/registration/clientmanager/repository/GlobalParamRepository.java | head -n 50Repository: mosip/android-registration-client
Length of output: 5873
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check GlobalParamRepositoryTest for test values that show the unit
echo "=== GlobalParamRepositoryTest.java content ==="
cat android/clientmanager/src/test/java/io/mosip/registration/clientmanager/repository/GlobalParamRepositoryTest.java 2>/dev/null | head -n 200 || echo "File not found or empty"
# Look for property files or config files with example distance values
echo -e "\n=== Property files with distance config ==="
find android/clientmanager -name "*.properties" -o -name "*.xml" 2>/dev/null | xargs grep -l "distance\|DIST_FRM" 2>/dev/null | head -n 20 || echo "No property files found"
# Search for any mock setup or test data that sets the distance
echo -e "\n=== Mock/test setup of distance value ==="
rg -n --hidden -S "DIST_FRM_MACHINE_TO_CENTER|getCachedStringMachineToCenterDistance" android/clientmanager/src/test -B 3 -A 3 2>/dev/null | head -n 100 || echo "No test references found"Repository: mosip/android-registration-client
Length of output: 15698
Confirm GPS behavior: should missing machine coords cause validation failure or be acceptable skip when GPS is enabled? Plus minor exception message improvement.
The code correctly assumes getCachedStringMachineToCenterDistance stores meters (verified in test: 0.1 km = 100 meters compared against "500" meters threshold), and getDistance returns kilometers as documented. The * 1000 conversion is correct.
However, two real issues remain:
-
GPS validation bypass: When GPS is enabled (
enableFlag == "Y"), missingmachineLongitude/machineLatitudecurrently skips validation entirely (lines 250–253). If the intent is to enforce location validation when GPS is enabled, this silently bypasses the check instead of failing. Verify whether this is acceptable fallback behavior or should reject the operation. -
Misleading exception message (line 313): The catch block logs
"Invalid center coordinates format", butNumberFormatExceptioncan also be thrown byDouble.parseDouble(maxDistanceStr)at line 296. Update the message to reflect both sources:"Invalid number format in center coordinates or max distance configuration".
🤖 Prompt for AI Agents
In
@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/SyncStatusValidatorServiceImpl.java
around lines 241 - 316, In validateCenterToMachineDistance, when
getCachedStringGpsDeviceEnableFlag() is "Y" do not silently skip if
machineLongitude or machineLatitude are null: replace the early return for null
machine coords with a thrown ClientCheckedException (use the same client error
pattern/context as other failures, e.g., new ClientCheckedException(context,
R.string.err_004)) so GPS-enabled mode enforces presence of device location;
also update the NumberFormatException log message in the catch block from
"Invalid center coordinates format" to "Invalid number format in center
coordinates or max distance configuration" and include the exception object in
the log to reflect that Double.parseDouble(maxDistanceStr) can fail.
1 participant
RCF-1354
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.