Skip to content

Add History Logging for Family Merge and Detach Operations - #790

Open
vahidbazzaz wants to merge 8 commits into
masterfrom
feat/beneficiary-family-history-logging
Open

Add History Logging for Family Merge and Detach Operations#790
vahidbazzaz wants to merge 8 commits into
masterfrom
feat/beneficiary-family-history-logging

Conversation

@vahidbazzaz

Copy link
Copy Markdown
Contributor

This PR adds comprehensive history logging for family merge and detach operations in the beneficiary management system. Previously, when families were merged or detached (either via action buttons or drag & drop), no audit trail was created in the history table. This made it impossible to investigate cases where beneficiaries (including small children) were unexpectedly deactivated or moved between families.

Changes Made

1. In Manage Beneficiaries add history log if beneficiary is added to a family via drag & drop

  • File: include/people.php:503
  • Added history logging when beneficiary is added to a family via drag & drop
  • Log format: added to family via drag & drop

2. In Manage Beneficiaries add history log if beneficiary is removed from a family via drag & drop

  • File: include/people.php:506
  • Added history logging when beneficiary is removed from a family via drag & drop
  • Log format: removed from family via drag & drop

3. In Manage Beneficiaries add history log if beneficiaries are merged to a family via selection and action button

  • File: include/people.php:442
  • Added history logging when beneficiaries are merged into a family via action button
  • Log format: merged to family (head: [id])

4. In Manage Beneficiaries add history log if beneficiaries are detached from a family via selection and action button

  • File: include/people.php:468
  • Added history logging when beneficiaries are detached from a family via action button
  • Log format: detached from family

5. Drag & Drop Implementation Details

  • Files: library/lib/list.php:58-94
  • Modified listBulkMove() to track parent_id changes during drag & drop operations
  • Returns parent change information to enable history logging

6. Cypress Tests

  • Files: cypress/support/database.js, cypress/e2e/1_feature_tests/5_3_Manage_Beneficiaries.js
  • Added checkHistoryLog() command to verify history entries exist in database
  • Added getBeneficiaryIdFromRow() command to extract beneficiary IDs from DOM
  • Created test helper endpoint library/ajax/testhistorycheck.php for test verification
  • Enhanced existing merge/detach tests to verify history logging works correctly

Testing

✅ All Cypress tests passing:

  • Merge beneficiaries: Verifies history log created
  • Detach beneficiaries: Verifies history log created
  • Existing tests: All still passing

@vahidbazzaz

Copy link
Copy Markdown
Contributor Author

@vahidbazzaz
vahidbazzaz requested a review from HaGuesto November 20, 2025 15:25

@HaGuesto HaGuesto left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Vahid I tested this a bit more in depth and would say we need another iteration:

from the functional test:

  1. the lastModified date in the people table is somehow not updated could you maybe adapt the definition of the simpleBulkSaveChangeHistory to do this too?

from the code review

  • since we do not show the history messages anywhere, I'm happy how they look like atm, but it would be great if you could move the from and to values out of the change message into the right bits of simpleBulkSaveChangeHistory($table, $records, $changes, $from = [], $to = [])
  • maybe put "parent_id;" in front in the change message to make clear that the parent_id in the record was changed.

I left comments in the code to make it clearer

Comment thread include/people.php Outdated
}
}
});
simpleBulkSaveChangeHistory('people', $ids, 'merged to family (head: '.$oldest.')');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should maybe be simpleBulkSaveChangeHistory('people', $ids, 'parent_id; merged to family', [], ['int'=> <parent_id>);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 671650c — now simpleBulkSaveChangeHistory('people', $ids, 'parent_id; merged to family', [], ['int' => $oldest]), so the new family head goes into the to column and the message is prefixed with parent_id;.

Comment thread include/people.php Outdated
db_query('UPDATE people SET parent_id = NULL WHERE id = :id', ['id' => $id]);
}
});
simpleBulkSaveChangeHistory('people', $ids, 'detached from family');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should maybe be simpleBulkSaveChangeHistory('people', $ids, 'parent_id; detached from family', ['int'=> <parent_id>, []);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 671650c, with one adjustment to your suggestion: instead of a single call with one from value, the beneficiaries are grouped by their previous parent_id and one simpleBulkSaveChangeHistory('people', $beneficiaryIds, 'parent_id; detached from family', ['int' => $parentId], []) call is made per family. A multi-select detach can span more than one family, so a single shared from would mis-attribute the old family for anyone not in the first group. Same grouping was applied to the drag & drop add/remove paths.

- Move parent_id values out of the change message into the from/to
  columns of simpleBulkSaveChangeHistory, grouping beneficiaries by
  their old/new parent so multi-family batches log correctly
- Prefix history messages with 'parent_id;' to name the changed column
- Update modified/modified_by on affected records after history insert,
  guarded by db_fieldexists so it no-ops on tables without the column
- Update Cypress assertions to expect the new message text
@vahidbazzaz

Copy link
Copy Markdown
Contributor Author

Thanks @HaGuesto — addressed all three points in 671650c:

  1. lastModified not updatingsimpleBulkSaveChangeHistory now runs an UPDATE {$table} SET modified = NOW(), modified_by = :user WHERE id IN (...) after the history insert, guarded by db_fieldexists($table, 'modified') so it's a no-op on tables without that column (e.g. qr). Note this also means the other callers (pdf/qr.php on stock, library/lib/list.php bulk delete/restore) now bump modified/modified_by too — this seemed like correct behavior everywhere rather than a special case, but happy to scope it down with a flag if you'd prefer.
  2. from/to out of the message — parent ids now go into the from/to int columns instead of the change string. For detach and drag & drop I group beneficiaries by their old/new parent_id and emit one call per family, since a multi-select batch can span more than one family.
  3. parent_id; prefix — added to all family history messages.

Verified locally: 5_3_Manage_Beneficiaries.js passes (merge + detach history assertions), and people.modified/modified_by update as expected.

…mily-history-logging

# Conflicts:
#	library/lib/tools.php
The merge case passed all selected ids (including the oldest/head) to
simpleBulkSaveChangeHistory, so the head got a 'parent_id; merged to
family' entry with to_int pointing at its own id, even though its
parent_id never changed. Filter out the head so only members whose
parent_id actually changed are logged.

Add a checkHistoryLogAbsent Cypress command and assert the head is not
logged while the merged member is, locking in the fix.
@vahidbazzaz

Copy link
Copy Markdown
Contributor Author

Follow-up correctness fix in d5f5d3d: while verifying the history rows I noticed the merge path was logging the family head as well as the members. Because the head's parent_id doesn't change during a merge, it was getting a parent_id; merged to family row with to_int pointing at its own id (a self-merge). The merge call now filters out the head (array_filter($ids, fn ($id) => $id != $oldest)), so only members whose parent_id actually changed are logged.

Added a checkHistoryLogAbsent Cypress command and updated the merge test to assert the head is not logged while the merged member is. Full spec passes (5/5).

@vahidbazzaz vahidbazzaz self-assigned this Jul 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants