Entra Follow Ups PR Review - edmozley/freeitsm GitHub Wiki

Entra follow-ups - PR review

PRs #237, #238, #239, #240 by vsc55 · Design #234 · Verdict: merge with small fixes

Four follow-ups to the Entra import, stacked so each builds on the one before. They were first written before 4.7.0 and carried commits already on main; after a first review they were rebased onto current main, and two were reworked as asked. All four were merged together, under their author's name, with a fix commit after (changelog #2747).

What each one does

PR	Change	Changelog
#237	Directory groups in People: a Group filter, a Groups tab, a page per group, groups on a person's and a company's page	#2691, #2692
#238	A person an import keeps up to date cannot be deleted by hand, and the run history says who joined and left each group	#2693, #2694
#239	Each directory page says when the built-in scheduler imports it	#2695
#240	The import script moves to `cron/`, with the old path kept as a pointer	#2696

What the first review asked, and what came back

Asked	Came back
Rebase each onto current `main`, holding only its own change	Done, as a stack (#237 → #240) so they merge without conflicts
#239 added its own way to start imports without a scheduled task. 4.8.0's built-in scheduler already does that - use it, or say what it covers that the scheduler doesn't	Reworked. The background tick, spawner and per-source interval are gone. The only thing the scheduler lacked was a different pace per source, judged not worth two overlapping schedules. What is left is a line on each page that reads the scheduler
#238 must leave a way through for the planned Remove imported people tool	The block is now explicitly the rule for deleting one person by hand, which an undo tool does not ask; and it lifts once the source is deleted

What the review checked

Company scoping in #237 - every new query limits groups by their provider's company and members by the reader's companies; a group out of reach filters the list to nobody. The new test proves it with two companies.
"Deletable once the source is deleted" in #238 - true: users.auth_provider_id has ON DELETE SET NULL (fk_users_auth_provider), and the rule ignores is_managed when the provider is gone.
The action column for the new history entries - VARCHAR(16), so group_leave fits.
The daily time in #239 is labelled with the install's timezone, the same one the scheduler computes it in.
Nothing still spawns the old path after #240; the moved script refuses web requests itself; the pointer runs the same script with the same arguments and exit codes.
PHP 7.4 parse of every directly-run file (the PHP floor check).

Run against a throwaway install with main merged in: the three new tests (18, 15 and 19 checks), the scheduler (38), job health (35), the organisation chart (89), Entra identity (17), identity reuse (14), and every changed page loading without a PHP error. CI passed on the merge.

What was fixed after the merge

Finding	Fix
The People header counted the members of every group, on every People page, only to decide whether to show the Groups tab - slow on a tenant with thousands of groups	`peopleHasGroups()`, one `LIMIT 1` row
`tests/people-scope.php` failed once groups existed: it expected an exact list of sections on a company page, and People's own Groups card is now one. Not seen before because the author's test database had no groups	The test allows the Groups card
About 50 new strings were English only; one existing string's English had changed in place, leaving its translations stale	German and Spanish
The help linked to a wiki page, Directory-Sync-Scheduling, that was never published (already broken on `main`)	Links to Scheduled tasks
`CHANGELOG.local.md` conflicted with a row added the same day	Both kept

What was left

Behaviour change on upgrade: imported people can no longer be deleted by hand in Tickets → Users. The button says why.
No per-source schedule - deliberately; every directory source is imported by the one scheduler job.
The known limit from #235 is still open: a reused email or sign-in name leaves the new starter out, with no screen to separate the two.