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.

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

fix: remove job move actions from webui #9603

Merged
merged 5 commits into from
Jul 9, 2024

Conversation

kkunapuli
Copy link
Contributor

@kkunapuli kkunapuli commented Jul 3, 2024

Ticket

RM-342

Description

Followup to #9585, which removes the ability for users to update a job's queue position within the same priority group in the backend. Now that actions like "Move To Top" and repositioning a job within "Manage Job" have no effect, remove the buttons from the webui.

"Move to Top" on Resource Pool page should be removed and "Position in Queue" on "Manage Job" pop-up should be removed, as shown in the highlighted section in screenshots.

Test Plan

None needed.

Checklist

  • Changes have been manually QA'd
  • New features have been approved by the corresponding PM
  • User-facing API changes have the "User-facing API Change" label
  • Release notes have been added as a separate file under docs/release-notes/
    See Release Note for details.
  • Licenses have been included for new code which was copied and/or modified from any external code

Screenshot 2024-07-02 at 2 03 37 PM
Screenshot 2024-07-02 at 2 03 16 PM

@cla-bot cla-bot bot added the cla-signed label Jul 3, 2024
Copy link

netlify bot commented Jul 3, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit ef04804
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/668563374f0fa50008a4232a
😎 Deploy Preview https://deploy-preview-9603--determined-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@kkunapuli kkunapuli changed the base branch from main to job-queue-move July 3, 2024 15:06
Copy link

codecov bot commented Jul 3, 2024

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 51.71%. Comparing base (95e090f) to head (0c294ab).

Additional details and impacted files
@@               Coverage Diff                @@
##           job-queue-move    #9603    +/-   ##
================================================
  Coverage           51.71%   51.71%            
================================================
  Files                1255     1255            
  Lines              152575   152447   -128     
  Branches             3091     3081    -10     
================================================
- Hits                78905    78844    -61     
+ Misses              73513    73446    -67     
  Partials              157      157            
Flag Coverage Δ
backend 44.17% <ø> (+<0.01%) ⬆️
harness 72.76% <ø> (ø)
web 48.63% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
master/internal/job/jobservice/jobservice.go 9.48% <ø> (+0.31%) ⬆️
webui/react/src/utils/job.ts 78.26% <ø> (+1.46%) ⬆️
webui/react/src/pages/JobQueue/JobQueue.tsx 0.00% <0.00%> (ø)
webui/react/src/pages/JobQueue/ManageJob.tsx 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

@kkunapuli kkunapuli changed the title first pass fix: remove job move actions from webui Jul 3, 2024
@kkunapuli kkunapuli marked this pull request as ready for review July 3, 2024 18:00
@kkunapuli kkunapuli requested review from a team as code owners July 3, 2024 18:00
@kkunapuli kkunapuli requested review from NicholasBlaskey, thiagodallacqua-hpe and hamidzr and removed request for a team and NicholasBlaskey July 3, 2024 18:00
Copy link
Contributor

@thiagodallacqua-hpe thiagodallacqua-hpe left a comment

Choose a reason for hiding this comment

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

Looking good from the Web UI side, just a minor comment.

Comment on lines -122 to -129
const rpTotalJobCount = useCallback(
(rpName: string) => {
const stats = rpStats.find((rp) => rp.resourcePool === rpName)?.stats;
return stats ? stats.queuedCount + stats.scheduledCount : 0;
},
[rpStats],
);

Copy link
Contributor

Choose a reason for hiding this comment

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

I get that the objective is to remove the unsupported actions, but, is this part of it? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question! As far as I can tell, rpTotalJobCount is only used to display out of x in the Position in Queue section (that we are removing).

Copy link
Contributor

@carolinaecalderon carolinaecalderon left a comment

Choose a reason for hiding this comment

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

LGTM

@kkunapuli kkunapuli merged commit b293470 into job-queue-move Jul 9, 2024
80 of 93 checks passed
@kkunapuli kkunapuli deleted the kunapuli/webui-deprecate-job-queue-move branch July 9, 2024 20:36
Copy link
Contributor

@hamidzr hamidzr left a comment

Choose a reason for hiding this comment

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

flu took me out for a few days but fwiw ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants