adjustment as per new signup form in app#151
Conversation
|
Claude finished @vvshk's task in 4m 32s —— View job PR Review: adjustment as per new signup form in appI reviewed the diff against
Key findings (inline)🟠 Should address
🟡 Worth confirming / nice-to-have 👍 Looks good
Note: I reviewed for correctness/conventions only and did not run a linter or tests (this is a static frontend with no test suite in the repo). |
| const res = await fetch( | ||
| `${CONFIG.baseUrl}/client/checkMobile/${val}` | ||
| ); |
There was a problem hiding this comment.
Missing auth header + no res.ok check on the phone lookup.
Every other fetch in this codebase sends Authorization: Bearer <token>, and there's already an authenticated admin endpoint for mobile lookups — ${CONFIG.basePath}/card/by-mobile/${mobno} (see admin/wifi/permanentCodeRequests.js:408). This new call instead hits the client API (CONFIG.baseUrl, not basePath) with no token.
- If
/client/checkMobilerequires auth, this will 401. Because the code never checksres.okand only readsdata.exists, the admin will always see the misleading "Phone number is not registered!" message instead of an error. - Please confirm the endpoint is intentionally public. If it isn't, switch to the authenticated admin endpoint.
At minimum, guard on the response status:
| const res = await fetch( | |
| `${CONFIG.baseUrl}/client/checkMobile/${val}` | |
| ); | |
| const res = await fetch( | |
| `${CONFIG.baseUrl}/client/checkMobile/${val}` | |
| ); | |
| if (!res.ok) { | |
| nameEl.textContent = 'Error checking phone number'; | |
| nameEl.style.color = '#c62828'; | |
| return; | |
| } |
| const res = await fetch(`${CONFIG.baseUrl}/client/checkMobile/${val}`); | ||
| const data = await res.json(); |
There was a problem hiding this comment.
Same phone-lookup concern as in createCard.js: this hits the client API (CONFIG.baseUrl) with no Authorization header — a deviation from every other request here — and never checks res.ok, so an error response silently renders as "Phone number is not registered!". Please verify the endpoint is public; otherwise use the authenticated admin lookup. A minimal safety guard:
| const res = await fetch(`${CONFIG.baseUrl}/client/checkMobile/${val}`); | |
| const data = await res.json(); | |
| const res = await fetch(`${CONFIG.baseUrl}/client/checkMobile/${val}`); | |
| if (!res.ok) { | |
| nameEl.textContent = 'Error checking phone number'; | |
| nameEl.style.color = '#c62828'; | |
| return; | |
| } | |
| const data = await res.json(); |
| referencePhone: document.getElementById('referencePhone')?.value || null, | ||
| guestType: document.getElementById('guestType')?.value || null, | ||
| department: document.getElementById('department')?.value || null |
There was a problem hiding this comment.
Stale data: referencePhone / guestType / department are always submitted regardless of res_status.
These fields are hidden (not cleared) when the status changes, and populateForm fills them from the fetched record. So editing a card that was a GUEST/SEVA KUTIR and switching it to MUMUKSHU/PR will still send the old reference phone / guest type / department to the backend.
createCard.js handles this correctly by only attaching these keys inside the matching res_status block. Mirror that here so irrelevant fields are sent as null:
| referencePhone: document.getElementById('referencePhone')?.value || null, | |
| guestType: document.getElementById('guestType')?.value || null, | |
| department: document.getElementById('department')?.value || null | |
| referencePhone: | |
| document.getElementById('res_status').value === 'GUEST' | |
| ? document.getElementById('referencePhone')?.value || null | |
| : null, | |
| guestType: | |
| document.getElementById('res_status').value === 'GUEST' | |
| ? document.getElementById('guestType')?.value || null | |
| : null, | |
| department: | |
| document.getElementById('res_status').value === 'SEVA KUTIR' | |
| ? document.getElementById('department')?.value || null | |
| : null |
| refPhoneInput.addEventListener('input', async (e) => { | ||
| const val = e.target.value.trim(); | ||
| const nameEl = document.getElementById('refPhoneName'); | ||
| if (val.length === 10) { | ||
| nameEl.textContent = 'Checking...'; | ||
| nameEl.style.color = '#777'; | ||
| try { | ||
| const res = await fetch( | ||
| `${CONFIG.baseUrl}/client/checkMobile/${val}` | ||
| ); | ||
| const data = await res.json(); | ||
| if (data.exists) { | ||
| if (data.res_status === 'MUMUKSHU') { | ||
| nameEl.textContent = `Name: ${data.name} (Mumukshu)`; | ||
| nameEl.style.color = '#2e7d32'; | ||
| } else { | ||
| nameEl.textContent = `Name: ${data.name} (${data.res_status}) - Warning: Not a Mumukshu`; | ||
| nameEl.style.color = '#c62828'; | ||
| } | ||
| } else { | ||
| nameEl.textContent = 'Phone number is not registered!'; | ||
| nameEl.style.color = '#c62828'; | ||
| } | ||
| } catch (err) { | ||
| nameEl.textContent = 'Error checking phone number'; | ||
| nameEl.style.color = '#c62828'; | ||
| } | ||
| } else { | ||
| nameEl.textContent = ''; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Two smaller points on this listener:
- No debounce / out-of-order responses. A request fires on every keystroke once the value hits 10 chars (and again on each subsequent edit). Since responses are async, a slower earlier request can resolve after a later one and overwrite the label with stale data. Consider debouncing, or tracking the latest query and ignoring outdated responses.
- Duplication. This inline handler is a near-exact copy of
checkReferencePhone()inupdateCard.js. Extracting a shared helper (or reusing the same function name/logic) would keep the two forms in sync as this evolves.
Neither blocks the PR, but worth cleaning up while the code is fresh.
| placeholder="Auto-generated" | ||
| readonly |
There was a problem hiding this comment.
cardno is now readonly / auto-generated and lost its previous required + pattern="[0-9]{10}". Nothing in createCard.js populates this field, so the submit handler sends cardno: form.cardno.value as an empty string (createCard.js:231).
Please confirm the /card/create backend treats an empty cardno as "generate one". If it still validates the format, "" will be rejected; if it auto-generates, consider dropping cardno from the request payload entirely to make the intent explicit.
No description provided.