diff --git a/docs/adr/0038-a-group-grants-it-does-not-copy.md b/docs/adr/0038-a-group-grants-it-does-not-copy.md index 9c216b0cb2..964ef6fe88 100644 --- a/docs/adr/0038-a-group-grants-it-does-not-copy.md +++ b/docs/adr/0038-a-group-grants-it-does-not-copy.md @@ -1351,7 +1351,11 @@ and the third is a deliberate departure from the units table. default and `Group::appendSnapinSequence()` numbers them afterward -- the same mechanism, for the same reason, as its host-side twin. -- **The imperative controls are marked deprecated, not removed.** Decision 10 +- **The imperative controls are removed.** *(Updated: the deprecation shipped + in #1647 and the removal in unit F, once fog-plugins #36 gave `location` and + `ou` the same mass-edit seam the core fields use. Seven controls went, not + six -- the group Active Directory card pushed to member hosts too and was + missed when the notices were added.)* Decision 10 says the order is: mass edit ships, the imperative tabs are marked deprecated in the UI and the docs, they are removed in a later release. The units table entry for unit E said "removal of the imperative tabs", which diff --git a/docs/development/group-split.md b/docs/development/group-split.md index a9ae895d62..96ed8faa87 100644 --- a/docs/development/group-split.md +++ b/docs/development/group-split.md @@ -765,6 +765,7 @@ serialize through conflicts anyway. | D3 | Group list "grants" column | `Router/Route.php`, `Pages/GroupManagement.php`, JS | D1 | **merged** (#1643) | | E | Group page rework: `Group::addSnapin`/`addPrinter`/`addModule` become grants, the tri-state coverage machinery goes, the imperative controls are marked deprecated | `Items/Group*Association.php`, `Items/Group.php`, `Pages/GroupManagement.php`, JS | **C5 proven** (Decision 10), D, M5 | **merged** (#1647) | | E1 | UNKNOWN-4's server half simulated against the real endpoint: revoking a grant removes only the granted printers | `tests/printer-grants-reach-the-client.test.php` | E | **merged** (#1648) | +| F | Decision 10's last step: the group page's imperative controls REMOVED, after fog-plugins #36 gave `location` and `ou` the same seam | `Pages/GroupManagement.php`, `fog.group.edit.js`, fog-plugins | E, and fog-plugins #36 | **merged** (#PENDING) | | E2 | `groups.groupOrder` made settable -- step 404 added the column and the resolver read it, but nothing could ever write it | `Items/Group.php`, `Pages/GroupManagement.php` | E | **merged** (#1649) | **Two bugs in C5's own surface were found and fixed while D was landing**, diff --git a/docs/plugin-development.md b/docs/plugin-development.md index 5c9c56a67e..3cb780bb6d 100644 --- a/docs/plugin-development.md +++ b/docs/plugin-development.md @@ -1085,7 +1085,9 @@ neither covers: changing a value across a set. `HOST_MASSEDIT_*` is that seam It is why `location` and `ou` each shipped a whole second hook file whose only job was to set one value across a group's members — and those files always -clobbered, because there was no way to say *leave this host alone*. +clobbered, because there was no way to say *leave this host alone*. Both were +converted to this seam in fog-plugins #36 and are the worked example to copy: +`AddLocationHost::massEditFields()` / `massEditApply()`. **Contribute a field** — fired when the Mass Edit form is built, and again when it is applied, with the same arguments both times so the two can never diff --git a/docs/release/1.6.0-release-notes.DRAFT.md b/docs/release/1.6.0-release-notes.DRAFT.md index 5964933bdb..61e31db068 100644 --- a/docs/release/1.6.0-release-notes.DRAFT.md +++ b/docs/release/1.6.0-release-notes.DRAFT.md @@ -310,18 +310,24 @@ pre-upgrade *dump* — it is not a schema rollback. in that order. **Default printer** is one of the granted printers, and a host that has chosen its own default keeps it. -- **The group page's push-to-all controls are deprecated and now say so.** - Setting an image, kernel, product key, AD details, printer level, screen - resolution, auto-logout or hostname enforcement from a group applies the - value **once**, to whichever hosts are members at that moment. A host added - afterward does not get it; a host removed keeps it. That has always been - true and was never visible. Each of those cards now carries a notice saying - so and pointing at **Edit selected hosts** on the Hosts list, which does the - same job over any selection and can be repeated. They still work exactly as - before in this release, and are removed in a later one. +- **The group page's push-to-all controls are gone.** Setting an image, + kernel, kernel arguments, init, primary disk, product key, BIOS/EFI exit + type, AD details, printer level, screen resolution, auto-logout or hostname + enforcement from a group applied the value **once**, to whichever hosts were + members at that moment. A host added afterward did not get it; a host + removed kept it. That was always true and was never visible. + + Every one of those settings is now edited from **Edit selected hosts** on + the Hosts list, which does the same job over any selection you choose, can + be repeated, and can say *leave this host alone* -- a state the group cards + had no way to express. Nothing is lost: the mass edit covers all of them. + + The `location` and `ou` plugins each shipped a group tab doing the same + thing, with the same two defects. Both now contribute to the host mass edit + instead, so **Location** and **OU** appear there beside the core fields. Power Management is not affected -- it creates tasks rather than copying a - value. + value, so it stays on the group page. - **The host list shows which groups a host is in, and you can search on them.** A new Groups column carries one chip per group, each a link to that diff --git a/packages/web/management/js/fog/group/fog.group.edit.js b/packages/web/management/js/fog/group/fog.group.edit.js index 7f36923ee6..1a5f31a178 100644 --- a/packages/web/management/js/fog/group/fog.group.edit.js +++ b/packages/web/management/js/fog/group/fog.group.edit.js @@ -89,32 +89,6 @@ }); }); - // --------------------------------------------------------------- - // IMAGE TAB - var groupImageUpdateBtn = $('#group-image-send'); - - function disableImageButtons(disable) { - groupImageUpdateBtn.prop('disabled', disable); - } - - groupImageUpdateBtn.on('click', function(e) { - e.preventDefault(); - disableImageButtons(true); - var method = $(this).attr('method'), - action = $(this).attr('action'), - image = $('#image option:selected').val(), - opts = { - confirmimage: 1, - image: image - }; - $.apiCall(method,action,opts,function(err) { - disableImageButtons(false); - if (err) { - return; - } - }); - }); - // --------------------------------------------------------------- // TASKS TAB var taskItem = $('.taskitem'), @@ -302,26 +276,6 @@ }); }); - // Config area - var groupPrinterConfigBtn = $('#printer-config-send'); - - groupPrinterConfigBtn.on('click', function(e) { - e.preventDefault(); - groupPrinterConfigBtn.prop('disabled', true); - var method = $(this).attr('method'), - action = $(this).attr('action'), - opts = { - confirmlevelup: 1, - level: $('.checked input[name="level"]').val() - }; - $.apiCall(method,action,opts,function(err) { - groupPrinterConfigBtn.prop('disabled', false); - if (err) { - return; - } - }); - }); - // --------------------------------------------------------------- // SNAPINS TAB // Association area @@ -459,226 +413,6 @@ ] }); - // Display manager area - var groupModuleDisplaymanBtn = $('#group-displayman-send'), - groupModuleDisplayForm = $('#group-displayman-form'); - - function disableModuleDisplayButtons(disable) { - groupModuleDisplaymanBtn.prop('disabled', disable); - } - - groupModuleDisplayForm.on('submit', function(e) { - e.preventDefault(); - }); - - groupModuleDisplaymanBtn.on('click', function(e) { - e.preventDefault(); - var method = $(this).attr('method'), - action = $(this).attr('action'), - opts = { - confirmdisplaysend: 1, - x: $('#x').val(), - y: $('#y').val(), - r: $('#r').val() - }; - disableModuleDisplayButtons(true); - $.apiCall(method,action,opts,function(err) { - disableModuleDisplayButtons(false); - }); - }); - - // Auto log out area - var groupModuleAloBtn = $('#group-alo-send'), - groupModuleAloForm = $('#group-alo-form'); - - function disableModuleAloButtons(disable) { - groupModuleAloBtn.prop('disabled', disable); - } - - groupModuleAloBtn.on('click', function(e) { - e.preventDefault(); - disableModuleAloButtons(true); - var method = $(this).attr('method'), - action = $(this).attr('action'), - opts = { - confirmalosend: 1, - tme: $('#tme').val() - }; - $.apiCall(method,action,opts,function(err) { - disableModuleAloButtons(false); - // Blank is a no-op server-side (no-clobber), and a failure left the - // members alone -- in neither case did anything move. - var raw = $.trim(String(opts.tme)); - if (err || raw === '') { - return; - } - // Mirror groupModulePost(): (int) the value, and anything below the - // minimum saves as 0 (disabled). The threshold rides in on - // data-alo-min from GroupManagement::ALO_MIN_MINUTES rather than - // being repeated here, so the two cannot disagree. - var hint = $('#alo-shared-hint'), - mins = parseInt(raw, 10); - if (isNaN(mins) || mins < parseInt(hint.data('alo-min'), 10)) { - mins = 0; - } - hint.text( - hint.data('hosts-label') + ' ' + ( - mins === 0 ? - hint.data('default-label') : - mins + ' ' + hint.data('min-label') - ) - ); - // Same reasoning as the enforce select: the field is a pending - // action (blank = leave alone), not a display of the current value, - // which the hint above now carries. Leaving the number in it reads - // as a change still queued. - $('#tme').val(''); - }); - }); - - // Host enforce ad join reboot and hostname changes area - var groupModuleEnforceBtn = $('#group-enforce-send'), - groupModuleEnforceForm = $('#group-enforce-form'); - - function disableModuleEnforceButtons(disable) { - groupModuleEnforceBtn.prop('disabled', disable); - } - - groupModuleEnforceForm.on('submit', function(e) { - e.preventDefault(); - }); - - groupModuleEnforceBtn.on('click', function(e) { - e.preventDefault(); - disableModuleEnforceButtons(true); - var method = $(this).attr('method'), - action = $(this).attr('action'), - opts = { - confirmenforcesend: 1, - // #enforce is a tri-state , so the ternary posted 0 for every - // choice -- "Enable on all hosts" disabled them instead, and - // "No change" clobbered the whole group. Send the raw value and - // let groupModulePost() decide; it already ignores ''. - enforce: $('#enforce').val() - }; - $.apiCall(method,action,opts,function(err) { - disableModuleEnforceButtons(false); - // '' was a no-op server-side, so nothing moved; a failure leaves the - // members as they were. Only a successful '1'/'0' changes state. - if (err || (opts.enforce !== '1' && opts.enforce !== '0')) { - return; - } - // Every member was just forced to the same value, so the readout is - // known -- no round trip needed. Translated text comes from the - // server via data-*; this only chooses between them. - var hint = $('#enforce-shared-hint'); - hint.text( - hint.data('hosts-label') + ' ' + ( - opts.enforce === '1' ? - hint.data('enabled-label') : - hint.data('disabled-label') - ) - ); - // The select is a pending action, not a value display. Leaving it - // armed makes the card claim a change is still queued when it has - // already been applied, so put it back to the no-clobber default. - $('#enforce').val(''); - }); - }); - - // ACTIVE DIRECTORY TAB - var ADForm = $('#active-directory-form'), - ADFormBtn = $('#ad-send'), - ADClearBtn = $('#ad-clear'), - ADJoinDomain = $('#adEnabled'); - - // #adEnabled is a tri-state ' - . '' - . '' - . '' - . '' - // The hint is server-rendered from the members' current values, so - // it goes stale the moment the tri-state is applied over AJAX (the - // tab is never re-rendered). Applying '1'/'0' forces every member to - // that value, so the resulting state is known without a round trip - // -- carry both translated readouts as data-* for the JS to swap in, - // the same way makeReloadToggle() hands its labels over. Translation - // stays here; the JS only picks. - . '

' - . _('Hosts:') . ' ' . $enfText - . '

'; - $fields = [ - self::makeLabel( - $labelClass, - 'enforce', - _('Enforce Hostname | AD Join Reboots') - ) => $enforceControl - ]; - $buttons = self::makeButton( - 'group-enforce-send', - _('Update'), - 'btn btn-primary float-end', - $props - ); - - self::$HookManager->processEvent( - 'GROUP_ENFORCE_FIELDS', - [ - 'fields' => &$fields, - 'buttons' => &$buttons, - 'Group' => &$this->obj - ] - ); - $rendered = self::formFields($fields); - unset($fields); - - echo '
'; - echo '
'; - echo '

'; - echo _('Enforce Hostname | AD Join Reboots'); - echo '

'; - echo self::_pushDeprecationNotice(); - echo '

'; - echo _( - 'This tells the client to force reboots for host name ' - . 'changing and AD Joining.' - ); - echo '

'; - echo '

'; - echo _( - 'If disabled, the client will not make changes until all users ' - . 'are logged off' - ); - echo '

'; - echo '
'; - echo '
'; - echo self::makeFormTag( - '', - 'group-enforce-form', - self::makeTabUpdateURL( - 'group-module', - $this->obj->get('id') - ), - 'post', - 'application/x-www-form-urlencoded', - true - ); - echo $rendered; - echo ''; - echo '
'; - echo '
'; - echo $buttons; - echo '
'; - echo '
'; } /** * Group Service post. @@ -1581,40 +664,11 @@ public function groupModules() */ public function groupModulePost() { + // Grants only. The display-manager, auto-logout and enforce-hostname + // pushes that used to live here are set from the Hosts list's Edit + // selected hosts (ADR 0038 decision 10) as `resolution`, `autologout` + // and `enforce`. $this->assocPost('addModule', 'removeModule'); - if (isset($_POST['confirmdisplaysend'])) { - $x = (int)filter_input(INPUT_POST, 'x'); - $y = (int)filter_input(INPUT_POST, 'y'); - $r = (int)filter_input(INPUT_POST, 'r'); - $this->obj->setDisp($x, $y, $r); - } - if (isset($_POST['confirmalosend'])) { - // No-clobber: blank = leave each host's auto-logout alone. A number - // pushes to all (below ALO_MIN_MINUTES disables it, as before). - $raw = filter_input(INPUT_POST, 'tme'); - if ($raw !== null && trim((string)$raw) !== '') { - $tme = (int)$raw; - if ($tme < self::ALO_MIN_MINUTES) { - $tme = 0; - } - $this->obj->setAlo($tme); - } - } - if (isset($_POST['confirmenforcesend'])) { - // Tri-state: '' = no change (no-clobber), '1'/'0' = force on all. - // hostEnforce is tinyint(1) since ADR 0028, so a string and an - // int now mean the same thing. It stays a string because the - // POST value is one and the '' arm has to stay distinguishable - // from 0 -- casting would make "no change" mean "disable". - $enforce = (string)filter_input(INPUT_POST, 'enforce'); - if ($enforce === '1' || $enforce === '0') { - self::getClass('HostManager')->update( - ['id' => $this->obj->get('hosts')], - '', - ['enforce' => $enforce] - ); - } - } } /** * Display the group PM stuff. @@ -2540,15 +1594,6 @@ public function edit() } ]; - // Image - $tabData[] = [ - 'name' => _('Image'), - 'id' => 'group-image', - 'generator' => function () { - $this->groupImage(); - } - ]; - // Tasks $tabData[] = [ 'name' => _('Tasks'), @@ -2600,23 +1645,6 @@ public function edit() $this->groupModules(); } ], - [ - 'name' => _('Active Directory'), - 'id' => 'group-active-directory', - 'generator' => function () { - echo $this->_groupADStateHint(); - $this->adFieldsToDisplay( - '', - '', - '', - '', - '', - true, - false, - true - ); - } - ], [ 'name' => _('Power Management'), 'id' => 'group-powermanagement', @@ -2710,12 +1738,6 @@ function (&$serverFault) { case 'group-general': $this->groupGeneralPost(); break; - case 'group-image': - $this->groupImagePost(); - break; - case 'group-active-directory': - $this->groupADPost(); - break; case 'group-powermanagement': $this->groupPowermanagementPost(); break; diff --git a/packages/web/src/Pages/HostManagement.php b/packages/web/src/Pages/HostManagement.php index 1b5efe01a9..c5b136360f 100644 --- a/packages/web/src/Pages/HostManagement.php +++ b/packages/web/src/Pages/HostManagement.php @@ -4446,9 +4446,11 @@ private function massEditApplyRows(array $resolved, array $hostIDs) * (API_MASSDATA_MAPPING) and a bulk DELETE seam (DELETEMASS_API) and no * bulk EDIT seam. Between them sat the operation neither covers -- * changing a value across a set -- and its absence is why `location` and - * `ou` each ship a whole second hook file whose only job is to set one - * value across many hosts, always clobbering, unable to express "leave - * alone" at all. + * `ou` each shipped a whole second hook file whose only job was to set + * one value across many hosts, always clobbering, unable to express + * "leave alone" at all. Both now contribute through this seam instead + * (fog-plugins #36), which is what allowed decision 10's last step -- + * removing the group page's push controls -- to happen at all. * * The selection is passed because a plugin's mixed-value hint has to be * computed over it -- "(varies)" is a statement about THESE hosts. Both diff --git a/tests/auto-logout-minimum-has-one-spelling.test.php b/tests/auto-logout-minimum-has-one-spelling.test.php index 618ae51e1a..39fa09486e 100644 --- a/tests/auto-logout-minimum-has-one-spelling.test.php +++ b/tests/auto-logout-minimum-has-one-spelling.test.php @@ -52,9 +52,14 @@ * `$tme` is the field name on both pages' forms, so a comparison against a * bare number is the drift this file is for. */ +// The group page is deliberately not in this list any more. ADR 0038 +// decision 10 removed its auto-logout card -- the setting is edited from the +// Hosts list's mass edit, so the group page has no $tme to compare and no +// minimum to hand the browser. Leaving it here would assert that a page still +// gates on a rule it no longer applies, which passes only while some +// unrelated mention of MIN_MINUTES survives in the file. $pages = [ 'src/Pages/HostManagement.php', - 'src/Pages/GroupManagement.php', ]; foreach ($pages as $rel) { $src = (string)@file_get_contents($webroot . '/' . $rel); @@ -68,25 +73,14 @@ ); } -// The group page hands the same number to the browser. A form that lets a -// value through which the save then discards is the visible half of this -// drift, so the attribute has to come from the constant too. -$group = (string)@file_get_contents( - $webroot . '/src/Pages/GroupManagement.php' +// The one remaining spelling has to be the constant itself, not a literal +// that happens to agree with it today. +$host = (string)@file_get_contents( + $webroot . '/src/Pages/HostManagement.php' ); $check( - 'the group form gets data-alo-min from the constant', - 1 === preg_match( - '/data-alo-min="\'\s*\.\s*self::ALO_MIN_MINUTES/', - $group - ) -); -$check( - 'the group page aliases the constant rather than restating it', - 1 === preg_match( - '/const ALO_MIN_MINUTES = HostAutoLogout::MIN_MINUTES;/', - $group - ) + 'the host mass edit gets its minimum from HostAutoLogout', + 1 === preg_match('/HostAutoLogout::MIN_MINUTES/', $host) ); if (count($failures)) { diff --git a/tests/group-grants-are-owned.test.php b/tests/group-grants-are-owned.test.php index 3decd276ee..db4600d7d3 100644 --- a/tests/group-grants-are-owned.test.php +++ b/tests/group-grants-are-owned.test.php @@ -422,41 +422,104 @@ function ($sql) { } // --------------------------------------------------------------- -// Decision 10's middle step: the controls that COPY onto member hosts say -// so, and the grant tabs beside them do not. Getting that backwards would -// tell an admin the group's snapins are going away, which is the opposite -// of what this ADR does. -$notice = '_pushDeprecationNotice'; +// Decision 10's LAST step: the controls that copied onto member hosts are +// gone, not merely marked. They were deprecated in #1647 and removed once +// fog-plugins #36 gave `location` and `ou` the same seam the core fields +// use, so nothing was left depending on a group-page push. +// +// These assertions replace the ones that pinned the deprecation notices. +// Inverting them rather than deleting them is deliberate: "the notice is +// present" and "the control is absent" are both statements about the same +// migration, and the second is the one worth keeping, because it is the +// state that must not silently regress. $t->check( - 'the push-deprecation notice exists', - false !== strpos($pageSrc, "private static function $notice(") + 'the push-deprecation notice is gone with the controls it described', + false === strpos($pageSrc, '_pushDeprecationNotice') ); + +// The whole tabs, and their POST handlers. foreach ( [ - 'groupGeneral', 'groupImage', - 'groupPrinters', - 'groupModules', - 'groupPowermanagement', + 'groupImagePost', + 'groupADPost', + '_uniformHostValues', + '_sharedHint', + '_sharedAloHint', + '_groupADStateHint', ] as $method ) { - $body = methodBody($pageSrc, $method); - // groupPowermanagement is the control: it creates tasks rather than - // copying a value, so it must NOT be marked. - $want = 'groupPowermanagement' !== $method; $t->check( - $want - ? "$method() marks its imperative controls deprecated" - : "$method() is not marked, because it copies nothing", - $want === (false !== strpos($body, $notice)) + "$method() is gone from the group page", + false === strpos($pageSrc, " function $method(") ); } -// One per imperative card, not one per tab: the printers and modules tabs -// each carry a grant tab AND a push control, so a single tab-level banner -// would condemn the grants too. + +// A dispatch arm surviving its handler is the shape that fails at runtime +// rather than at parse time, so it is worth its own check. +foreach (['group-image', 'group-active-directory'] as $tab) { + $t->check( + "the '$tab' tab is no longer dispatched", + false === strpos($pageSrc, "case '$tab':") + ); +} + +// The pushes that lived inside surviving methods. Each of these is a write +// to the MEMBER hosts from a group control -- the exact thing ADR 0038 +// removes -- and each would look, from the page, like the control that used +// to be there. +$pushes = [ + 'groupGeneralPost' => ['HostManager', 'productKey', 'bootTypeExit'], + 'groupPrinterPost' => ['printerLevel', 'confirmlevelup'], + 'groupModulePost' => ['setDisp', 'setAlo', 'confirmenforcesend'], +]; +foreach ($pushes as $method => $needles) { + $body = codeOnly(methodBody($pageSrc, $method)); + foreach ($needles as $needle) { + $t->check( + "$method() no longer pushes $needle onto the members", + false === strpos($body, $needle) + ); + } +} + +// The control. groupPowermanagement CREATES TASKS rather than copying a +// value, so it is not part of this removal and must still be there -- a +// sweep that took it too would be over-deletion passing as success. $t->check( - 'every imperative card carries its own notice', - 6 === substr_count($pageSrc, "self::$notice()") + 'groupPowermanagement() survives, because it copies nothing', + false !== strpos($pageSrc, ' function groupPowermanagement(') ); +// And the grants themselves are untouched: this removed the pushes, not the +// tabs the ADR is delivering. +foreach (['groupPrinters', 'groupSnapins', 'groupModules'] as $method) { + $t->check( + "$method() still renders its grant tab", + false !== strpos(methodBody($pageSrc, $method), 'renderAssocTab') + || false !== strpos(methodBody($pageSrc, $method), 'assocItemsList') + ); +} + +// The JS that drove the removed cards goes with them; a handler left bound +// to an id nothing renders is dead code that still looks wired. +$js = (string)file_get_contents( + $root . '/management/js/fog/group/fog.group.edit.js' +); +foreach ( + [ + 'group-image-send', + 'printer-config-send', + 'group-displayman-send', + 'group-alo-send', + 'group-enforce-send', + 'alo-shared-hint', + ] as $id +) { + $t->check( + "the group JS no longer wires #$id", + false === strpos($js, $id) + ); +} + $t->finish();