UI: Fix client sorting #6817
UI: Fix client sorting #6817
Conversation
Sorting starts as descending by default!
| - listSorted: a copy of listToSort that has been sorted | ||
| */ | ||
| export default function sortableFactory(properties) { | ||
| const eachProperties = properties.map(property => `listToSort.@each.${property}`); |
There was a problem hiding this comment.
I agree with your concern that this problem should be fixed for all tables, and I also agree your resistance to dig into all of them at once.
Maybe a good middle ground just so the problem isn't forgotten about once this merges is to warn here if properties.length === 0. The warning could also include something like "The Sortable mixin is deprecated in favor of SortableFactory".
There was a problem hiding this comment.
👍 thanks for the suggestion; I added something in 0131c36, it’s somewhat unwieldy because of when mixins are mixed in, but it works, thoughts?
There was a problem hiding this comment.
Looks like it'll do, but I agree it's unwieldy. What's preventing you from putting it right here, in the sortableFactory but not in the returned Mixin? Don't we have all the information at this point? It would also prevent the state tracking needed right now.
There was a problem hiding this comment.
oops, I merged this 😳 I’m sorry
I think that would have the same problem though because the function is called when the mixin is created, which happens when the application loads, so the warnings all happen at the same time.
There was a problem hiding this comment.
Nice job chasing down this bug and good writeup too!
I agree with all the decisions you made, even if they fall in the "lesser of two evils" camp.
The only strong suggestions I have are the warning in SortableFactory and merging state and compositeStatus and glossing over the behavioral different in node-status-light.scss.
But I'd also be happy merging this as is if you disagree.
| } else { | ||
| return this.status; | ||
| } | ||
| }), |
There was a problem hiding this comment.
Hmmmm. I think we can drop compositeStatus in favor of state (but maybe keep the compositeStatus name? idk) with little effort or risk. We could add a css class for the node status light for draining that is equivalent to ineligible.
There was a problem hiding this comment.
Agreed, I did this in ad3863f. One caveat is that I removed an assertion in a client detail test:
assert.equal(
ClientDetail.statusLight.objectAt(0).id,
node.status,
'Title includes status light'
);
Being able to test that with randomness was unreliable and would require the duplication of the compositeStatus computed property in the test. Since there’s another test that specifically sets a node to ineligible and checks for that, it seemed to me safe to remove.
I did make .draining equivalent to .ineligible for the light, but I wonder whether it wouldn’t make sense to use the blue draining colour that shows in the client details boxed section and the state column of the client list? For consistency.
Also, I’ve been surprised that hovering over the status light doesn’t have any description of what the colour means; it’s true that it can usually be decoded by looking for the corresponding colour in the client details boxed section, but maybe a tooltip would be a good idea to be more explicit?
There was a problem hiding this comment.
Good thoughts on ways to improve that indicator. I know @joshklekamp noodled with it too as part of the drain work (added icons, thought through colors).
I forget if he added a draining state. Either way, worth discussing further but out of scope for this bug fix.
| .lookup('service:store') | ||
| .peekAll('node') | ||
| .findBy('modifyIndex', 4); | ||
| readyClient.set('schedulingEligibility', 'ineligible'); |
There was a problem hiding this comment.
I don't like this, but as you point out there isn't a good way to emulate blocking queries in tests, sooooooooo....
This less-than-elegant inclusion of the warning in the computed property is necessary because the mixing-in happens as the application is initialised, so having it outside of the computed property causes it to unhelpfully with no context.
This seems unhelpful to have printed repeatedly in tests.
These fields only differed by the inclusion of draining, and the existing compositeStatus was only being used in one place. This removes an assertion that was unreliable with certain random circumstances; it checked that the composite status was equal to the node status, but that wasn’t true for draining nodes. It felt okay to remove because testing a generalised replacement would necessitate reproducing the entire computed property and there’s already an assertion that tests the specific “ineligible” composite status.
|
Thanks for the feedback, @DingoEatingFuzz. I’ve made some followup commits and have a question about tooltips for the status light, let me know what you think? |
I unintentionally introduced a flapping test in #6817. The draining status of the node will be randomly chosen and that flag takes precedence over eligibility. This forces the draining flag to be false rather than random so the test should no longer flap. See here for an example failure: https://circleci.com/gh/hashicorp/nomad/26368
I unintentionally introduced a flapping test in #6817. The draining status of the node will be randomly chosen and that flag takes precedence over eligibility. This forces the draining flag to be false rather than random so the test should no longer flap. See here for an example failure: https://circleci.com/gh/hashicorp/nomad/26368
I unintentionally introduced a flapping test in #6817. The draining status of the node will be randomly chosen and that flag takes precedence over eligibility. This forces the draining flag to be false rather than random so the test should no longer flap. See here for an example failure: https://circleci.com/gh/hashicorp/nomad/26368
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This is a first pass at a fix for #6262. Here’s a GIF of it working; look for the fourth client to change state and become the third in the list:
Here’s a GIF of what happens in
master; look for the fourth client changing state but not sort position:You can see it yourself by visiting the
masterNetlify deployment and running this in the console:There are two changes here, and some caveats/commentary:
The “State“ table column was actually sorting only by
status. Thestatewas not an actual property, just something calculated in each client row, as a product ofstatus,isEligible, andisDraining. This PR extracts that calculation into the model so it can be sorted on.drainingclients are separated by aninitializinginterlopercompositeStatuscomputed property that combinesstatusandisEligible; it’s used to determine the colour of the circle on the individual client routestateandcompositeStatusare close relatives, and the nomenclature is confusing, not sure of the best way out of thisThe
Sortablemixin declares dependent keys that cause the sort to be live-updating, but only if the members of the array change, such as if a new client is added, but not if any of the sortable properties change. This PR adds aSortableFactoryfunction that generates a mixin whoselistSortedcomputed property includes dependent keys for the sortable properties, so the table will live-update if any of the sortable properties change, not just the array members.Sortableinterface stable by making it callSortableFactorywith no list of dependent keysNomadUi.__container__.lookup('service:store').peekAll('job').find(job => job.name.startsWith('jbod')).set('type', 'system')