Merge ~caleb-ellis/maas:machine-truncation-2.6 into maas:master

Proposed by Caleb Ellis
Status: Merged
Approved by: Caleb Ellis
Approved revision: 2241cbe331554d97ebe6a4fa81c6e78045e3372a
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~caleb-ellis/maas:machine-truncation-2.6
Merge into: maas:master
Diff against target: 169 lines (+81/-15)
3 files modified
src/maasserver/static/js/angular/directives/machines_table.js (+31/-9)
src/maasserver/static/js/angular/directives/tests/test_machines_table.js (+30/-0)
src/maasserver/static/partials/machines-table.html (+20/-6)
Reviewer Review Type Date Requested Status
Steve Rydz (community) Approve
Adam Collard (community) Needs Information
MAAS Lander Approve
Review via email: mp+373912@code.launchpad.net

Commit message

Forward port machine list truncation from 2.6 branch.

Description of the change

## Done
- Copy+pasted the machine list truncation work from the 2.6 branch to here. This adds the "Show all..." functionality and simultaneously fixes the error where machines wouldn't load (likely fixed by `MachinesManager.clear()` being removed)

## QA
- Check that the machine list truncation works like in 2.6 i.e. a maximum of 5 machines show per group by default, clicking "Show all..." shows all. Test it out in grouping by status, grouping my owner, and no grouping.
- Go to a KVM details page and check that the machine list displays correctly there too.
- Go to any page that isn't the machine list and refresh the app. Navigate back to the machine list and check that the machines load in correctly.

To post a comment you must log in.
Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b machine-truncation-2.6 lp:~caleb-ellis/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 2241cbe331554d97ebe6a4fa81c6e78045e3372a

review: Approve
Revision history for this message
Adam Collard (adam-collard) wrote :

What's the rationale for this? I know we did it for 2.6, but do we want it in 2.7?

review: Needs Information
Revision history for this message
Steve Rydz (steverydz) wrote :

LGTM +1

review: Approve
Revision history for this message
Caleb Ellis (caleb-ellis) wrote :

To reiterate what was said on IRC - this branch improves first load performance on the machine list a lot by removing vs-repeat. It also solves the problem of having to reload the machine data every time a user navigates to the machine list. While we would have liked to have moved to React by now it just simply won't happen this cycle, so I've opted to pull in this stop-gap performance fix from 2.6.

Relevant performance discussions:
https://github.com/canonical-web-and-design/MAAS-squad/issues/1314
https://docs.google.com/document/d/18kE1XU56odpf_ZOWgiSaYPjLcmPfZy_AiK8ZiRn1mW8/edit

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/src/maasserver/static/js/angular/directives/machines_table.js b/src/maasserver/static/js/angular/directives/machines_table.js
index 0cd2753..7405028 100644
--- a/src/maasserver/static/js/angular/directives/machines_table.js
+++ b/src/maasserver/static/js/angular/directives/machines_table.js
@@ -74,15 +74,6 @@ function maasMachinesTable(
74 NodeStatus.EXITING_RESCUE_MODE,74 NodeStatus.EXITING_RESCUE_MODE,
75 NodeStatus.TESTING75 NodeStatus.TESTING
76 ];76 ];
77
78 // This is an performance optimisation to unblock initial rendering,
79 // otherwise when there are many machines, due to numerous nested
80 // ng-repeats the initial digest cycle is slow and the UI is
81 // blocked on first navigation.
82 if (angular.isDefined($scope.loading)) {
83 MachinesManager.clear();
84 $scope.metadata = MachinesManager.getMetadata();
85 }
86 const machines = MachinesManager.getItems();77 const machines = MachinesManager.getItems();
8778
88 // Scope variables.79 // Scope variables.
@@ -97,6 +88,29 @@ function maasMachinesTable(
97 machineActions: GeneralManager.getData("machine_actions")88 machineActions: GeneralManager.getData("machine_actions")
98 };89 };
9990
91 $scope.DISPLAY_LIMIT = 5;
92 $scope.displayLimits = {};
93 const groupLabels = [
94 "Failed",
95 "New",
96 "Commissioning",
97 "Testing",
98 "Ready",
99 "Allocated",
100 "Deploying",
101 "Deployed",
102 "Rescue mode",
103 "Releasing",
104 "Broken",
105 "Other"
106 ];
107
108 $scope.getLimit = group => $scope.displayLimits[group.label];
109
110 $scope.loadAll = selectedGroup => {
111 $scope.displayLimits[selectedGroup.label] = undefined;
112 };
113
100 $scope.statusMenuActions = [114 $scope.statusMenuActions = [
101 "commission",115 "commission",
102 "acquire",116 "acquire",
@@ -480,6 +494,9 @@ function maasMachinesTable(
480 ]494 ]
481 }495 }
482 ];496 ];
497 groupLabels.forEach(label => {
498 $scope.displayLimits[label] = $scope.DISPLAY_LIMIT;
499 });
483 return;500 return;
484 }501 }
485502
@@ -500,6 +517,9 @@ function maasMachinesTable(
500 });517 });
501518
502 $scope.groupedMachines = groupedByOwner;519 $scope.groupedMachines = groupedByOwner;
520 groupedByOwner.forEach(owner => {
521 $scope.displayLimits[owner.label] = $scope.DISPLAY_LIMIT;
522 });
503 return;523 return;
504 }524 }
505525
@@ -509,6 +529,7 @@ function maasMachinesTable(
509 machines: $scope.table.filteredMachines529 machines: $scope.table.filteredMachines
510 }530 }
511 ];531 ];
532 $scope.displayLimits["none"] = $scope.DISPLAY_LIMIT;
512 return;533 return;
513 };534 };
514535
@@ -517,6 +538,7 @@ function maasMachinesTable(
517 $scope.table.machines,538 $scope.table.machines,
518 $scope.search539 $scope.search
519 );540 );
541 $scope.displayLimits["none"] = $scope.DISPLAY_LIMIT;
520 };542 };
521543
522 // When the list of filtered machines change update the all checkbox.544 // When the list of filtered machines change update the all checkbox.
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 1d47339..d8403cd 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
@@ -1083,4 +1083,34 @@ barbaz`); // Has to be formatted this way for tooltip
1083 expect(scope.getArchitectureText("i386/generic")).toBe("i386");1083 expect(scope.getArchitectureText("i386/generic")).toBe("i386");
1084 });1084 });
1085 });1085 });
1086
1087 describe("display limits", () => {
1088 it("returns the default display limit for the group", () => {
1089 const directive = compileDirective();
1090 const scope = directive.isolateScope();
1091
1092 scope.groupByLabel = "status";
1093 const group = {
1094 label: "Allocated",
1095 machines: []
1096 };
1097 scope.$digest();
1098
1099 expect(scope.getLimit(group)).toEqual(scope.DISPLAY_LIMIT);
1100 });
1101
1102 it("sets the groups display limit to undefined", () => {
1103 const directive = compileDirective();
1104 const scope = directive.isolateScope();
1105
1106 scope.groupByLabel = "status";
1107 const group = {
1108 label: "New",
1109 machines: []
1110 };
1111 scope.loadAll(group);
1112
1113 expect(scope.getLimit(group)).toEqual(undefined);
1114 });
1115 });
1086});1116});
diff --git a/src/maasserver/static/partials/machines-table.html b/src/maasserver/static/partials/machines-table.html
index c925b57..5f8c4a9 100644
--- a/src/maasserver/static/partials/machines-table.html
+++ b/src/maasserver/static/partials/machines-table.html
@@ -99,12 +99,13 @@
99 <th class="p-table__col--storage u-align--right" role="columnheader"></th>99 <th class="p-table__col--storage u-align--right" role="columnheader"></th>
100 </tr>100 </tr>
101 </thead>101 </thead>
102 <tbody vs-repeat data-ng-if="!closedGroups.includes(group.label)">102 <tbody data-ng-if="!closedGroups.includes(group.label)">
103 <tr class="p-table__row" data-ng-repeat="node in group.machines | nodesFilter:search | orderBy:table.predicate:table.reverse track by node.system_id"103 <tr class="p-table__row"
104 data-ng-class="{ 'table--error': machineHasError({ $machine: node }),104 ng-class="{
105 'is-active': node.$selected,105 'table--error': machineHasError({ $machine: node }),
106 'is-grouped': group.label !== 'none'106 'is-active': node.$selected,
107 }"107 'is-grouped': group.label !== 'none'}"
108 ng-repeat="node in group.machines | limitTo:getLimit(group) | nodesFilter:search | orderBy:table.predicate:table.reverse track by node.system_id"
108 tabindex="0"109 tabindex="0"
109 >110 >
110 <td class="p-table__col--name p-double-row" aria-label="FQDN" data-ng-if="table.column === 'fqdn' || table.column === 'ip_address'">111 <td class="p-table__col--name p-double-row" aria-label="FQDN" data-ng-if="table.column === 'fqdn' || table.column === 'ip_address'">
@@ -449,6 +450,19 @@
449 </span>450 </span>
450 </td>451 </td>
451 </tr>452 </tr>
453 <tr data-ng-if="group.machines && group.machines.length > getLimit(group)">
454 <td colspan="12">
455 <div class="p-table__group-label" style="padding: .25rem 0 .3rem">
456 <a class="p-muted-text" data-ng-click="loadAll(group)">
457 <span>Show all</span>
458 <span data-ng-if="group.label === 'none'">machines&hellip;</span>
459 <span data-ng-if="group.label === 'No owner'">unowned&hellip;</span>
460 <span data-ng-if="groupByLabel === 'owner' && group.label !== 'No owner'">owned by {$ group.label $}&hellip;</span>
461 <span data-ng-if="groupByLabel === 'status'">{$ group.label | lowercase $}&hellip;</span>
462 </a>
463 </div>
464 </td>
465 </tr>
452 </tbody>466 </tbody>
453</table>467</table>
454<div class="p-strip" data-ng-if="search && !loading && !table.filteredMachines.length">468<div class="p-strip" data-ng-if="search && !loading && !table.filteredMachines.length">

Subscribers

People subscribed via source and target branches