-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
Add filters to Allocations #11544
Add filters to Allocations #11544
Conversation
Ember Asset Size actionAs of ad80c84 Files that got Bigger 🚨:
Files that stayed the same size 🤷:
|
Ember Test Audit comparison
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
Filtering by status doesn't work because the filter is reading the wrong alloc property (status
vs clientStatus
). It would be good to have some tests for this as well, they would've likely caught this problem. Take a look at other pages with filters to see how they are done.
I think filtering by job version would could be helpful as well, and seems like a quick addition. But feel free to drop it if it's not.
The PR is also missing a CHANGELOG entry.
Searchable, | ||
WithNamespaceResetting | ||
) { | ||
/*eslint-disable indent */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh...I forgot to fix this. I think we need to use this ruleset to prevent eslint
and prettier
from fighting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't fix it.
Ember Test Audit flaky testsEmber Test Audit detected these flaky tests on main:
Ember Test Audit detected these flaky tests on febcd5e:
|
|
||
@computed('model.allocations.[]', 'selectionClient') | ||
get optionsClients() { | ||
const clients = Array.from(new Set(this.model.allocations.mapBy('node.shortId'))).compact(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are repeating this multiple times in this file. I understand that this pattern is used in other places as well controllers/clients/index.js
for example. It looks like we can create a computed-macro out of this pattern.
// Update query param when the list of clients changes. | ||
scheduleOnce('actions', () => { | ||
// eslint-disable-next-line ember/no-side-effects | ||
this.set('qpClient', serialize(intersection(clients, this.selectionClient))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably don't want to trigger side-effects in computeds. Again this is done in other places as well but we should investigate why we feel this is necessary to do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can tackle the macro in a follow-up PR and apply it to the rest of the codebase that also uses this pattern.
Good work! 👏
Ember Test Audit flaky testsEmber Test Audit detected these flaky tests on a8854bc:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think filtering by job version would could be helpful as well
On second thought, it wasn't very helpful. Long-lived jobs can have their version in the hundreds (if not thousands), the filter list would be pretty useless and hard to use.
So I removed this filter. Sorry for the unnecessary work 😅
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. |
Mock
Actual