View and Edit Subscriptions in full cores. #644
Conversation
|
|
There was a problem hiding this comment.
Thanks for sending this!
This is looking good. One thing -- the "Edit Subscription Size" and "Edit Burst Size" dialogs still show the larger numbers -- those will need to be changed too.
(To be clear -- you get to those dialogs by right-clicking on each subscription in the list.)
|
Thanks for the feedback, |
|
That's correct -- it is possible to utilize only partial cores, which is why the data is stored that way. However it's more of a "power user" type of feature. The most common way of using OpenCue is to only use full cores. So I think we should keep the database/API values as they are, and only change the GUI. |
|
Changes have been made :) |
|
#648 is merged so please rebase or merge master into your branch. That should hopefully fix the tests. |
Changed the subscriptions display so as to display straight cores instead of cores*100.
Changed the "Edit Subscription Size" and "Edit Burst Size" widgets so that they no longer display cores*100 but straight cores.
57e769a
to
10ab364
Compare
|
Done, let me know if its alright. |
|
Tests didn't re-run automatically -- we seem to be having some problem with our Azure integration -- but I re-ran them manually here: https://dev.azure.com/cipriano0988/OpenCue%20Testing/_build/results?buildId=769&view=results Tests passing now, as you can see. |
There was a problem hiding this comment.
Getting close -- one more change needed to get the correct values passed back into the API backend.
Also, please update the PR title to be more descriptive. Something like "Display and edit subscriptions in full cores."
The PR titles get copied into our release notes automatically, so it's good to imagine you're reading release notes and think about what information you'd need to understand exactly what changed.
Thanks!
| @@ -1138,7 +1138,7 @@ def editSize(self, rpcObjects=None): | |||
| "contact the resource department." | |||
| minSize = 0 | |||
| decimalPlaces = 0 | |||
| (value, choice) = QtWidgets.QInputDialog.getDouble(self._caller, title, body, current, | |||
| (value, choice) = QtWidgets.QInputDialog.getDouble(self._caller, title, body, current/100.0, | |||
There was a problem hiding this comment.
Remember that the API backend is still expecting values in hundredths of a core, so when you call cuebotCall below you need to multiple value by 100 there.
Also -- please update the tests for editSize and editBurst to assert that the correct value (i.e. the input * 100) is passed to setSize/setBurst.
To run the unit tests locally you can change to the cuegui directory then run python setup.py test .
Updated editSize and editBurst so that they send values in terms of hundredths of a core. Updated Tests for the same.
|
All changes made. Let me know if anything else is to be done :) |
There was a problem hiding this comment.
Looks good to me!
Waiting on a second approval now. @smith1511 @larsbijl @gregdenton
|
@smith1511 @larsbijl @gregdenton Could we get a second review here please? This is tied to Summer of Code work so I'd like to get it merged ASAP. |
|
LGTM |
Fixes #639
Changed the subscriptions display so as to display straight cores instead of cores*100.