Implemented the "confirm visit" in visit service - #56
Conversation
|
Hi @axymthr sir please review and merge pull request. |
| visitRepository.save(visit); | ||
| return true; | ||
| } else { | ||
| return false; |
There was a problem hiding this comment.
Here's a flow we need to consider. Somehow this method needs to communicate to calling code whether all verification attempts failed, or we need to retry verification. Now that you're also diving into the Controller code, think about how this flow should work.
Let me know your thoughts. Is there a design flaw here, and we should approach it differently, or does our design support this?
There was a problem hiding this comment.
@anuragSharma1112 I think I have it, let me know your thoughts.
You have 2 options:
- Throw a checked exception (with metadata) in OTP service and catch it in the Controller and do further processing, OR
- Return a VerificationResult object. This object should contain:
- bool success
- bool reattempt
- String message (for failure or retry)
This should be simple enough yet still flexible. Check bool success in VisitService and return as is if false, else save into DB
What do you think?
| visitRepository.save(visit); | ||
| return true; | ||
| } else { | ||
| return false; |
There was a problem hiding this comment.
@anuragSharma1112 I think I have it, let me know your thoughts.
You have 2 options:
- Throw a checked exception (with metadata) in OTP service and catch it in the Controller and do further processing, OR
- Return a VerificationResult object. This object should contain:
- bool success
- bool reattempt
- String message (for failure or retry)
This should be simple enough yet still flexible. Check bool success in VisitService and return as is if false, else save into DB
What do you think?
|
Hi @axymthr sir i think option 2 good to go, and i am on it. |
|
Hi @axymthr sir, I've removed the markVisitAsVerified call from OtpService as per earlier feedback to ensure OTPService focuses strictly on OTP handling logic. The visit approval (visit.setIsApproved(true)) is now solely handled inside VisitService where it belongs. OtpService.validateOtp(...) now only returns a boolean — VisitService decides what to do with that outcome. We return a VerificationResult object containing success status, reattempt info, and message. This aligns with your suggestion to keep control flow clean and extensible. All state updates are encapsulated in VisitService, keeping OTPService stateless and isolated from business concerns. This design improves separation of concerns, makes the codebase more testable, and follows SRP. |
There was a problem hiding this comment.
Can we convert this to a record? It's just an immutable data holder.
| visit.setIsApproved(true); | ||
| visitRepository.save(visit); | ||
|
|
||
| return new VerificationResult(true, false, "OTP verified successfully"); |
There was a problem hiding this comment.
Good progress, but don't do this here. VerificationResult should be returned from OTPService.validateOtp(). We want to encapsulate all the logic of re-attempts there. So hasExceededOtpAttempts() doesn't need to be exposed outside, it's all internal. If we have to change no. of re-attempts, then it's just 1 config change inside OTPService and nothing changes here in VisitService.
Simply check the VerificationResult.success flag here.
|
Hi @axymthr sir, |
| } | ||
|
|
||
| boolean isValid = otpService.validateOtp(visit, otp); | ||
| VerificationResult result = otpService.validateOtp(visit, otp); |
There was a problem hiding this comment.
Are we calling validateOTP form here or from visitService? Only one of them should be kept.
| if (isValid) { | ||
| if (result.success()) { | ||
| otpService.markVisitAsVerified(visit); | ||
| return ResponseEntity.ok("<p class=\"text-green-600 font-bold\">OTP Verified Successfully!</p>"); |
There was a problem hiding this comment.
While you're changing and touching the UI, also move this HTML to a JTE fragment.
…Service-for-visitor-confirmation-task-53 # Conflicts: # web-backend/src/main/java/com/statusneo/vms/controller/VisitorController.java
|
Hi @axymthr I have some changes in visit Confirmation please review. |
| #else | ||
| <p style="color: red;">Failed to confirm visit: ${result.message()}</p> | ||
| #if(result.reattempt()) | ||
| <p>You can try again.</p> |
There was a problem hiding this comment.
Don't hard code the message here, it defeats the point of sending a verification result message.
If reattempt is allowed, the backend message should contain that.
If it's not allowed, it would contain that.
The issue for me is, how are they going to try again?
We need to provide a 'Resend OTP' option somehow.
Or should we auto-resent OTP in case of failure?
Think through the behavior before building the UI flow.
|
Hi @axymthr sir, i have fixed the resend otp issue and clean up the code, please review and merge. |
|
@anuragSharma1112 please rebase your branch from develop. Looks like there are merge conflicts. |
2 participants
Description
Implemented the "confirm visit" feature to validate OTP, update visit approval status, and ensure secure and verified check-ins for visitors.
Fixes # (issue)
NA
Type of change
How Has This Been Tested?
Checklist:
Screenshots (for UX changes):
NA
Additional Notes:
NA