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

Added session relaunch feature #2529

Merged
merged 2 commits into from
Jan 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions apps/dashboard/app/helpers/application_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ def help_custom_url
ENV['OOD_DASHBOARD_HELP_CUSTOM_URL']
end

def fa_icon(icon, fa_style: 'fas', id: '', classes: 'app-icon')
def fa_icon(icon, fa_style: 'fas', id: '', classes: 'app-icon', title: "FontAwesome icon specified: #{icon}")
content_tag(:i, '', id: id, class: [fa_style, "fa-#{icon}", 'fa-fw'].concat(Array(classes)),
title: "FontAwesome icon specified: #{icon}", "aria-hidden": true)
title: title, "aria-hidden": true)
end

def app_icon_tag(app)
Expand Down
35 changes: 31 additions & 4 deletions apps/dashboard/app/helpers/batch_connect/sessions_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ def session_panel(session)
status << content_tag(:span, pluralize(num_cores, "core"), class: "badge badge-#{status_context(session)} badge-pill") unless num_cores.zero?
end
status << "#{status session}"
relaunch(status, session)
tag.span(status.join(" | ").html_safe, class: "card-text")
end
)
Expand Down Expand Up @@ -195,6 +196,28 @@ def status_context(session)
end
end

def relaunch(status_array, session)
return unless session.completed?

batch_connect_app = session.app
return unless batch_connect_app.valid?

user_context = session.user_context
params = batch_connect_app.attributes.map{|attribute| ["batch_connect_session_context[#{attribute.id}]", user_context.fetch(attribute.id, '')]}.to_h
title = "#{t('dashboard.batch_connect_sessions_relaunch_title')} #{session.title} #{t('dashboard.batch_connect_sessions_word')}"
status_array << button_to(
batch_connect_session_contexts_path(token: batch_connect_app.token),
method: :post,
class: %w[btn px-1 py-0 btn-outline-dark relaunch],
form_class: %w[d-inline relaunch],
title: title,
'aria-label': title,
data: { toggle: "tooltip", placement: "left" },
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
data: { toggle: "tooltip", placement: "left" },
data: { toggle: "tooltip", placement: "left", selector: true },

For whatever reason, we need to add this flag here because jQuery/bootstrap tooltip does something funny with the title and it throws off screen readers.

You can see in the html title is empty, replaced by data-original-title.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Well one second, that actually breaks the hover-over tooltip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that while testing, the Delete tooltip was broken.
Reading about, the selector is needed when dynamic HTML is used and it should be a String with the locator of the tooltips elements. But I could not make it work locally.

In our case, we would need the selector for the card updates done with AJAX.

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that while testing, the Delete tooltip was broken.

delete tooltip still works for me. Let's see if we can't get aria-labeledby='data-original-title' to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

This'll work for now, I'll create a follow up ticket to track tooltip buttons because I'd really like to use the title, but IDK if bs tooltips is going to let us.

'aria-label': title,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you are happy with the default HTML tooltip, we can remove the whole:
toggle: "tooltip", placement: "bottom", selector: true

This is only needed for Bootstrap tooltip.
By doing this, the default title will be available in the button element.

Screenshot 2023-01-31 at 16 17 27

Copy link
Contributor

Choose a reason for hiding this comment

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

No, let's keep the tooltip because that does tell folks what this button does. I think just adding 'aria-label': title, to that hash will give us both the tooltip for sighted users and the accessibility for screen readers.

      # ...
      title: title,
      'aria-label': title,
      data: { toggle: "tooltip", placement: "left" },
      params: params
    ) do

Copy link
Contributor

Choose a reason for hiding this comment

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

and we can fix the cancel/delete buttons in the same way.

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

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

params: params
) do
"#{fa_icon('sync', classes: nil, title: '')}".html_safe
end
end

def cancel_or_delete(session)
if Configuration.cancel_session_enabled && !session.completed?
Expand All @@ -205,24 +228,28 @@ def cancel_or_delete(session)
end

def delete(session)
title = "#{t('dashboard.batch_connect_sessions_delete_title')} #{session.title} #{t('dashboard.batch_connect_sessions_word')}"
button_to(
batch_connect_session_path(session.id),
method: :delete,
class: "btn btn-danger float-right btn-delete",
title: "#{t('dashboard.batch_connect_sessions_delete_title')} #{session.title} #{t('dashboard.batch_connect_sessions_word')}",
data: { confirm: t('dashboard.batch_connect_sessions_delete_confirm'), toggle: "tooltip", placement: "bottom", selector: true }
title: title,
'aria-label': title,
data: { confirm: t('dashboard.batch_connect_sessions_delete_confirm'), toggle: "tooltip", placement: "bottom"}
) do
"#{fa_icon('times-circle', classes: nil)} <span aria-hidden='true'>#{t('dashboard.batch_connect_sessions_delete_title')}</span>".html_safe
end
end

def cancel(session)
title = "#{t('dashboard.batch_connect_sessions_cancel_title')} #{session.title} #{t('dashboard.batch_connect_sessions_word')}"
button_to(
batch_connect_cancel_session_path(session.id),
method: :post,
class: "btn btn-danger float-right btn-cancel",
title: "#{t('dashboard.batch_connect_sessions_cancel_title')} #{session.title} #{t('dashboard.batch_connect_sessions_word')}",
data: { confirm: t('dashboard.batch_connect_sessions_cancel_confirm'), toggle: "tooltip", placement: "bottom", selector: true }
title: title,
'aria-label': title,
data: { confirm: t('dashboard.batch_connect_sessions_cancel_confirm'), toggle: "tooltip", placement: "bottom" }
) do
"#{fa_icon('times-circle', classes: nil)} <span aria-hidden='true'>#{t('dashboard.batch_connect_sessions_cancel_title')}</span>".html_safe
end
Expand Down
11 changes: 11 additions & 0 deletions apps/dashboard/app/javascript/packs/batch_connect_sessions.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,14 @@ function installSettingHandlers(name) {

window.installSettingHandlers = installSettingHandlers;
window.tryUpdateSetting = tryUpdateSetting;

jQuery(function (){
function showSpinner() {
$('body').addClass('modal-open');
$('#full-page-spinner').removeClass('d-none');
}

$('button.relaunch').each((index, element) => {
$(element).on('click', showSpinner);
});
});
2 changes: 2 additions & 0 deletions apps/dashboard/app/models/user_configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ def nav_categories

# The current user profile. Used to select the configuration properties.
def profile
return CurrentUser.user_settings[:profile_override].to_sym if CurrentUser.user_settings[:profile_override]

if Configuration.host_based_profiles
request_hostname
else
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,7 @@ locals: {
</div>
</div>
<%- end -%>
</div>
<div id="full-page-spinner" class="d-none">
<div class="spinner-border" role="status"></div>
</div>
1 change: 1 addition & 0 deletions apps/dashboard/app/views/layouts/application.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
<%= yield :head %>

<meta name="viewport" content="width=device-width, initial-scale=1">
<meta name="profile" content="<%= @user_configuration.profile %>">

<!-- configuration options exposed to javascript -->
<meta id="ood_config"
Expand Down
1 change: 1 addition & 0 deletions apps/dashboard/config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ en:
batch_connect_sessions_delete_title: "Delete"
batch_connect_sessions_cancel_confirm: "Are you sure?"
batch_connect_sessions_cancel_title: "Cancel"
batch_connect_sessions_relaunch_title: "Relaunch"
batch_connect_sessions_errors_staging: "Failed to stage the template with the following error:"
batch_connect_sessions_errors_submission: "Failed to submit session with the following error:"
batch_connect_sessions_novnc_launch: "Launch %{app_title}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,3 @@ disable_bc_shell: true
cancel_session_enabled: true
bc_clean_old_dirs: true
hide_app_version: true

Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,47 @@ class BatchConnect::SessionsHelperTest < ActionView::TestCase
assert_equal delete_session_title, button['title']
end

def create_session(state = :running)
test 'relaunch should ignore sessions when session is not completed' do
OodCore::Job::Status.states.each do |state|
next if state == :completed

status_array = []
relaunch(status_array, create_session(state))
assert_equal [], status_array
end
end

test 'relaunch should ignore sessions when session application is not valid' do
status_array = []
relaunch(status_array, create_session(:completed, valid: false))
assert_equal [], status_array
end

test 'relaunch should add relaunch form when session is completed' do
status_array = []
relaunch(status_array, create_session(:completed))

assert_equal 1, status_array.size
html = Nokogiri::HTML(status_array[0])
form = html.at_css('form')
assert_equal batch_connect_session_contexts_path(token: 'sys/token'), form['action']
assert_equal true, form['class'].include?('relaunch')

button = html.at_css('button')
assert_equal 'Relaunch AppName Session', button['title']
assert_equal true, button['class'].include?('relaunch')
end

def create_session(state = :running, valid: true)
value = '{"id":"1234","job_id":"1","created_at":1669139262,"token":"sys/token","title":"AppName","cache_completed":false}'
BatchConnect::Session.new.from_json(value).tap do |session|
session.stubs(:status).returns(OodCore::Job::Status.new(state: state))
OpenStruct.new.tap do |sys_app|
sys_app.send('valid?=', valid)
sys_app.attributes = []
sys_app.token = 'sys/token'
session.stubs(:app).returns(sys_app)
end
end
end

Expand Down
15 changes: 15 additions & 0 deletions apps/dashboard/test/integration/sessions_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -96,4 +96,19 @@ def setup
assert_equal batch_connect_cancel_session_path('1234'), form.first['action']
end
end

test 'should render session panel with relaunch button' do
value = '{"id":"1234","job_id":"1","created_at":1669139262,"token":"sys/token","title":"session title","cache_completed":true}'
session = BatchConnect::Session.new.from_json(value)
session.stubs(:status).returns(OodCore::Job::Status.new(state: :completed))
session.stubs(:app).returns(stub(valid?: true, token: 'sys/token', attributes: [], session_info_view: nil))
BatchConnect::Session.stubs(:all).returns([session])

get batch_connect_sessions_path
assert_response :success

assert_select 'div#id_1234 div.card-heading div.float-right form.relaunch' do |form|
assert_equal batch_connect_session_contexts_path(token: 'sys/token'), form.first['action']
end
end
end