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

Fixed ActiveJobsHelper dependency on ApplicationHelper #3892

Merged
merged 2 commits into from
Oct 24, 2024

Conversation

abujeda
Copy link
Contributor

@abujeda abujeda commented Oct 22, 2024

After the refactoring completed in #3700, a dependency was added to the ActiveJobsHelper to render the status_label method. The dependent method is in ApplicationHelper

This is breaking the Active jobs > job details AJAX call.

This is a possible fix for the issue

@abujeda
Copy link
Contributor Author

abujeda commented Oct 22, 2024

A sample error message from the logs

undefined method `status_class' for #<ActiveJobs::Jobstatusdata:0x00007f3f62b04548 @pbsid="23", @jobname="ondemand/sys/ood/sys/RStudio", @username="ood", @account="aday">

    "<span class='badge #{status_class(status)}'>#{status_text(status)}</span>".html_safe
                          ^^^^^^^^^^^^
Did you mean?  status_label:undefined method `status_class' for #<ActiveJobs::Jobstatusdata:0x00007f3f62b04548 @pbsid="23", @jobname="ondemand/sys/ood/sys/RStudio", @username="ood", @account="aday">

    "<span class='badge #{status_class(status)}'>#{status_text(status)}</span>".html_safe
                          ^^^^^^^^^^^^
Did you mean?  status_label

@johrstrom
Copy link
Contributor

I can't replicate. What's more is, I think activejobs renders the label in javascript.

function status_label(status){
const labelClass = cssBadgeForState(status);
var label = "Undetermined"
if(status === "queued_held") {
label = "Hold";
} else {
label = capitalizeFirstLetter(status);
}
return `<span class="badge ${labelClass}">${escapeHtml(label)}</span>`;
}

Is the error stack coming from one of these rescue blocks? I'd have to look it over, but I don't think that should be a label string, it should be a symbol or string that's just undefined. Not sure why we're trying to add a label html string to a generic struct.

OpenStruct.new(name: jobid, error: "No job details because job has already left the queue." , status: status_label("completed") )

@abujeda
Copy link
Contributor Author

abujeda commented Oct 22, 2024

Are you testing with the latest from main? Not sure this change has made it to a release.

For me the error happens in 2 places. On the call to ActiveJobs::Jobstatusdata.new(data, cluster, true)
As Jobstatusdata constructor has a reference to status_label and on the rescue blocks of the controller.

ActiveJobs::Jobstatusdata.new(data, cluster, true)

self.status = status_label(info.status.state.to_s)

OpenStruct.new(name: jobid, error: "No job details available.\n" + e.backtrace.to_s, status: status_label("") )

@johrstrom
Copy link
Contributor

🤦‍♂️ it's when you expand one of the rows. I can take a look now.

@johrstrom
Copy link
Contributor

OK - I have 2 options for you. I can accept this, with another ticket to actually fix it, or we can actually fix it now.

Seems like the issue originates here -

All helpers are available when rendering partials (just FYI, they're not namespaced, they're all available and can be overwritten/redefined as they're loaded).

So what we should really do is refactor this so that the models just have a status string like undetermined and the partial (any partial) can use status_label.

@abujeda
Copy link
Contributor Author

abujeda commented Oct 22, 2024

OK - Will create the actual fix with this PR.

So what we should really do is refactor this so that the models just have a status string like undetermined and the partial (any partial) can use status_label.

Looking into this

@abujeda abujeda force-pushed the active_jobs_helper_fix branch from fa7fc24 to ac322be Compare October 22, 2024 16:08
@abujeda
Copy link
Contributor Author

abujeda commented Oct 22, 2024

I have completed the changes

@abujeda
Copy link
Contributor Author

abujeda commented Oct 24, 2024

I found an issue with the extended_data.json.builder template. I have committed a fix

@abujeda abujeda force-pushed the active_jobs_helper_fix branch from 6c21645 to 2485ad0 Compare October 24, 2024 12:06
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!

@johrstrom johrstrom merged commit 121dc72 into OSC:master Oct 24, 2024
23 checks passed
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