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

Fixes to edit script javascript #3095

Merged
merged 2 commits into from
Oct 3, 2023
Merged

Conversation

abujeda
Copy link
Contributor

@abujeda abujeda commented Oct 2, 2023

Fixes to the edit script javascript functionality:

  • Fixed error when adding a new field after removing all fields in the form.
  • Disabled Add new option button while there is one in progress.
  • Disabled Add new option button if no more fields to add.
  • Fixed form submission when all form fields are deleted.

function lastOption() {
return $('.new_script').find('.editable-form-field').last();
function addNewFieldButton() {
return $('#add_new_field_button');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the fix to support deleting all editable fields from the form.

@abujeda abujeda force-pushed the edit_script_js_fixes branch from 74f01ca to 4c2b74d Compare October 3, 2023 08:36
@@ -152,6 +152,7 @@ def destroy
end

def update(params)
params ||= {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why this is here - Rails should always populate params. In any case I think params is a HashWithIndifferentAccess which could be important in case we're using strings and symbols here and there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When all form fields are removed from the edit script page and then the form is submitted, the application will throw a null pointer and show the stacktrace on the screen. When there are no fields on the form, params is nil in update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

# ScriptsController#51
@script.update(save_script_params[:script])

save_script_params[:script] will return nil when the form is empty

Copy link
Contributor

Choose a reason for hiding this comment

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

When all form fields are removed from the edit script page and then the form is submitted

I haven't looked this over in detail quite yet - but you shouldn't be able to remove all fields. You need at least auto_script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK - at the moment you can. I will revert this and take a look to disable deleting auto_scripts from the form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@abujeda abujeda force-pushed the edit_script_js_fixes branch from 4c2b74d to 45fca40 Compare October 3, 2023 15:36
Comment on lines 64 to 66
def script_remove_field_enabled(id)
!['script_auto_scripts'].include?(id.to_s)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this to a question like removable? or removable_field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

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.

Looks good, thanks!

@johrstrom johrstrom merged commit f9f6438 into OSC:master Oct 3, 2023
@abujeda
Copy link
Contributor Author

abujeda commented Oct 4, 2023

One final thing @johrstrom, should we disable deleting auto_batch_clusters too?

It seems that even if deleted, it appears again in the show and edit views.

@johrstrom
Copy link
Contributor

One final thing @johrstrom, should we disable deleting auto_batch_clusters too?

No. To submit a job you need a script to submit and a scheduler to submit it to. That's across all schedulers and systems.

At OSC you'd also need to specify an account - so we probably need a ticket to make required items configurable.

But without a script to run or a cluster to submit it to - nothing can happen.

@abujeda
Copy link
Contributor Author

abujeda commented Oct 4, 2023

Sorry @johrstrom, I am not sure I am not reading your comment correctly.
By your comment, I am understanding that a user needs a script to submit and a cluster to submit it to. So auto_scripts and auto_batch_clusters are needed to successfully submit a job.

We removed the "Remove" button from auto_scripts for this reason. But you are saying that we do not need the same for auto_batch_clusters?

@johrstrom
Copy link
Contributor

We removed the "Remove" button from auto_scripts for this reason. But you are saying that we do not need the same for auto_batch_clusters?

Sorry the double logic of disable deleting has got me confused this morning. You're right and I'm consfused - users should not be able to delete auto_batch_clusters, so yes we should disable deleting for that field.

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