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

Pm job management #3899

Merged
merged 14 commits into from
Nov 15, 2024
Merged

Pm job management #3899

merged 14 commits into from
Nov 15, 2024

Conversation

ashton22305
Copy link
Contributor

Work on #3759

Adds a button to delete a completed job from the job log and a button to stop a queued or running job.

@johrstrom
Copy link
Contributor

Sorry for the delay, I'll be looking into this today.

Comment on lines 124 to 130
@project.remove_logged_job(job_details_params[:jobid].to_s, cluster_str)

@valid_project = Launcher.clusters?
@valid_scripts = Launcher.scripts?(@project.directory)
@scripts = Launcher.all(@project.directory)

render :show
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should follow a pattern here where remove_logged_job returns true or false based on whether the operation was successful. Then the controller toggles based off of that.

Like say how update, destroy, create or works in this same controller. I think redirecting here is fine (so that we don't have to create/recreate those attributes), it just seems like the toggle is redirect with a notice when successful or an alert for a failure.

Comment on lines 141 to 151
begin
cluster.job_adapter.delete(job_details_params[:jobid].to_s) unless hpc_job.status.to_s == 'completed'
rescue StandardError => e
flash.now[:alert] = I18n.t('dashboard.jobs_project_generic_error', error: e.message.to_s)
end

@valid_project = Launcher.clusters?
@valid_scripts = Launcher.scripts?(@project.directory)
@scripts = Launcher.all(@project.directory)

render :show
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar comment to above, I think we should redirect instead of creating all these objects all over again.

Also seems like you need a notice in the case that it was successful.

Copy link
Contributor

Choose a reason for hiding this comment

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

All of these destructive buttons need an alert confirmation.

All you need to do is add a data-confirm attribute with some string. Here I just pulled a random confirm localized string. You could make a new one or use this, I'm sort of ambivalent on that.

diff --git a/apps/dashboard/app/views/projects/buttons/_completed_buttons.html.erb b/apps/dashboard/app/views/projects/buttons/_completed_buttons.html.erb
index 7d09e06fe..0df5fa971 100644
--- a/apps/dashboard/app/views/projects/buttons/_completed_buttons.html.erb
+++ b/apps/dashboard/app/views/projects/buttons/_completed_buttons.html.erb
@@ -2,5 +2,6 @@
   'Delete',
   project_delete_job_path(project_id: project_id, cluster: cluster, jobid: id),
   method: :delete,
+  data: { confirm: t('dashboard.batch_connect_sessions_delete_confirm') },
   class: 'btn btn-danger'
 ) %>

Copy link
Contributor

@johrstrom johrstrom left a comment

Choose a reason for hiding this comment

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

Thanks for putting up with all the back and forth!

@johrstrom johrstrom merged commit 6fa9a84 into master Nov 15, 2024
25 checks passed
@johrstrom johrstrom deleted the pm-job-management branch November 15, 2024 15:47
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.

3 participants