From 8b3bc712e9501e35c1b9ea5262da3eda9aa9ffbe Mon Sep 17 00:00:00 2001 From: Aday Bujeda Date: Fri, 1 Sep 2023 17:26:29 +0100 Subject: [PATCH 1/2] Updated project and script model to use random alphanumeric ids --- apps/dashboard/app/models/project.rb | 7 +- apps/dashboard/app/models/script.rb | 26 +- .../app/views/projects/index.html.erb | 2 +- .../app/views/projects/show.html.erb | 2 +- apps/dashboard/test/system/jobs_app_test.rb | 232 ++++++++++-------- 5 files changed, 153 insertions(+), 116 deletions(-) diff --git a/apps/dashboard/app/models/project.rb b/apps/dashboard/app/models/project.rb index cecec9dd11..152d61c4a5 100644 --- a/apps/dashboard/app/models/project.rb +++ b/apps/dashboard/app/models/project.rb @@ -21,10 +21,7 @@ def lookup_table end def next_id - lookup_table.map { |id, _directory| id } - .map(&:to_i) - .concat([0]) # could be the first project - .max + 1 + SecureRandom.alphanumeric(8).downcase end def all @@ -35,7 +32,7 @@ def all def find(id) opts = lookup_table.select do |lookup_id, _directory| - lookup_id == id.to_i + lookup_id == id.to_s end.map do |lookup_id, directory| { id: lookup_id, directory: directory } end.first diff --git a/apps/dashboard/app/models/script.rb b/apps/dashboard/app/models/script.rb index 2606a2f8f0..696b889b13 100644 --- a/apps/dashboard/app/models/script.rb +++ b/apps/dashboard/app/models/script.rb @@ -5,7 +5,7 @@ class Script class ClusterNotFound < StandardError; end - attr_reader :title, :id, :project_dir, :smart_attributes + attr_reader :title, :id, :created_at, :project_dir, :smart_attributes class << self def scripts_dir(project_dir) @@ -14,6 +14,10 @@ def scripts_dir(project_dir) end end + def creation_marker_path(script_dir) + File.join(script_dir, ".created.json") + end + def find(id, project_dir) script_path = Script.script_path(project_dir, id) file = script_form_file(script_path) @@ -23,7 +27,9 @@ def find(id, project_dir) def all(project_dir) Dir.glob("#{scripts_dir(project_dir).to_s}/*/form.yml").map do |file| Script.from_yaml(file, project_dir) - end.compact + end.compact.sort_by do |s| + s.created_at + end end def from_yaml(file, project_dir) @@ -41,11 +47,7 @@ def from_yaml(file, project_dir) end def next_id(project_dir) - all(project_dir) - .map(&:id) - .map(&:to_i) - .prepend(0) - .max + 1 + SecureRandom.alphanumeric(8).downcase end end @@ -55,6 +57,7 @@ def initialize(opts = {}) @project_dir = opts[:project_dir] || raise(StandardError, 'You must set the project directory') @id = opts[:id] @title = opts[:title].to_s + @created_at = opts[:created_at] sm_opts = { form: opts[:form] || [], attributes: opts[:attributes] || {} @@ -78,7 +81,7 @@ def to_yaml hash[sm.id.to_s] = sm.options_to_serialize end.deep_stringify_keys - hsh = { 'title' => title } + hsh = { 'title' => title, 'created_at' => created_at } hsh.merge!({ 'form' => smart_attributes.map { |sm| sm.id.to_s } }) hsh.merge!({ 'attributes' => attributes }) hsh.to_yaml @@ -129,9 +132,11 @@ def []=(_id, value) def save @id = Script.next_id(project_dir) if @id.nil? + @created_at = Time.now.to_i if @created_at.nil? script_path = Script.script_path(project_dir, id) script_path.mkpath unless script_path.exist? File.write(Script.script_form_file(script_path), to_yaml) + add_creation_marker(script_path) true rescue StandardError => e @@ -251,6 +256,11 @@ def cache_file_exists? cache_file_path.exist? end + def add_creation_marker(script_dir) + creation_marker = Pathname.new(Script.creation_marker_path(script_dir)) + File.write(creation_marker, { }.to_json) unless creation_marker.exist? + end + def cached_values @cached_values ||= begin cache_file_path = OodAppkit.dataroot.join(Script.scripts_dir("#{project_dir}"), "#{id}_opts.json") diff --git a/apps/dashboard/app/views/projects/index.html.erb b/apps/dashboard/app/views/projects/index.html.erb index 28412fba35..15003b04ba 100644 --- a/apps/dashboard/app/views/projects/index.html.erb +++ b/apps/dashboard/app/views/projects/index.html.erb @@ -6,7 +6,7 @@
<% @projects.each do |project| %> -
+
<%= link_to(project_path(project.id), class: 'text-dark') do %> <%= icon_tag(URI.parse(project.icon)) %> diff --git a/apps/dashboard/app/views/projects/show.html.erb b/apps/dashboard/app/views/projects/show.html.erb index 47e6b6e0b4..90658e0b33 100644 --- a/apps/dashboard/app/views/projects/show.html.erb +++ b/apps/dashboard/app/views/projects/show.html.erb @@ -23,7 +23,7 @@ <% @scripts.each do |script| %> - + #{project_dir}/my_cool_script.sh` `echo 'hostname' > #{project_dir}/my_cooler_script.bash` + + project_id end def setup_script(project_id) @@ -35,11 +44,15 @@ def setup_script(project_id) click_on 'New Script' find('#script_title').set('the script title') click_on 'Save' + + script_element = find('.script-card') + script_element[:id] end - def add_account(project_id) + def add_account(project_id, script_id) visit project_path(project_id) - find("[href='/projects/#{project_id}/scripts/1/edit']").click + edit_script_path = edit_project_script_path(project_id, script_id) + find(%Q{[href="#{edit_script_path}"]}).click # now add 'auto_accounts' click_on('Add new option') @@ -48,9 +61,10 @@ def add_account(project_id) click_on(I18n.t('dashboard.save')) end - def add_bc_num_hours(project_id) + def add_bc_num_hours(project_id, script_id) visit project_path(project_id) - find("[href='/projects/#{project_id}/scripts/1/edit']").click + edit_script_path = edit_project_script_path(project_id, script_id) + find(%Q{[href="#{edit_script_path}"]}).click # now add 'bc_num_hours' click_on('Add new option') @@ -62,54 +76,58 @@ def add_bc_num_hours(project_id) test 'create a new project on fs and display the table entry' do Dir.mktmpdir do |dir| - setup_project(dir) + project_id = setup_project(dir) assert_selector '.alert-success', text: 'Project successfully created!' - assert_selector '[href="/projects/1"]', text: 'Test Project' - assert File.directory? File.join("#{dir}/projects", '1') + assert_selector %Q{[href="/projects/#{project_id}"]}, text: 'Test Project' + assert File.directory? File.join("#{dir}/projects", project_id) end end test 'creates .ondemand directory with project' do Dir.mktmpdir do |dir| - setup_project(dir) + project_id = setup_project(dir) - assert File.directory? File.join("#{dir}/projects", '1/.ondemand') + assert File.directory? File.join("#{dir}/projects", project_id, '.ondemand') end end - test 'creates project overriding default project location' do + test 'creates project overriding project location' do Dir.mktmpdir do |dir| - setup_project(dir, true) + project_override_dir = File.join(dir, 'dir_override') + Pathname(project_override_dir).mkpath + setup_project(dir, project_override_dir) - assert File.directory? File.join("#{dir}", '.ondemand') + assert File.directory?(File.join("#{project_override_dir}", '.ondemand')) + assert File.exist?(File.join("#{project_override_dir}", '.ondemand', 'manifest.yml')) #Cleanup to avoid side effects accept_confirm do click_on 'Delete' end + assert_selector '.alert-success', text: 'Project successfully deleted!' end end test 'delete a project from the fs and ensure no table entry' do Dir.mktmpdir do |dir| - setup_project(dir) + project_id = setup_project(dir) accept_confirm do click_on 'Delete' end assert_selector '.alert-success', text: 'Project successfully deleted!' - assert_no_selector '[href="/projects/1"]', text: 'Test Project' - assert_not File.directory? File.join("#{dir}/projects", '1') + assert_no_selector %Q{[href="/projects/#{project_id}"]}, text: 'Test Project' + assert_not File.directory? File.join("#{dir}/projects", project_id) end end test 'work in a project' do Dir.mktmpdir do |dir| - setup_project(dir) + project_id = setup_project(dir) - find('[href="/projects/1"]').click + find(%Q{[href="/projects/#{project_id}"]}).click assert_selector 'h1', text: 'Test Project' assert_selector '.btn.btn-default', text: 'Back' end @@ -117,16 +135,16 @@ def add_bc_num_hours(project_id) test 'edit a project' do Dir.mktmpdir do |dir| - setup_project(dir) + project_id = setup_project(dir) click_on 'Edit' find('#project_name').set('my-test-project') click_on 'Save' - assert_selector '[href="/projects/1"]', text: 'My Test Project' + assert_selector %Q{[href="/projects/#{project_id}"]}, text: 'My Test Project' click_on 'Edit' assert_selector 'h1', text: 'Editing: My Test Project' assert_equal 'my-test-project', find('#project_name').value - assert_equal "#{dir}/projects/1", find('#project_directory').value + assert_equal "#{dir}/projects/#{project_id}", find('#project_directory').value assert_equal 'test-description', find('#project_description').value assert_equal 'fas://arrow-right', find('#product_icon_select').value assert_selector '.btn.btn-default', text: 'Back' @@ -176,12 +194,13 @@ def add_bc_num_hours(project_id) test 'creating and showing scripts' do Dir.mktmpdir do |dir| - setup_project(dir) - setup_script(1) + project_id = setup_project(dir) + script_id = setup_script(project_id) expected_yml = <<~HEREDOC --- title: the script title + created_at: #{@expected_now} form: - auto_batch_clusters - auto_scripts @@ -196,10 +215,10 @@ def add_bc_num_hours(project_id) auto_scripts: options: - - my_cool_script.sh - - "#{dir}/projects/1/my_cool_script.sh" + - "#{dir}/projects/#{project_id}/my_cool_script.sh" - - my_cooler_script.bash - - "#{dir}/projects/1/my_cooler_script.bash" - directory: "#{dir}/projects/1" + - "#{dir}/projects/#{project_id}/my_cooler_script.bash" + directory: "#{dir}/projects/#{project_id}" label: Script help: '' required: false @@ -207,21 +226,23 @@ def add_bc_num_hours(project_id) success_message = I18n.t('dashboard.jobs_scripts_created') assert_selector('.alert-success', text: "×\nClose\n#{success_message}") - assert_equal(expected_yml, File.read("#{dir}/projects/1/.ondemand/scripts/1/form.yml")) + assert_equal(expected_yml, File.read("#{dir}/projects/#{project_id}/.ondemand/scripts/#{script_id}/form.yml")) - find('[href="/projects/1/scripts/1"].btn-success').click + script_path = project_script_path(project_id, script_id) + find(%Q{[href="#{script_path}"].btn-success}).click assert_selector('h1', text: 'the script title', count: 1) end end test 'showing scripts with auto attributes' do Dir.mktmpdir do |dir| - setup_project(dir) - setup_script(1) - project_dir = "#{dir}/projects/1" - add_account(1) + project_id = setup_project(dir) + script_id = setup_script(project_id) + project_dir = File.join(dir, 'projects', project_id) + add_account(project_id, script_id) - find('[href="/projects/1/scripts/1"].btn-success').click + script_path = project_script_path(project_id, script_id) + find(%Q{[href="#{script_path}"].btn-success}).click assert_selector('h1', text: 'the script title', count: 1) expected_accounts = ['pas1604', 'pas1754', 'pas1871', 'pas2051', 'pde0006', 'pzs0714', 'pzs0715', 'pzs1010', @@ -238,10 +259,10 @@ def add_bc_num_hours(project_id) test 'deleting a script that succeeds' do Dir.mktmpdir do |dir| - setup_project(dir) - setup_script(1) - project_dir = "#{dir}/projects/1" - script_dir = "#{project_dir}/.ondemand/scripts/1" + project_id = setup_project(dir) + script_id = setup_script(project_id) + project_dir = File.join(dir, 'projects', project_id) + script_dir = File.join(project_dir, '.ondemand', 'scripts', script_id) # ASSERT SCRIPT DIRECTORY IS CREATED assert_equal true, File.directory?(script_dir) @@ -264,13 +285,15 @@ def add_bc_num_hours(project_id) test 'submitting a script with auto attributes that succeeds' do Dir.mktmpdir do |dir| - setup_project(dir) - setup_script(1) - project_dir = "#{dir}/projects/1" - script_dir = "#{project_dir}/.ondemand/scripts/1" - add_account(1) + project_id = setup_project(dir) + script_id = setup_script(project_id) + project_dir = File.join(dir, 'projects', project_id) + script_dir = File.join(project_dir, '.ondemand', 'scripts', script_id) + add_account(project_id, script_id) + - find('[href="/projects/1/scripts/1"].btn-success').click + script_path = project_script_path(project_id, script_id) + find(%Q{[href="#{script_path}"].btn-success}).click assert_selector('h1', text: 'the script title', count: 1) # assert defaults @@ -292,14 +315,10 @@ def add_bc_num_hours(project_id) OodCore::Job::Adapters::Slurm.any_instance .stubs(:info).returns(OodCore::Job::Info.new(id: 'job-id-123', status: :running)) - Time - .stubs(:now) - .returns(Time.at(1_679_943_564)) - click_on 'Launch' assert_selector('.alert-success', text: 'job-id-123') assert_equal [{ 'id' => 'job-id-123', - 'submit_time' => 1_679_943_564, + 'submit_time' => @expected_now, 'cluster' => 'owens' }], YAML.safe_load(File.read("#{script_dir}/job_history.log")) end @@ -307,13 +326,14 @@ def add_bc_num_hours(project_id) test 'submitting a script with auto attributes that fails' do Dir.mktmpdir do |dir| - setup_project(dir) - setup_script(1) - project_dir = "#{dir}/projects/1" - script_dir = "#{project_dir}/.ondemand/scripts/1" - add_account(1) - - find('[href="/projects/1/scripts/1"].btn-success').click + project_id = setup_project(dir) + script_id = setup_script(project_id) + project_dir = File.join(dir, 'projects', project_id) + script_dir = File.join(project_dir, '.ondemand', 'scripts', script_id) + add_account(project_id, script_id) + + script_path = project_script_path(project_id, script_id) + find(%Q{[href="#{script_path}"].btn-success}).click assert_selector('h1', text: 'the script title', count: 1) # assert defaults @@ -340,11 +360,13 @@ def add_bc_num_hours(project_id) test 'editing scripts initializes correctly' do Dir.mktmpdir do |dir| - setup_project(dir) - setup_script('1') + project_id = setup_project(dir) + script_id = setup_script(project_id) + + visit project_path(project_id) - visit project_path('1') - find('[href="/projects/1/scripts/1/edit"]').click + edit_script_path = edit_project_script_path(project_id, script_id) + find(%Q{[href="#{edit_script_path}"]}).click click_on('Add new option') new_field_id = 'add_new_field_select' @@ -357,11 +379,13 @@ def add_bc_num_hours(project_id) test 'adding new fields to scripts' do Dir.mktmpdir do |dir| - setup_project(dir) - setup_script('1') + project_id = setup_project(dir) + script_id = setup_script(project_id) + + visit project_path(project_id) - visit project_path('1') - find('[href="/projects/1/scripts/1/edit"]').click + edit_script_path = edit_project_script_path(project_id, script_id) + find(%Q{[href="#{edit_script_path}"]}).click # only shows 'cluster' & 'auto_scripts' assert_equal 2, page.all('.form-group').size @@ -373,8 +397,9 @@ def add_bc_num_hours(project_id) end # add bc_num_hours - add_bc_num_hours(1) - find('[href="/projects/1/scripts/1/edit"]').click + add_bc_num_hours(project_id, script_id) + script_edit_path = edit_project_script_path(project_id, script_id) + find(%Q{[href="#{script_edit_path}"]}).click # now shows 'cluster', 'auto_scripts' & the newly added'bc_num_hours' assert_equal 3, page.all('.form-group').size @@ -393,12 +418,13 @@ def add_bc_num_hours(project_id) click_on(I18n.t('dashboard.save')) success_message = I18n.t('dashboard.jobs_scripts_updated') assert_selector('.alert-success', text: "×\nClose\n#{success_message}") - assert_current_path '/projects/1' + assert_current_path project_path(project_id) # note that bc_num_hours has default, min & max expected_yml = <<~HEREDOC --- title: the script title + created_at: #{@expected_now} form: - auto_scripts - auto_batch_clusters @@ -407,11 +433,11 @@ def add_bc_num_hours(project_id) auto_scripts: options: - - my_cool_script.sh - - "#{dir}/projects/1/my_cool_script.sh" + - "#{dir}/projects/#{project_id}/my_cool_script.sh" - - my_cooler_script.bash - - "#{dir}/projects/1/my_cooler_script.bash" - directory: "#{dir}/projects/1" - value: "#{dir}/projects/1/my_cool_script.sh" + - "#{dir}/projects/#{project_id}/my_cooler_script.bash" + directory: "#{dir}/projects/#{project_id}" + value: "#{dir}/projects/#{project_id}/my_cool_script.sh" label: Script help: '' required: false @@ -433,22 +459,23 @@ def add_bc_num_hours(project_id) required: true HEREDOC - assert_equal(expected_yml, File.read("#{dir}/projects/1/.ondemand/scripts/1/form.yml")) + assert_equal(expected_yml, File.read("#{dir}/projects/#{project_id}/.ondemand/scripts/#{script_id}/form.yml")) end end test 'removing script fields' do Dir.mktmpdir do |dir| - setup_project(dir) - setup_script('1') + project_id = setup_project(dir) + script_id = setup_script(project_id) # add bc_num_hours - add_bc_num_hours(1) - add_account(1) + add_bc_num_hours(project_id, script_id) + add_account(project_id, script_id) # go to edit it and see that there is cluster and bc_num_hours - visit project_path('1') - find('[href="/projects/1/scripts/1/edit"]').click + visit project_path(project_id) + edit_script_path = edit_project_script_path(project_id, script_id) + find(%Q{[href="#{edit_script_path}"]}).click # puts page.body assert_equal 4, page.all('.form-group').size assert_not_nil find('#script_auto_batch_clusters') @@ -471,11 +498,12 @@ def add_bc_num_hours(project_id) click_on(I18n.t('dashboard.save')) success_message = I18n.t('dashboard.jobs_scripts_updated') assert_selector('.alert-success', text: "×\nClose\n#{success_message}") - assert_current_path '/projects/1' + assert_current_path project_path(project_id) expected_yml = <<~HEREDOC --- title: the script title + created_at: #{@expected_now} form: - auto_accounts - auto_scripts @@ -501,11 +529,11 @@ def add_bc_num_hours(project_id) auto_scripts: options: - - my_cool_script.sh - - "#{dir}/projects/1/my_cool_script.sh" + - "#{dir}/projects/#{project_id}/my_cool_script.sh" - - my_cooler_script.bash - - "#{dir}/projects/1/my_cooler_script.bash" - directory: "#{dir}/projects/1" - value: "#{dir}/projects/1/my_cool_script.sh" + - "#{dir}/projects/#{project_id}/my_cooler_script.bash" + directory: "#{dir}/projects/#{project_id}" + value: "#{dir}/projects/#{project_id}/my_cool_script.sh" label: Script help: '' required: false @@ -519,7 +547,7 @@ def add_bc_num_hours(project_id) required: false HEREDOC - assert_equal(expected_yml, File.read("#{dir}/projects/1/.ondemand/scripts/1/form.yml")) + assert_equal(expected_yml, File.read("#{dir}/projects/#{project_id}/.ondemand/scripts/#{script_id}/form.yml")) end end @@ -555,18 +583,18 @@ def add_bc_num_hours(project_id) test 'cant show invalid script' do Dir.mktmpdir do |dir| - setup_project(dir) - visit project_script_path('1', '1') - assert_current_path('/projects/1') + project_id = setup_project(dir) + visit project_script_path(project_id, '1') + assert_current_path("/projects/#{project_id}") assert_selector('.alert-danger', text: "×\nClose\nCannot find script 1") end end test 'cant edit invalid script' do Dir.mktmpdir do |dir| - setup_project(dir) - visit edit_project_script_path('1', '1') - assert_current_path('/projects/1') + project_id = setup_project(dir) + visit edit_project_script_path(project_id, '1') + assert_current_path("/projects/#{project_id}") assert_selector('.alert-danger', text: "×\nClose\nCannot find script 1") end end @@ -579,11 +607,11 @@ def add_bc_num_hours(project_id) # asserts that the _new_ list of excluded accounts have actually been removed from script#show test 'excluding and including select options' do Dir.mktmpdir do |dir| - setup_project(dir) - setup_script(1) - add_account(1) + project_id = setup_project(dir) + script_id = setup_script(project_id) + add_account(project_id, script_id) - visit edit_project_script_path('1', '1') + visit edit_project_script_path(project_id, script_id) find('#edit_script_auto_accounts').click exclude_accounts = ['pas2051', 'pas1871', 'pas1754', 'pas1604'] @@ -602,8 +630,9 @@ def add_bc_num_hours(project_id) end find("#save_script_edit").click - assert_current_path('/projects/1') - find('[href="/projects/1/scripts/1"].btn-success').click + assert_current_path(project_path(project_id)) + script_path = project_script_path(project_id, script_id) + find(%Q{[href="#{script_path}"].btn-success}).click # now let's check scripts#show to see if they've actually been excluded. show_account_options = page.all("#script_auto_accounts option").map(&:value) @@ -611,7 +640,7 @@ def add_bc_num_hours(project_id) assert(!show_account_options.include?(acct)) end - visit edit_project_script_path('1', '1') + visit edit_project_script_path(project_id, script_id) find('#edit_script_auto_accounts').click exclude_accounts.each do |acct| @@ -629,8 +658,9 @@ def add_bc_num_hours(project_id) end find("#save_script_edit").click - assert_current_path('/projects/1') - find('[href="/projects/1/scripts/1"].btn-success').click + assert_current_path(project_path(project_id)) + script_path = project_script_path(project_id, script_id) + find(%Q{[href="#{script_path}"].btn-success}).click # now let's check scripts#show and they should be back. show_account_options = page.all("#script_auto_accounts option").map(&:value) From 48726e989bf4eec4bae61e5fbe63a0a45750af5a Mon Sep 17 00:00:00 2001 From: Aday Bujeda Date: Fri, 1 Sep 2023 17:55:49 +0100 Subject: [PATCH 2/2] Improvements to script model after review --- apps/dashboard/app/models/script.rb | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/apps/dashboard/app/models/script.rb b/apps/dashboard/app/models/script.rb index 696b889b13..e7cb0e55a7 100644 --- a/apps/dashboard/app/models/script.rb +++ b/apps/dashboard/app/models/script.rb @@ -14,10 +14,6 @@ def scripts_dir(project_dir) end end - def creation_marker_path(script_dir) - File.join(script_dir, ".created.json") - end - def find(id, project_dir) script_path = Script.script_path(project_dir, id) file = script_form_file(script_path) @@ -46,7 +42,7 @@ def from_yaml(file, project_dir) nil end - def next_id(project_dir) + def next_id SecureRandom.alphanumeric(8).downcase end end @@ -131,12 +127,11 @@ def []=(_id, value) end def save - @id = Script.next_id(project_dir) if @id.nil? + @id = Script.next_id if @id.nil? @created_at = Time.now.to_i if @created_at.nil? script_path = Script.script_path(project_dir, id) script_path.mkpath unless script_path.exist? File.write(Script.script_form_file(script_path), to_yaml) - add_creation_marker(script_path) true rescue StandardError => e @@ -256,11 +251,6 @@ def cache_file_exists? cache_file_path.exist? end - def add_creation_marker(script_dir) - creation_marker = Pathname.new(Script.creation_marker_path(script_dir)) - File.write(creation_marker, { }.to_json) unless creation_marker.exist? - end - def cached_values @cached_values ||= begin cache_file_path = OodAppkit.dataroot.join(Script.scripts_dir("#{project_dir}"), "#{id}_opts.json")