feat: Implement credit card application feature with validation and error handling#31
feat: Implement credit card application feature with validation and error handling#31yortch wants to merge 4 commits into
Conversation
yortch
commented
Feb 26, 2026
yortch
commented
- Added GlobalExceptionHandler for centralized exception handling.
- Created CreditCardApplicationController for handling application submissions and retrievals.
- Developed CreditCardApplicationRequest and CreditCardApplicationResponse DTOs for data transfer.
- Implemented CreditCardApplication entity with JPA annotations for persistence.
- Created CreditCardApplicationRepository for database interactions.
- Developed CreditCardApplicationService for business logic related to applications.
- Added tests for CreditCardApplicationController and CreditCardApplicationRepository.
- Implemented CardApplicationPage in the frontend for user interaction with the application process.
Co-authored-by: yortch <4576246+yortch@users.noreply.github.com>
…rror handling - Added GlobalExceptionHandler for centralized exception handling. - Created CreditCardApplicationController for handling application submissions and retrievals. - Developed CreditCardApplicationRequest and CreditCardApplicationResponse DTOs for data transfer. - Implemented CreditCardApplication entity with JPA annotations for persistence. - Created CreditCardApplicationRepository for database interactions. - Developed CreditCardApplicationService for business logic related to applications. - Added tests for CreditCardApplicationController and CreditCardApplicationRepository. - Implemented CardApplicationPage in the frontend for user interaction with the application process.
There was a problem hiding this comment.
Pull request overview
Implements an end-to-end “credit card application” feature, adding backend persistence + REST endpoints and a new React application flow so users can submit and track applications.
Changes:
- Added backend application domain (entity/repository/service/controller) plus centralized exception handling.
- Added backend tests for the new controller and repository.
- Added a multi-step frontend application page and introduced Copilot instruction files for Java/React.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/pages/CardApplicationPage.jsx | New multi-step application UI that fetches card data and submits applications |
| backend/src/main/java/com/threeriversbank/config/GlobalExceptionHandler.java | Centralized validation and runtime exception handling for REST APIs |
| backend/src/main/java/com/threeriversbank/controller/CreditCardApplicationController.java | New REST endpoints for submitting and retrieving applications |
| backend/src/main/java/com/threeriversbank/service/CreditCardApplicationService.java | Business logic for creating applications and mapping entities to DTOs |
| backend/src/main/java/com/threeriversbank/repository/CreditCardApplicationRepository.java | Spring Data repository for application queries |
| backend/src/main/java/com/threeriversbank/model/entity/CreditCardApplication.java | JPA entity for persisted application records |
| backend/src/main/java/com/threeriversbank/model/dto/CreditCardApplicationRequest.java | Request DTO with bean validation constraints |
| backend/src/main/java/com/threeriversbank/model/dto/CreditCardApplicationResponse.java | Response DTO returned to clients after submission/lookup |
| backend/src/test/java/com/threeriversbank/repository/CreditCardApplicationRepositoryTest.java | JPA tests for repository query methods |
| backend/src/test/java/com/threeriversbank/controller/CreditCardApplicationControllerTest.java | MVC tests for request validation and basic controller behavior |
| .github/instructions/reactjs.instructions.md | Adds repo guidance for React development standards |
| .github/instructions/java.instructions.md | Adds repo guidance for Java development standards |
| log.info("Submitting credit card application for card id: {} by {}", | ||
| request.getCardId(), request.getEmail()); |
There was a problem hiding this comment.
submitApplication logs the applicant email address at INFO level (... by {email}). Since this is PII, consider removing it, masking it, or lowering to DEBUG to reduce exposure in logs.
| log.info("Submitting credit card application for card id: {} by {}", | |
| request.getCardId(), request.getEmail()); | |
| log.info("Submitting credit card application for card id: {}", request.getCardId()); |
| import { creditCardService, applicationService } from '../services/api'; | ||
|
|
There was a problem hiding this comment.
applicationService is imported from ../services/api, but frontend/src/services/api.js currently only exports creditCardService (and a default axios instance). This will cause a build/runtime error when applicationService.submitApplication is referenced. Export an applicationService from api.js (e.g., wrapping POST /applications) or update this import to the actual module that provides it.
| import { creditCardService, applicationService } from '../services/api'; | |
| import apiClient, { creditCardService } from '../services/api'; | |
| const applicationService = { | |
| submitApplication: (applicationData) => apiClient.post('/applications', applicationData), | |
| }; |
| border: selectedCardId === card.id ? '2px solid' : '2px solid transparent', | ||
| borderColor: selectedCardId === card.id ? 'secondary.main' : 'transparent', | ||
| }} | ||
| onClick={() => setSelectedCardId(card.id)} |
There was a problem hiding this comment.
selectedCardId is initialized from the route/query param (string) but later compared to card.id (number) using strict equality. This prevents the selected-card styling from applying reliably. Normalize the ID type (all strings or all numbers) and adjust comparisons/query keys accordingly.
| border: selectedCardId === card.id ? '2px solid' : '2px solid transparent', | |
| borderColor: selectedCardId === card.id ? 'secondary.main' : 'transparent', | |
| }} | |
| onClick={() => setSelectedCardId(card.id)} | |
| border: selectedCardId === String(card.id) ? '2px solid' : '2px solid transparent', | |
| borderColor: selectedCardId === String(card.id) ? 'secondary.main' : 'transparent', | |
| }} | |
| onClick={() => setSelectedCardId(String(card.id))} |
| if (!formData.businessName) errors.businessName = 'Business name is required'; | ||
| if (!formData.annualRevenue) errors.annualRevenue = 'Annual revenue is required'; | ||
| if (formData.annualRevenue && isNaN(formData.annualRevenue)) errors.annualRevenue = 'Must be a number'; | ||
| if (!formData.yearsInBusiness) errors.yearsInBusiness = 'Years in business is required'; |
There was a problem hiding this comment.
Step 1 validation treats whitespace as a valid annualRevenue value (!formData.annualRevenue is false for ' ' and isNaN(' ') is also false), which can let users progress and then submit NaN (via parseFloat). Consider trimming input before required/numeric checks and validating with something like Number.isFinite(Number(value)).
| if (!formData.businessName) errors.businessName = 'Business name is required'; | |
| if (!formData.annualRevenue) errors.annualRevenue = 'Annual revenue is required'; | |
| if (formData.annualRevenue && isNaN(formData.annualRevenue)) errors.annualRevenue = 'Must be a number'; | |
| if (!formData.yearsInBusiness) errors.yearsInBusiness = 'Years in business is required'; | |
| const businessNameTrimmed = formData.businessName ? formData.businessName.trim() : ''; | |
| const annualRevenueTrimmed = String(formData.annualRevenue ?? '').trim(); | |
| const annualRevenueNumber = annualRevenueTrimmed === '' ? NaN : Number(annualRevenueTrimmed); | |
| const yearsInBusinessValue = | |
| typeof formData.yearsInBusiness === 'string' | |
| ? formData.yearsInBusiness.trim() | |
| : formData.yearsInBusiness; | |
| if (!businessNameTrimmed) errors.businessName = 'Business name is required'; | |
| if (!annualRevenueTrimmed) { | |
| errors.annualRevenue = 'Annual revenue is required'; | |
| } else if (!Number.isFinite(annualRevenueNumber)) { | |
| errors.annualRevenue = 'Must be a number'; | |
| } | |
| if (!yearsInBusinessValue) errors.yearsInBusiness = 'Years in business is required'; |
| @Column(length = 36) | ||
| private String referenceNumber; | ||
|
|
||
| @Column(nullable = false) | ||
| private LocalDateTime submittedAt; | ||
|
|
||
| @PrePersist | ||
| protected void onCreate() { | ||
| submittedAt = LocalDateTime.now(); | ||
| if (status == null) { | ||
| status = "PENDING"; | ||
| } | ||
| if (referenceNumber == null) { | ||
| referenceNumber = "TRB-" + System.currentTimeMillis(); | ||
| } |
There was a problem hiding this comment.
referenceNumber is used as a public lookup key (findByReferenceNumber returns Optional), but it is generated from System.currentTimeMillis() and has no uniqueness constraint. Under concurrent inserts in the same millisecond this can collide and break lookups (or cause non-unique results). Generate a collision-resistant value (e.g., UUID) and enforce uniqueness at the DB level (@Column(nullable=false, unique=true, ...)).
| description = "Look up all applications for a given email address") | ||
| public ResponseEntity<List<CreditCardApplicationResponse>> getApplicationsByEmail( | ||
| @RequestParam String email) { | ||
| log.info("GET /api/applications?email={}", email); |
There was a problem hiding this comment.
This endpoint logs the applicant email address at INFO level (GET /api/applications?email=...). Email addresses are PII and typically shouldn't be logged in plaintext at INFO. Consider removing it, masking it, or logging at DEBUG with appropriate redaction.
| log.info("GET /api/applications?email={}", email); | |
| log.info("GET /api/applications - fetching applications by email"); |
| @ExceptionHandler(RuntimeException.class) | ||
| public ResponseEntity<Map<String, Object>> handleRuntimeException(RuntimeException ex) { | ||
| Map<String, Object> response = new HashMap<>(); | ||
| response.put("error", ex.getMessage()); | ||
| return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(response); | ||
| } |
There was a problem hiding this comment.
The global RuntimeException handler returns the exception message directly to clients with HTTP 500 and does not log the exception. This can leak internal details and makes it hard to diagnose server-side failures. Prefer logging the exception (at least with stack trace) and returning a generic message for 500s; also consider handling "not found" as 404 rather than via the RuntimeException handler.
| @Test | ||
| void submitApplication_WithMissingFields_ShouldReturnBadRequest() throws Exception { | ||
| CreditCardApplicationRequest request = CreditCardApplicationRequest.builder() | ||
| .cardId(1L) | ||
| .build(); | ||
|
|
||
| mockMvc.perform(post("/api/applications") | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .content(objectMapper.writeValueAsString(request))) | ||
| .andExpect(status().isBadRequest()); | ||
| } |
There was a problem hiding this comment.
The controller validation tests only assert 400 Bad Request for invalid payloads, but the frontend relies on a fieldErrors map in the response body (from GlobalExceptionHandler) to display per-field errors. Add an assertion that the response JSON includes fieldErrors entries for missing/invalid fields to lock in this API contract.
| @Column(nullable = false, length = 200) | ||
| private String businessName; | ||
|
|
||
| @Column(precision = 15, scale = 2) |
There was a problem hiding this comment.
annualRevenue is required by the request DTO, but the entity column definition allows null (@Column(precision=15, scale=2) defaults to nullable). This can allow invalid rows if the entity is persisted outside the validated controller path. Align persistence constraints with the API contract by marking the column nullable = false (and consider validation at the entity layer if used elsewhere).
| @Column(precision = 15, scale = 2) | |
| @Column(nullable = false, precision = 15, scale = 2) |
| CreditCard card = creditCardRepository.findById(request.getCardId()) | ||
| .orElseThrow(() -> new RuntimeException("Credit card not found with id: " + request.getCardId())); | ||
|
|
There was a problem hiding this comment.
This service throws RuntimeException for not-found conditions (missing card / missing application). With the current GlobalExceptionHandler, these end up as HTTP 500, which misrepresents client-visible "not found" scenarios. Use a 404 mapping (e.g., ResponseStatusException(HttpStatus.NOT_FOUND, ...) or a dedicated NotFound exception handled as 404) so API consumers can distinguish not-found from server errors.
