Fixes #2282 - SLA GUI - inconsistency when no checkmark is set and other SLA form element improvements

app/assets/javascripts/app/controllers/_application_controller_form.coffee

@@ -94,8 +94,8 @@ class App.ControllerForm extends App.Controller
 
     for attributeName, attribute of attributesClean
 
-      # ignore read only attributes
+      # ignore read only or not rendered attributes attributes
-      if !attribute.readonly
+      if !attribute.readonly && !attribute.skipRendering
 
         # check generic filter
         if @filter && !attribute.filter

app/assets/javascripts/app/controllers/_ui_element/sla_times.coffee

@@ -2,8 +2,7 @@
 class App.UiElement.sla_times
   @render: (attribute, params = {}) ->
 
-    # set default value
+    if !params.id && !params.first_response_time
-    if !params.first_response_time && params.first_response_time isnt 0
       params.first_response_time = 120
 
     item = $( App.view('generic/sla_times')(
@@ -33,32 +32,50 @@ class App.UiElement.sla_times
         # reset data item
         row.find('.js-timeConvertFrom').val('')
         row.find('.js-timeConvertTo').val('')
+        row.find('.help-inline').empty()
+        row.removeClass('has-error')
     )
 
     # convert hours into minutes
     item.find('.js-timeConvertFrom').bind('keyup focus blur', (e) =>
       element = $(e.target)
       inText = element.val()
+
       row = element.closest('tr')
-      dest = element.closest('td').find('.js-timeConvertTo')
-      inMinutes = @toMinutes(inText)
-      if !inMinutes
-        element.addClass('has-error')
-        dest.val('')
-      else
-        element.removeClass('has-error')
-        dest.val(inMinutes)
       row.find('.js-activateRow').prop('checked', true)
       row.addClass('is-active')
+
+      element
+        .closest('td')
+        .find('.js-timeConvertTo')
+        .val(@toMinutes(inText) || '')
+    )
+
+    # toggle row on clicking name cell
+    item.find('.js-forward-click').bind('click', (e) ->
+      $(e.currentTarget).closest('tr').find('.checkbox-replacement').click()
+    )
+
+    # focus time input on clicking surrounding cell
+    item.find('.js-focus-input').bind('click', (e) ->
+      $(e.currentTarget).find('.form-control').focus()
+    )
+
+    # show placeholder instead of 00:00
+    item.find('.js-timeConvertFrom').bind('changeTime.timepicker', (e) ->
+      if $(e.currentTarget).val() == '00:00'
+        $(e.currentTarget).val('')
     )
 
     # set initial active/inactive rows
     item.find('.js-timeConvertFrom').each(->
-      row = $(@).closest('tr').find('.js-activateRow')
+      row = $(@).closest('tr')
+      checkbox = row.find('.js-activateRow')
       if $(@).val()
-        row.prop('checked', true)
+        checkbox.prop('checked', true)
+        row.addClass('is-active')
       else
-        row.prop('checked', false)
+        checkbox.prop('checked', false)
     )
 
     item
@@ -69,6 +86,7 @@ class App.UiElement.sla_times
     minute = parseInt(hh[1])
     return if hour is NaN
     return if minute is NaN
+    return if hour is 0 and minute is 0
     (hour * 60) + minute
 
   @toText: (m) ->

app/assets/javascripts/app/models/sla.coffee

@@ -11,6 +11,9 @@ class App.Sla extends App.Model
     { name: 'created_at',     display: 'Created',         tag: 'datetime', readonly: 1 },
     { name: 'updated_by_id',  display: 'Updated by',      relation: 'User', readonly: 1 },
     { name: 'updated_at',     display: 'Updated',         tag: 'datetime', readonly: 1 },
+    { name: 'first_response_time', null: false, skipRendering: true, required_if: { 'first_response_time_enabled': ['on'] } },
+    { name: 'update_time',         null: false, skipRendering: true, required_if: { 'update_time_enabled': ['on'] } },
+    { name: 'solution_time',       null: false, skipRendering: true, required_if: { 'solution_time_enabled': ['on'] } },
   ]
   @configure_delete = true
   @configure_overview = [

app/assets/javascripts/app/views/generic/sla_times.jst.eco

@@ -6,47 +6,47 @@
       <th><%- @T('Time') %> <span class="text-muted"><%- @T('in hours') %></span>
   </thead>
   <tbody>
-    <tr>
+    <tr class="form-group">
       <td class="u-positionOrigin">
         <label class="checkbox-replacement checkbox-replacement--fullscreen dont-grey-out">
-          <input type="checkbox" class="js-activateRow" id="first_response_time">
+          <input type="checkbox" class="js-activateRow" id="first_response_time" name="first_response_time_enabled">
           <%- @Icon('checkbox', 'icon-unchecked') %>
           <%- @Icon('checkbox-checked', 'icon-checked') %>
         </label>
-      <td>
+      <td class="u-clickable js-forward-click">
-        <label class="inline-label" for="first_response_time"><%- @T('First Response Time') %></label>
+        <div><%- @T('First Response Time') %></div>
         <p class="subtle"><%- @T('Timeframe for the first response.') %></p>
-      <td>
+      <td class="u-clickable js-focus-input">
         <input type="hidden" name="first_response_time" value="<%= @first_response_time %>" class="js-timeConvertTo">
-        <input type="text" value="<%= @first_response_time_in_text %>" class="form-control--small timeframe js-timeConvertFrom"  placeholder="hh:mm" name="first_response_time_in_text">
+        <input type="text" value="<%= @first_response_time_in_text %>" class="form-control form-control--small timeframe js-timeConvertFrom"  placeholder="hh:mm" name="first_response_time_in_text" data-name="first_response_time">
 
-    <tr>
+    <tr class="form-group">
       <td class="u-positionOrigin">
         <label class="checkbox-replacement checkbox-replacement--fullscreen dont-grey-out">
-          <input type="checkbox" class="js-activateRow" id="update_time">
+          <input type="checkbox" class="js-activateRow" id="update_time" name="update_time_enabled">
           <%- @Icon('checkbox', 'icon-unchecked') %>
           <%- @Icon('checkbox-checked', 'icon-checked') %>
         </label>
-      <td>
+      <td class="u-clickable js-forward-click">
-        <label class="inline-label" for="update_time"><%- @T('Update Time') %></label>
+        <div><%- @T('Update Time') %></div>
         <p class="subtle"><%- @T('Timeframe for every following response.') %></p>
       <td>
         <input type="hidden" name="update_time" value="<%= @update_time %>" class="js-timeConvertTo">
-        <input type="text" value="<%= @update_time_in_text %>" class="form-control--small timeframe js-timeConvertFrom" placeholder="hh:mm" name="update_time_in_text">
+        <input type="text" value="<%= @update_time_in_text %>" class="form-control form-control--small timeframe js-timeConvertFrom" placeholder="hh:mm" name="update_time_in_text" data-name="update_time">
 
-    <tr>
+    <tr class="form-group">
       <td class="u-positionOrigin">
         <label class="checkbox-replacement checkbox-replacement--fullscreen dont-grey-out">
-          <input type="checkbox" id="solution_time" class="js-activateRow">
+          <input type="checkbox" id="solution_time" class="js-activateRow" name="solution_time_enabled">
           <%- @Icon('checkbox', 'icon-unchecked') %>
           <%- @Icon('checkbox-checked', 'icon-checked') %>
         </label>
-      <td>
+      <td class="u-clickable js-forward-click">
-        <label class="inline-label" for="solution_time"><%- @T('Solution Time') %></label>
+        <div><%- @T('Solution Time') %></div>
         <p class="subtle"><%- @T('Timeframe for solving the problem.') %></p>
       <td>
         <input type="hidden" name="solution_time" value="<%= @solution_time %>" class="js-timeConvertTo">
-        <input type="text" value="<%= @solution_time_in_text %>" class="form-control--small timeframe js-timeConvertFrom" placeholder="hh:mm" name="solution_time_in_text">
+        <input type="text" value="<%= @solution_time_in_text %>" class="form-control form-control--small timeframe js-timeConvertFrom" placeholder="hh:mm" name="solution_time_in_text" data-name="solution_time">
 
     </tr>
   </tbody>

app/views/tests/form_skip_rendering.html.erb

@@ -0,0 +1,21 @@
+<link rel="stylesheet" href="/assets/tests/qunit-1.21.0.css">
+<%= javascript_include_tag "/assets/tests/qunit-1.21.0.js", "/assets/tests/syn-0.14.1.js", "/assets/tests/form_skip_rendering.js", nonce: true %>
+
+<style type="text/css">
+body {
+  padding-top: 0px;
+}
+</style>
+
+<%= javascript_tag nonce: true do -%>
+<% end -%>
+
+<div id="qunit" class="u-dontfold"></div>
+
+<div>
+  <form class="form-stacked pull-left">
+    <div id="forms"></div>
+    <button type="submit" class="btn btn-primary submit">Submit</button>
+  </form>
+</div>
+

app/views/tests/form_sla_times.html.erb

@@ -0,0 +1,21 @@
+<link rel="stylesheet" href="/assets/tests/qunit-1.21.0.css">
+<%= javascript_include_tag "/assets/tests/qunit-1.21.0.js", "/assets/tests/syn-0.14.1.js", "/assets/tests/form_sla_times.js", nonce: true %>
+
+<style type="text/css">
+body {
+  padding-top: 0px;
+}
+</style>
+
+<%= javascript_tag nonce: true do -%>
+<% end -%>
+
+<div id="qunit" class="u-dontfold"></div>
+
+<div>
+  <form class="form-stacked pull-left">
+    <div id="forms"></div>
+    <button type="submit" class="btn btn-primary submit">Submit</button>
+  </form>
+</div>
+

config/routes/test.rb

@@ -20,6 +20,8 @@ Zammad::Application.routes.draw do
   match '/tests_form_column_select',          to: 'tests#form_column_select',         via: :get
   match '/tests_form_searchable_select',      to: 'tests#form_searchable_select',     via: :get
   match '/tests_form_ticket_perform_action',  to: 'tests#form_ticket_perform_action', via: :get
+  match '/tests_form_sla_times',              to: 'tests#form_sla_times',             via: :get
+  match '/tests_form_skip_rendering',         to: 'tests#form_skip_rendering',        via: :get
   match '/tests_table',                       to: 'tests#table',                      via: :get
   match '/tests_table_extended',              to: 'tests#table_extended',             via: :get
   match '/tests_html_utils',                  to: 'tests#html_utils',                 via: :get

public/assets/tests/form_extended.js

@@ -168,10 +168,13 @@ test('form checks', function() {
     priority4_id: '2',
     priority5_id: '1',
     first_response_time: '150',
+    first_response_time_enabled: 'on',
     first_response_time_in_text: '02:30',
     solution_time: '',
+    solution_time_enabled: undefined,
     solution_time_in_text: '',
     update_time: '45',
+    update_time_enabled: 'on',
     update_time_in_text: '00:45',
     working_hours: {
       mon: {
@@ -313,10 +316,13 @@ test('form checks', function() {
       },
     },
     first_response_time: '30',
+    first_response_time_enabled: 'on',
     first_response_time_in_text: '00:30',
     solution_time: '',
+    solution_time_enabled: undefined,
     solution_time_in_text: '',
     update_time: '',
+    update_time_enabled: undefined,
     update_time_in_text: '',
   }
   deepEqual(params, test_params, 'form param check')

public/assets/tests/form_skip_rendering.js

@@ -0,0 +1,19 @@
+test("form elements not rendered", function(assert) {
+  $('#forms').append('<hr><h1>form elements check</h1><form id="form1"></form>')
+
+  var el = $('#form1')
+
+  new App.ControllerForm({
+    el:        el,
+    model:     {
+      configure_attributes: [
+        { name: 'shown',  display: 'Shown',  tag: 'input' },
+        { name: 'hidden', display: 'Hidden', tag: 'input', skipRendering: true }
+      ]
+    },
+    autofocus: true
+  });
+
+  ok(el.find('input[name=shown]').get(0), 'control element is visible')
+  notOk(el.find('input[name=hidden]').get(0), 'element with skipRendering is not shown')
+});

public/assets/tests/form_sla_times.js

@@ -0,0 +1,117 @@
+test("form SLA times highlights first row and sets 2:00 by default for new item", function(assert) {
+  $('#forms').append('<hr><h1>SLA with defaults</h1><form id="form1"></form>')
+
+  var el = $('#form1')
+
+  var item = new App.Sla()
+
+  new App.ControllerForm({
+    el:     el,
+    model:  item.constructor,
+    params: item
+  });
+
+  var row = el.find('.sla_times tbody > tr:first')
+
+  ok(row.hasClass('is-active'))
+  equal(row.find('input[data-name=first_response_time]').val(), '02:00')
+
+  $('#forms').append('<hr><h1>SLA with empty times</h1><form id="form2"></form>')
+
+  var el = $('#form2')
+
+  var item = new App.Sla()
+  item.id = '123'
+
+  new App.ControllerForm({
+    el:     el,
+    model:  item.constructor,
+    params: item
+  });
+
+  var row = el.find('.sla_times tbody > tr:first')
+
+  notOk(row.hasClass('is-active'))
+  equal(row.find('input[data-name=first_response_time]').val(), '')
+});
+
+test("form SLA times highlights and shows settings accordingly", function(assert) {
+  $('#forms').append('<hr><h1>SLA with non-first time set</h1><form id="form3"></form>')
+
+  var el = $('#form3')
+
+  var item = new App.Sla()
+  item.id = '123'
+  item.update_time = 240
+
+  new App.ControllerForm({
+    el:     el,
+    model:  item.constructor,
+    params: item
+  });
+
+  var firstRow = el.find('.sla_times tbody > tr:first')
+  var secondRow = el.find('.sla_times tbody > tr:nth-child(2)')
+
+  notOk(firstRow.hasClass('is-active'))
+  equal(firstRow.find('input[data-name=first_response_time]').val(), '')
+  ok(secondRow.hasClass('is-active'))
+  equal(secondRow.find('input[data-name=update_time]').val(), '04:00')
+})
+
+test("form SLA times highlights errors when submitting empty active row", function(assert) {
+  $('#forms').append('<hr><h1>SLA error handling</h1><form id="form4"></form>')
+
+  var el = $('#form4')
+
+  var item = new App.Sla()
+  item.id = '123'
+  item.update_time = 240
+
+  new App.ControllerForm({
+    el:     el,
+    model:  item.constructor,
+    params: item
+  });
+
+  var row = el.find('.sla_times tbody > tr:nth-child(2)')
+  var input = row.find('input[data-name=update_time]')
+  input.val('').trigger('blur')
+
+  item.load(App.ControllerForm.params(el))
+
+  App.ControllerForm.validate({form: el, errors: item.validate()})
+
+  equal(input.css('border-top-color'), 'rgb(255, 0, 0)', 'highlighted as error') // checking border-color fails on Firefox
+
+  var anotherRow = el.find('.sla_times tbody > tr:nth-child(3)')
+  var anotherInput = anotherRow.find('input[data-name=update_time]')
+
+  notEqual(anotherInput.css('border-color'), 'rgb(255, 0, 0)', 'not highlighted as error')
+
+  row.find('td:nth-child(2)').click()
+  notOk(row.hasClass('is-active'), 'deactivates class by clicking on name cell)')
+
+  notEqual(input.css('border-color'), 'rgb(255, 0, 0)', 'error cleared by deactivating')
+})
+
+test("form SLA times clears field instead of 00:00", function(assert) {
+  $('#forms').append('<hr><h1>SLA placeholder instead of 00:00</h1><form id="form5"></form>')
+
+  var el = $('#form5')
+
+  var item = new App.Sla()
+
+  new App.ControllerForm({
+    el:     el,
+    model:  item.constructor,
+    params: item
+  });
+
+  var row = el.find('.sla_times tbody > tr:nth-child(2)')
+  var input = row.find('input[data-name=update_time]')
+
+  input.val('asd').blur()
+
+  equal(input.val(), '', 'shows placeholder')
+});

spec/system/js/q_unit_spec.rb

@@ -109,6 +109,14 @@ RSpec.describe 'QUnit', type: :system, authenticated: false, set_up: true, webso
     it 'Validation' do
       q_unit_tests('form_validation')
     end
+
+    it 'Skip rendering' do
+      q_unit_tests('form_skip_rendering')
+    end
+
+    it 'SLA times' do
+      q_unit_tests('form_sla_times')
+    end
   end
 
   context 'Table' do