-
Notifications
You must be signed in to change notification settings - Fork 77
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 device count and estimated device count for Deployment views #1517
Conversation
7426761
to
cbef6d6
Compare
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.
All in all this looks great. Just some small change suggestions.
cbef6d6
to
5b6e1af
Compare
Gotta make it not break also... |
This allows seeing how big a deployment is in device count in listings and detail view. Beyond that the Edit form for Deployments will estimate the matching devices based on the conditions set.
5b6e1af
to
f8a4870
Compare
And fixed :D |
Test hit a deadlock in an unrelated test so I gave it a re-run. Worth figuring out but very unlikely to be this PR? |
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.
Is it worth us adding tests for this?
Possibly but not enough that I think anyone would care if it broke and it would take kind of weird stuff to break it. You can make a call on this. I'll write the tests if you prefer them. For me they are smack in the do/don't split. |
Same, this is smack bang on that line. I'm going to skip the need for a test as we are looking at making changes to Deployments in the long run. |
This allows seeing how big a deployment is in device count in listings
and detail view.
Beyond that the Edit form for Deployments will estimate the matching
devices based on the conditions set.