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

Add project quick script launch button #2773

Closed
wants to merge 51 commits into from

Conversation

Oglopf
Copy link
Contributor

@Oglopf Oglopf commented Apr 20, 2023

Fixes #2652

┆Issue is synchronized with this Asana task by Unito

@Oglopf
Copy link
Contributor Author

Oglopf commented Apr 21, 2023

The functionality works, but the button placement is ugly and I need to fix it. After that I'll mark this as ready for review.

@Oglopf
Copy link
Contributor Author

Oglopf commented Apr 21, 2023

UI is done, but I need to fix some tests and write a few then it'll be ready.

@Oglopf
Copy link
Contributor Author

Oglopf commented Apr 21, 2023

Having some trouble with this failing test. The issue is the controller will see the cache and so the submit succeeds despite the Open3.stub(:capture3) so the alert-danger never shows up.

Tried to get around this using Script.any_instance.stubs(:cached_values).returns(false) but I'm running into the error we saw in #2770 when the test runs:

Error:
ProjectsTest#test_submitting_a_script_with_auto_attributes_that_fails:
ActionView::Template::Error: undefined method `most_recent_job_id' for nil:NilClass
    app/views/projects/show.html.erb:29
    app/views/projects/show.html.erb:25:in `each'
    app/views/projects/show.html.erb:25

For now, I'll write the tests for the new functionality and circle back to this later this afternoon.

@Oglopf
Copy link
Contributor Author

Oglopf commented Apr 21, 2023

Still having trouble with the tests and I'm struggling to understand how our fixtures and the auto stuff work in a test environment. Eventually I just made my own cache to use in hopes of getting the correct json object which worked, but now the stub on OodCore doesn't seem to return the job_id to display. I'll have to keep at it and see what's up.

@Oglopf
Copy link
Contributor Author

Oglopf commented Apr 28, 2023

I'm not sure how to get these tests to work. I'm fine just pulling the functionality in and writing a ticket to loop back and write them, because if not this will sit here until i understand how to mock the cluster in the tests.

@Oglopf Oglopf marked this pull request as ready for review April 28, 2023 18:50
@Oglopf
Copy link
Contributor Author

Oglopf commented Apr 28, 2023

Ah, i forgot the other test from before is still broke somehow from this so I'm gonna have to figure this out somehow.

@Oglopf Oglopf marked this pull request as draft April 28, 2023 18:52
@johrstrom
Copy link
Contributor

I know the fix for this error. We need to .compact the array in scripts#all. Somehow there's an invalid form somewhere and that script object is never created because it errors out.

Error:
ProjectsTest#test_submitting_a_script_with_auto_attributes_that_fails:
ActionView::Template::Error: undefined method `most_recent_job_id' for nil:NilClass
    app/views/projects/show.html.erb:29
    app/views/projects/show.html.erb:25:in `each'
    app/views/projects/show.html.erb:25

@johrstrom
Copy link
Contributor

I'm not sure how to get these tests to work. I'm fine just pulling the functionality in and writing a ticket to loop back and write them, because if not this will sit here until i understand how to mock the cluster in the tests.

Yea we can defer tests.

@johrstrom
Copy link
Contributor

@johrstrom I'm not sure how to get this to work. I keep getting failures around the cluster_id not being present and I can't track down how this works with the smart_attributes functionality. I can see the object in the logs, but trying to pull data from it fails. It also looks like the launch happens more than once when I click the button, which I also can't understand.

OK I can take a look.

osc-bot and others added 2 commits May 15, 2023 11:18
update changelog

---------

Co-authored-by: Jeff Ohrstrom <johrstrom@osc.edu>
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.

launch (scripts#submit) button on projects#show
6 participants