Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Servers table switch to ag-grid #4769

Merged
merged 27 commits into from
Jun 17, 2020
Merged

Servers table switch to ag-grid #4769

merged 27 commits into from
Jun 17, 2020

Conversation

ocket8888
Copy link
Contributor

@ocket8888 ocket8888 commented Jun 9, 2020 •
edited
Loading

What does this PR (Pull Request) do?

Switches the "/servers" table from jQuery datatables to agGrid, increasing load speed by a factor of about 14.

The new tables have a superset of the functionality of the old tables.

Which Traffic Control components are affected by this PR?

  • Traffic Portal

What is the best way to verify this PR?

Run Traffic Portal - preferably with a whole ton of servers so you can be impressed by the lightning fast load speed (at least compared to the jQuery data tables).

If #4747 isn't merged yet then you'll need to be using a version of TO that doesn't include the changes from #4700 .

The following criteria are ALL met by this PR

  • This PR includes tests OR I have explained why tests are unnecessary
  • This PR includes documentation OR I have explained why documentation is unnecessary
  • This PR includes an update to CHANGELOG.md OR such an update is not necessary
  • This PR includes any and all required license headers
  • This PR ensures that database migration sequence is correct OR this PR does not include a database migration
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY

@ocket8888 ocket8888 added improvement The functionality exists but it could be improved in some way. low impact affects only a small portion of a CDN, and cannot itself break one performance impacts/improves/measures performance Traffic Portal v1 related to Traffic Portal version 1 labels Jun 9, 2020
@ocket8888 ocket8888 marked this pull request as ready for review June 10, 2020 00:07
@mitchell852
Copy link
Member

mitchell852 commented Jun 16, 2020 •
edited
Loading

image

after getting this, i cleared local storage and it works now. maybe this is because your work is in progress?

@mitchell852
Copy link
Member

image

Not sure why "Clear Updates" is disabled.

@ocket8888
Copy link
Contributor Author

"Clear Server Updates" is meant to be disabled when the server isn't a cache server and/or doesn't have pending updates. "Queue Server Updates" is meant to be disabled when the server isn't a cache server and/or does have pending updates. Just to be clear about the intended behavior, because the old context menu would hide those options instead of disabling them.

Clearly, though, that's a cache server, and it looks like it has updates pending. So something's probably wrong.

mitchell852
mitchell852 reviewed Jun 16, 2020
View reviewed changes
Copy link
Member

@mitchell852 mitchell852 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A couple suggestions:

  1. add a global search if it's not too hard. i think there is value in the ability to do a quick search across all columns. and the less functionality lost, the easier the transition to the new grid. plus, i think it might help getting the TP tests to pass.
  2. i've found that some people don't like pagination or they like to adjust the number of rows in a page. maybe infinite scroll would be cool or at least a way to adjust pagination counts.
  3. cdn column is empty but i think you know that.

otherwise, very cool!

@mitchell852
Copy link
Member

Clearly, though, that's a cache server, and it looks like it has updates pending. So something's probably wrong.

yeah, something is weird there. it's a cache server and updPending=true so "clear updates" should be an option. i looked at the logic and it seemed right so not sure:

            <button type="button" ng-click="clearServerUpdates(server, $event)" ng-disabled="!isCache(server) || !server.updPending">Clear Server Updates</button>

@mitchell852
Copy link
Member

mitchell852 commented Jun 16, 2020 •
edited
Loading

Maybe in order to get this merged sooner rather than later. just focus on these 3 things i found:

  1. cdn column is empty
  2. clear updates context menu is disabled
  3. ensure that this is not a problem:

image

other stuff can be added in a subsequent PR:

  • global search
  • infinite scrolling maybe?

@ocket8888
Copy link
Contributor Author

"cdn column is empty" is already fixed. Might not have pushed it.
"Failed to load stored column state: one or more columns not found" is also not a real problem. I think that's actually related to the cdns thing. It saved column state with a "cdn" column, and the fix was to make it a "cdnName" column. So you need to clear your local storage. Nobody who didn't use it while it was still in a pull request should see that error.

mitchell852
mitchell852 reviewed Jun 16, 2020
View reviewed changes
Copy link
Member

@mitchell852 mitchell852 left a comment

Choose a reason for hiding this comment

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

couple more things

traffic_portal/app/src/common/modules/table/servers/TableServersController.js Outdated Show resolved Hide resolved
traffic_portal/app/src/common/modules/table/servers/TableServersController.js Show resolved Hide resolved
@mitchell852
Copy link
Member

nit: the context menu doesn't close when you select 'open in new tab" or "navigate to fqdn" or "show charts". basically anything that opens a new window/tab

@ocket8888
Copy link
Contributor Author

That should be fixed now

@mitchell852 mitchell852 self-assigned this Jun 17, 2020
@mitchell852 mitchell852 merged commit e5d7dc8 into apache:master Jun 17, 2020
@ocket8888 ocket8888 deleted the ag-grid-tables branch June 17, 2020 01:42
mitchell852 pushed a commit to mitchell852/trafficcontrol that referenced this pull request Sep 2, 2020
@ocket8888 @mitchell852
Servers table switch to ag-grid (apache#4769)
d5241a3 
* Added ag-grid tables to TP

* Fixed build issues

* Replaced servers table with agGrid

* Switch to tabs

* Started setting up context menu support

* Added some context menu functionality, fixed some styling

* Put styling in the stylesheet; added server delete to context menu

* Moved menu outside of panel

* Added server status update to context menu

* Added queue updates to context menu

* Added clear updates to context menu; added disabled menuitem styling

* Fixed a type error, finished context menu

* Removed unused things, general clean-up and re-organization

* added CSV export

* Rolled back changes to source of AngularJS

* Fixed menuitem button styling

* table now saves sort, filter, and column state

* table now saves column sizes

* Rolled back inneffectual browserify changes

* fixes server TP tests to work with ag-grid

* fixes broken TP ds test

* adds a new super controller for the *servers tables

* Fixed blank CDNs column

* Fixed incorrect update pending label icon

* Removed unused coldef properties, removed unused gridOptions property

* Fixed broken 'show charts' button in servers table context menu

* Fixed context menu not closing when certain actions were selected

Co-authored-by: Jeremy Mitchell <mitchell852@gmail.com>
(cherry picked from commit e5d7dc8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mitchell852 mitchell852 mitchell852 left review comments

Assignees

@mitchell852 mitchell852

Labels
improvement The functionality exists but it could be improved in some way. low impact affects only a small portion of a CDN, and cannot itself break one performance impacts/improves/measures performance Traffic Portal v1 related to Traffic Portal version 1
Projects
None yet
Milestone
No milestone
2 participants
@ocket8888 @mitchell852