Merge ~caleb-ellis/maas:deploy-in-table into maas:master

[Caleb Ellis]

LP: #1825143 - Display default OS/release deployment in in-table action menu

## Done
- Display default OS/release deployment in in-table action menu
- Removed in-table actions from pod details page (was causing errors if refreshing the browser at this page, because not all the machine data is passed through in this view alone)

## QA
- Check that deploying a machine has the correct default OS and release selected

## Screenshot
https://user-images.githubusercontent.com/25733845/55886580-f0d81280-5ba3-11e9-9882-67771d200656.png

[Andres Rodriguez (andreserl)]

This is not a review, but just a quick question, shouldn't the backend be doing a failsafe in which if the UI doesn't send this info, its already set? Or is the issue because the UI is setting blank fields and sending blank fields causes this?

[Caleb Ellis (caleb-ellis)]

I don't believe the backend falls back on inferring os/release from defaults if none are provided to the deploy method, so the issue was originally that these were not provided by the in-table deploy action (which I've added in this MP).

[Anthony Dillon (ya-bo-ng)]

Code and QA looks good to me. +1

review: Approve

[Lee Trager (ltrager)]

This is actually a backend error. What happened is the UI asked the region to deploy over the websocket and didn't specify an osystem or distro series. The osystem and distro series are both set to '' which the UI shows as /. I've posted a fix[1] to the websocket so a osystem and distro series are always set, like the API.

[1] https://code.launchpad.net/~ltrager/maas/+git/maas/+merge/366158

review: Disapprove

[Caleb Ellis (caleb-ellis)]

Fair enough - I can remove the part where default params are passed to the deploy method via the UI, but I think displaying the default os/release in the menu is still helpful to the user.

[Andres Rodriguez (andreserl)]

is The commit message still valid? Since Lee has submitted a branch to fix this in the backend I don’t understand why this would be relevant for the UI anymore?

review: Needs Information

[Caleb Ellis (caleb-ellis)]

This branch is simply just to change the text from "Deploy..." to e.g "Deploy Ubuntu 18.04..." - not the most urgent, but the work's already done so why not have the UI nicety?

[Andres Rodriguez (andreserl)]

Caleb,

Maybe I’m missing something but i thought the UI already does show the OS being deployed?

[Andres Rodriguez (andreserl)]

In fact it’s always shown it?

[Caleb Ellis (caleb-ellis)]

You're right it does in the main action menu dropdown when you choose deploy, and in the status message, but it doesn't for the in-table action.

[Andres Rodriguez (andreserl)]

So just to confirm, if for whatever reason the OS is set to other than not default, the in table action would show that ? E.g. the machine does have a node.os field which could be forced to something different than the default, and would deploying that show it?

review: Needs Information

[Caleb Ellis (caleb-ellis)]

The in-table deploy action only displays whatever is set as default in settings. If you wanted to deploy a machine using non-default os/release you would have to do that via the top-right action menu that shows the different dropdowns for os/release/kernel. The only visible difference in this MP is that "Deploy..." becomes "Deploy {{ default_os/default_release }}..."

[Lee Trager (ltrager)]

Thanks for the update and the explanation. This LGTM!

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b deploy-in-table lp:~caleb-ellis/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/5543/consoleText

2d2239f... by Caleb Ellis on 2019-04-25

fix tests

diff --git a/src/maasserver/static/js/angular/directives/machines_table.js b/src/maasserver/static/js/angular/directives/machines_table.js
index 710d9ea..672fb66 100644
--- a/src/maasserver/static/js/angular/directives/machines_table.js
+++ b/src/maasserver/static/js/angular/directives/machines_table.js
@@ -18,7 +18,7 @@ function maasMachinesTable(
       actionOption: "=",
       ngDisabled: "&",
       machineHasError: "&",
-      hideCheckboxes: "=?",
+      hideActions: "=?",
       onListingChange: "&",
       onCheckAll: "&",
       onCheck: "=",
@@ -71,7 +71,8 @@ function maasMachinesTable(
       allViewableChecked: false,
       machines,
       filteredMachines: machines,
-      osinfo: GeneralManager.getData("osinfo")
+      osinfo: GeneralManager.getData("osinfo"),
+      machineActions: GeneralManager.getData("machine_actions")
     };
 
     $scope.statusMenuActions = [
@@ -584,7 +585,28 @@ function maasMachinesTable(
     };
 
     $scope.getActionTitle = actionName => {
-      const machineActions = GeneralManager.getData('machine_actions');
+      const { table } = $scope;
+      const { machineActions, osinfo } = table;
+
+      // Display default OS and release if in-table action is "deploy"
+      if (actionName === "deploy" && osinfo) {
+        const {
+          osystems,
+          releases,
+          default_osystem,
+          default_release
+        } = osinfo;
+        const os = osystems && osystems.find(os => os[0] === default_osystem);
+        const release = releases && releases.find(
+          release => release[0] === `${default_osystem}/${default_release}`
+        );
+        if (os && release) {
+          return `Deploy ${os[1]} ${release[1]}...`;
+        }
+        return `Deploy ${default_osystem}/${default_release}...`;
+      }
+
+      // Otherwise, just display the action's title
       if (machineActions.length) {
         return machineActions
           .find(action => action.name === actionName)
diff --git a/src/maasserver/static/js/angular/directives/tests/test_machines_table.js b/src/maasserver/static/js/angular/directives/tests/test_machines_table.js
index a7dd821..c1e7f2b 100644
--- a/src/maasserver/static/js/angular/directives/tests/test_machines_table.js
+++ b/src/maasserver/static/js/angular/directives/tests/test_machines_table.js
@@ -46,6 +46,19 @@ describe("maasMachinesTable", function() {
         return machine;
     }
 
+    // Make OS choice.
+    function makeOS() {
+        const name = makeName("os");
+        return [name, name.toUpperCase()];
+    }
+
+    // Make release choice for OS.
+    function makeRelease(os) {
+        const release = makeName("release");
+        const osRelease = os[0] + "/" + release;
+        return [osRelease, release];
+    }
+
     // Return the compiled directive with the items from the scope.
     function compileDirective(design) {
         var directive;
@@ -78,7 +91,8 @@ describe("maasMachinesTable", function() {
           allViewableChecked: false,
           machines: MachinesManager.getItems(),
           filteredMachines: MachinesManager.getItems(),
-          osinfo: GeneralManager.getData("osinfo")
+          osinfo: GeneralManager.getData("osinfo"),
+          machineActions: GeneralManager.getData("machine_actions")
         });
         expect(scope.table.machines).toBe(MachinesManager.getItems());
     });
@@ -764,16 +778,38 @@ describe("maasMachinesTable", function() {
         it("returns the correct action title, given an action name", () => {
             const directive = compileDirective();
             const scope = directive.isolateScope();
-            const actions = [
-                { title: "this action title", name: "this_action" },
-                { title: "other action title", name: "other_action" }
-            ];
+            scope.table = {
+                machineActions: [
+                    { title: "this action title", name: "this_action" },
+                    { title: "other action title", name: "other_action" }
+                ]
+            };
 
-            spyOn(GeneralManager, "getData")
-                .and.returnValue(actions);
+            expect(scope.getActionTitle(scope.table.machineActions[0].name))
+                .toEqual(scope.table.machineActions[0].title);
+        });
+
+        it("returns the default OS and release if action is 'deploy'", () => {
+            const directive = compileDirective();
+            const scope = directive.isolateScope();
+            const [defaultOS, otherOS] = [makeOS(), makeOS()];
+            const [defaultRelease, otherRelease]
+                = [makeRelease(defaultOS), makeRelease(otherOS)];
+
+            scope.table = {
+                osinfo: {
+                    default_osystem: defaultOS[0],
+                    default_release: defaultRelease[0].split("/")[1],
+                    osystems: [defaultOS, otherOS],
+                    releases: [defaultRelease, otherRelease]
+                },
+                machineActions: [
+                    { title: "Deploy...", name: "deploy" }
+                ]
+            };
 
-            expect(scope.getActionTitle(actions[0].name))
-                .toEqual(actions[0].title);
+            expect(scope.getActionTitle("deploy"))
+                .toEqual(`Deploy ${defaultOS[1]} ${defaultRelease[1]}...`);
         });
     });
 
diff --git a/src/maasserver/static/partials/machines-table.html b/src/maasserver/static/partials/machines-table.html
index 16a1748..abd1366 100644
--- a/src/maasserver/static/partials/machines-table.html
+++ b/src/maasserver/static/partials/machines-table.html
@@ -2,7 +2,7 @@
     <thead>
         <tr class="p-table__header p-table__row" data-ng-if="$first">
             <th class="p-table__col--name p-double-row u-align--left">
-                <div class="p-double-row__checkbox" data-ng-if="!hideCheckboxes">
+                <div class="p-double-row__checkbox" data-ng-if="!hideActions">
                     <input class="checkbox" type="checkbox" data-ng-click="toggleCheckAll()" data-ng-checked="table.allViewableChecked" id="check-all" data-ng-disabled="ngDisabled()" />
                     <label class="p-checkbox--action" for="check-all" ng-class="getAllCheckboxClass(table.filteredMachines)"></label>
                 </div>
@@ -73,7 +73,7 @@
                              'is-grouped': group.label !== 'none'
                             }">
             <td class="p-table__col--name p-double-row" aria-label="FQDN" data-ng-if="table.column === 'fqdn' || table.column === 'ip_address'">
-                <div class="p-double-row__checkbox" data-ng-if="!hideCheckboxes">
+                <div class="p-double-row__checkbox" data-ng-if="!hideActions">
                     <input type="checkbox" data-ng-click="toggleChecked(node)" data-ng-checked="node.$selected" id="{$ node.fqdn $}" data-ng-disabled="ngDisable()" />
                     <label class="p-checkbox--action" for="{$ node.fqdn $}" ng-class="getCheckboxClass(node)"></label>
                 </div>
@@ -101,7 +101,7 @@
                 </div>
             </td>
             <td class="p-table__col--name" aria-label="MAC" data-ng-if="table.column === 'pxe_mac'">
-                <div class="u-float--left" data-ng-if="!hideCheckboxes">
+                <div class="u-float--left" data-ng-if="!hideActions">
                     <input class="checkbox" type="checkbox" data-ng-click="toggleChecked(node)" data-ng-checked="node.$selected" id="{$ node.fqdn $}" data-ng-disabled="ngDisable()" />
                     <label class="p-checkbox--action checkbox-label" for="{$ node.fqdn $}" ng-class="getCheckboxClass(node)"></label>
                 </div>
@@ -110,7 +110,7 @@
             </td>
             <td class="p-table__col--power p-double-row" aria-label="Power state">
                 <span class="p-table-menu">
-                    <div data-ng-click="toggleMenu(node.system_id + '-power')" class="p-double-row__icon-container">
+                    <div data-ng-click="!hideActions && toggleMenu(node.system_id + '-power')" class="p-double-row__icon-container">
                         <i
                             title="{$ node.power_state $}"
                             data-ng-class="{
@@ -131,7 +131,7 @@
                             <span title="{$ node.power_type $}">{$ node.power_type $}</span>
                         </div>
                     </div>
-                    <button class="p-table-menu__toggle p-icon--contextual-menu u-hide--br3" data-ng-click="toggleMenu(node.system_id + '-power')"></button>
+                    <button class="p-table-menu__toggle p-icon--contextual-menu u-hide--br3" data-ng-click="toggleMenu(node.system_id + '-power')" data-ng-if="!hideActions"></button>
                     <span class="p-table-menu__dropdown" data-ng-show="openMenu === node.system_id + '-power'" data-ng-if="!node.powerTransition">
                         <div class="p-table-menu__title--icon">Take action:</div>
                         <button
@@ -167,7 +167,7 @@
             </td>
             <td class="p-table__col--status p-double-row" aria-label="Status, Events" title="{$ getStatusText(node) $}">
                 <span class="p-table-menu">
-                    <div class="p-double-row__icon-container" data-ng-click="toggleMenu(node.system_id + '-status')">
+                    <div class="p-double-row__icon-container" data-ng-click="!hideActions && toggleMenu(node.system_id + '-status')">
                         <i data-ng-if="showSpinner(node)" class="p-icon--spinner u-animation--spin"></i>
                         <i data-ng-if="showNodeStatus(node)" data-maas-script-status="script-status" data-script-status="node.other_test_status" aria-label="{$ node.other_test_status_tooltip $}"></i>
                         <span class="p-tooltip p-tooltip--top-left" data-ng-if="showFailedTestWarning(node)">
@@ -184,7 +184,7 @@
                             <span>{$ getStatusMessage(node) $}</span>
                         </div>
                     </div>
-                    <button class="p-table-menu__toggle p-icon--contextual-menu u-hide--br3" data-ng-click="toggleMenu(node.system_id + '-status')"></button>
+                    <button class="p-table-menu__toggle p-icon--contextual-menu u-hide--br3" data-ng-click="toggleMenu(node.system_id + '-status')" data-ng-if="!hideActions"></button>
                     <span class="p-table-menu__dropdown" data-ng-show="openMenu === node.system_id + '-status'">
                         <div class="p-table-menu__title">Take action:</div>
                         <div class="p-contextual-menu__group" data-ng-if="getStatusActions(node).length">
@@ -225,7 +225,7 @@
                             <span>{$ node.tags.join(', ') $}</span>
                         </div>
                     </div>
-                    <button class="p-table-menu__toggle p-icon--contextual-menu u-hide--br2" data-ng-click="toggleMenu(node.system_id + '-owner')"></button>
+                    <button class="p-table-menu__toggle p-icon--contextual-menu u-hide--br2" data-ng-click="toggleMenu(node.system_id + '-owner')" data-ng-if="!hideActions"></button>
                     <span class="p-table-menu__dropdown" data-ng-if="openMenu === node.system_id + '-owner'">
                         <div class="p-table-menu__title">Take action:</div>
                         <button
@@ -266,7 +266,7 @@
                         </div>
                     </div>
                     <button
-                        data-ng-if="node.actions.indexOf('set-pool') !== -1"
+                        data-ng-if="node.actions.indexOf('set-pool') !== -1 && !hideActions"
                         data-ng-click="toggleMenu(node.system_id + '-pool')"
                         class="p-table-menu__toggle p-icon--contextual-menu"
                     >
@@ -301,7 +301,7 @@
                         </div>
                     </div>
                     <button
-                        data-ng-if="node.actions.indexOf('set-zone') !== -1"
+                        data-ng-if="node.actions.indexOf('set-zone') !== -1 && !hideActions"
                         data-ng-click="toggleMenu(node.system_id + '-zone')"
                         class="p-table-menu__toggle p-icon--contextual-menu"
                     >
diff --git a/src/maasserver/static/partials/pod-details.html b/src/maasserver/static/partials/pod-details.html
index 6558e4a..bcfc763 100644
--- a/src/maasserver/static/partials/pod-details.html
+++ b/src/maasserver/static/partials/pod-details.html
@@ -646,7 +646,7 @@
         <section class="p-strip">
             <div class="row">
                 <div class="col-12" data-ng-if="pod.composed_machines_count">
-                    <maas-machines-table search="machinesSearch" hide-checkboxes="true" pools="pools" zones="zones"></maas-machines-table>
+                    <maas-machines-table search="machinesSearch" hide-actions="true" pools="pools" zones="zones"></maas-machines-table>
                 </div>
             </div>
             <div class="row">