feat: add server-side movement audit timestamps

[rzueger]

Move createdBy tracking from frontend saga to a Cloud Function that sets createdBy, createdByName, createdAt, updatedBy, updatedByName, and updatedAt using server timestamps and user lookup. Display audit info to admins in MovementDetails.

[rzueger]

@claude review the pr

[claude]

Claude finished @rzueger's task in 1m 43s —— View job

---

PR Review: feat/movement-audit-timestamps

• Gather context
• Read changed files
• Analyze implementation
• Post review feedback

---

Overall this is a clean, well-structured implementation. Moving audit tracking server-side is the right call. Here are my findings:

Observations

functions/movementAudit.js

1. Export naming vs trigger type mismatch (movementAudit.js:95-103): auditDepartureOnUpdate / auditArrivalOnUpdate use onWrite triggers (not onUpdate). onWrite fires on create, update, and delete. The delete/create cases are handled correctly via the !change.before.exists() || !change.after.exists() guard, but the export names are slightly misleading. Consider renaming to auditDepartureOnWrite / auditArrivalOnWrite for clarity, or switch to onUpdate (which only fires on actual updates and won't need the before/after existence checks).

2. JSON.stringify comparison is key-order-sensitive (movementAudit.js:63): If Firebase RTDB ever returns an object's properties in a different order before vs. after a write (which can happen in practice), JSON.stringify(before) === JSON.stringify(after) will return false even when the data hasn't changed, triggering an unnecessary updatedAt write. A more robust comparison would sort keys or use a deep-equal utility. In practice this is probably fine given RTDB's consistent ordering, but worth flagging.

3. Audit DetailsBox hidden when user not found (MovementDetails.tsx:130): The audit box only renders when props.data.createdBy is truthy. If the user record wasn't found at creation time, createdBy is never set (only createdAt gets written — see handleCreate lines 35-37). An admin would see no audit section at all, not even the timestamp. If createdAt is considered important, you could either always set a placeholder createdBy value or change the UI condition to props.isAdmin && (props.data.createdBy || props.data.createdAt).

4. negativeTimestamp === 0 edge case (movementAudit.js:43): if (user.email && movement.negativeTimestamp) will skip createdBy_orderKey if negativeTimestamp is 0 (falsy). This shouldn't happen in practice but could be made explicit: movement.negativeTimestamp !== undefined && movement.negativeTimestamp !== null.

functions/movementAudit.spec.js

5. Loose mock setup for the ref chain (movementAudit.spec.js:55-63): buildRefChain returns the same mockChild result object for both ref('users').child(uid) (user lookup) and ref('departures').child(key) (movement update). The once mock is only needed for user lookup and update only for the movement write — but both are available on all calls. This works due to call ordering, but tests would be more robust with separate mocks for each db path.

6. Missing test: createdBy set to null when user has no email — There's a test for "user not found" (line 177) but no test for "user found but email is undefined/empty". user.email || null would write null to the DB in that case.

Minor nits

• functions/movementAudit.js:4: Long single line for AUDIT_FIELDS — could be broken across lines for readability.
• The movementType === 'departure' ? 'departures' : 'arrivals' ternary (movementAudit.js:47,79) is repeated in both handleCreate and handleUpdate. Could be extracted to a small helper.

What's good

• The infinite loop prevention via stripAuditFields before comparing is correct and well-tested.
• Removing the frontend saga createdBy logic (sagas.ts) is clean — the diff is minimal and the test cleanup matches.
• ServerValue.TIMESTAMP is the correct approach for server-side timestamps.
• authType !== 'USER' guard correctly prevents admin SDK writes from triggering audit updates.
• Test coverage is thorough for the main paths.

[rzueger]

@claude address review comments. decide by yourself what needs to be addressed and what doesnt. amend changes to the existing commit, no extra commit.

[claude]

Claude finished @rzueger's task in 4m 41s —— View job

---

Claude finished @rzueger's task —

---

Addressing Review Comments

Addressed:

• Fix export naming: auditDepartureOnUpdate → auditDepartureOnWrite, auditArrivalOnUpdate → auditArrivalOnWrite (in both movementAudit.js and index.js)
• Fix MovementDetails: show audit section when createdAt exists even without createdBy — condition changed to props.isAdmin && (props.data.createdBy || props.data.createdAt)
• Add missing test: user found but email is undefined → createdBy set to null
• Extract repeated movementType ternary to getCollectionPath() helper

Skipped (with reasoning):

• JSON.stringify key-order sensitivity — RTDB is consistent in property ordering in practice; not worth the added complexity
• negativeTimestamp === 0 edge case — impossible for real movements (always a large negative number)
• Loose mock setup in tests — works correctly and testing behavior is verified; refactoring adds no functional benefit

⚠️ Force push required: All changes are in the amended commit (85952a1) locally, but the action's git safety system blocks force pushes. Please run:

git push --force-with-lease origin feat/movement-audit-timestamps

[rzueger]

@claude I believe a user doesnt have a name in the auth context. I believe there is only uid (always) and sometimes email too. Check if I am right and adjust where needed, or explain why I am wrong if I am wrong. you can push another commit to the branch, no amend needed.

[claude]

Claude finished @rzueger's task in 1m 30s —— View job

---

Investigation: Firebase auth context user properties

• Gather context
• Read movementAudit.js to check auth context usage
• Check auth context properties used
• Post findings

---

You are correct that context.auth in Firebase Functions only has uid (always) and sometimes email (via context.auth.token.email). There is no reliable name field on the auth context.

However, no code changes are needed. The current implementation already handles this correctly:

• movementAudit.js only reads context.auth.uid from the auth context (lines 35 and 70)
• It then does a separate RTDB lookup via getUserDetails(uid) to fetch user.email, user.firstname, and user.lastname from the users/{uid} database node
• Email and name are never read from context.auth — they always come from RTDB

The code is correct as-is. The firstname/lastname fields it uses are properties of your RTDB user records (confirmed by syncUsers and the auth saga), not from the Firebase Auth JWT token.