Fixes #3616 - Provide meaningful modal if report profile tries to use dates...

.gitlab/ci/integration/es.yml

@@ -11,7 +11,6 @@
     - bundle exec rails test test/integration/elasticsearch_active_test.rb
     - bundle exec rails test test/integration/elasticsearch_test.rb
     - bundle exec rspec --tag searchindex --tag ~type:system --profile 10
-    - bundle exec rails test test/integration/report_test.rb
 
 es:7:
   <<: *template_integration_es

app/assets/javascripts/app/controllers/_application_controller/technical_error_modal.coffee

@@ -0,0 +1,9 @@
+class App.ControllerTechnicalErrorModal extends App.ControllerModal
+  head:         "StatusCode: #{status}"
+  contentCode:  ''
+  buttonClose:  false
+  buttonSubmit: 'Ok'
+  onSubmit:     (e) -> @close(e)
+
+  content: ->
+    "<pre><code>#{@contentCode}</code></pre>"

app/assets/javascripts/app/controllers/report.coffee

@@ -171,6 +171,13 @@ class Graph extends App.Controller
         backends:  @params.backendSelected
       )
       processData: true
+      error:       (xhr) =>
+        return if !_.include([401, 403, 404, 422, 502], xhr.status)
+
+        @bodyModal = new App.ControllerTechnicalErrorModal(
+          head:        __('Cannot generate report')
+          contentCode: xhr.responseJSON.error
+        )
       success: (data) =>
         @update(data)
         @delay(@render, interval, 'report-update', 'page')

app/assets/javascripts/app/lib/app_post/ajax.coffee

@@ -102,12 +102,18 @@ class _ajaxSingleton
       # do not show any error message with code 502
       return if status is 502
 
+      try
+        json = JSON.parse(detail)
+        text = json.error_human || json.error
+
+      text = detail if !text
+
+      escaped = App.Utils.htmlEscape(text)
+
       # show error message
-      new App.ControllerModal(
-        head:          "StatusCode: #{status}"
-        contentInline: "<pre>#{App.Utils.htmlEscape(detail)}</pre>"
-        buttonClose:   true
-        buttonSubmit:  false
+      new App.ControllerTechnicalErrorModal(
+        contentCode: escaped
+        head:        "StatusCode: #{status}"
       )
     )
 

app/assets/stylesheets/zammad.scss

@@ -381,6 +381,26 @@ ul {
   z-index: 1;
 }
 
+code {
+  background: hsla(0, 0%, 0%, 0.2);
+  border-radius: 3px;
+  box-decoration-break: clone;
+}
+
+code,
+.hljs {
+  padding: 2px 4px;
+  font-size: 0.88em;
+}
+
+.hljs {
+  background: none;
+}
+
+pre code.hljs {
+  font-size: 1em;
+}
+
 pre {
   display: block;
   padding: 9.5px;
@@ -395,30 +415,25 @@ pre {
   border-radius: 3px;
 }
 
+.modal-content pre {
+  background: hsl(0, 0%, 97%);
+  border: 1px solid hsl(0, 0%, 87%);
+}
+
 pre code {
   padding: 0;
   font-size: inherit;
   color: inherit;
   white-space: pre-wrap;
-  background-color: transparent;
-  border-radius: 0;
-}
-
-.hljs,
-code {
   background: none;
-  padding: 2px 4px;
-  font-size: 0.88em;
-}
-
-code:not(.hljs) {
-  border: 1px solid rgba(0, 0, 0, 0.2);
-  border-radius: 3px;
-  white-space: nowrap;
-}
+  border-radius: 0;
+  border: none;
+  overflow-x: auto;
 
-pre code.hljs {
-  font-size: 1em;
+  &.hljs {
+    padding: 0;
+    background: none;
+  }
 }
 
 .textarea::placeholder,

app/controllers/reports_controller.rb

@@ -22,26 +22,34 @@ class ReportsController < ApplicationController
     get_params = params_all
     return if !get_params
 
-    result = {}
-    get_params[:metric][:backend].each do |backend|
-      condition = get_params[:profile].condition
-      if backend[:condition]
-        backend[:condition].merge(condition)
-      else
-        backend[:condition] = condition
+    begin
+      result = {}
+      get_params[:metric][:backend].each do |backend|
+        condition = get_params[:profile].condition
+        if backend[:condition]
+          backend[:condition].merge(condition)
+        else
+          backend[:condition] = condition
+        end
+        next if !backend[:adapter]
+
+        result[backend[:name]] = backend[:adapter].aggs(
+          range_start:     get_params[:start],
+          range_end:       get_params[:stop],
+          interval:        get_params[:range],
+          selector:        backend[:condition],
+          params:          backend[:params],
+          timezone:        get_params[:timezone],
+          timezone_offset: get_params[:timezone_offset],
+          current_user:    current_user
+        )
+      end
+    rescue => e
+      if e.message.include? 'Conflicting date range'
+        raise Exceptions::UnprocessableEntity, __('Conflicting date ranges. Please check your selected report profile.')
       end
-      next if !backend[:adapter]
 
-      result[backend[:name]] = backend[:adapter].aggs(
-        range_start:     get_params[:start],
-        range_end:       get_params[:stop],
-        interval:        get_params[:range],
-        selector:        backend[:condition],
-        params:          backend[:params],
-        timezone:        get_params[:timezone],
-        timezone_offset: get_params[:timezone_offset],
-        current_user:    current_user
-      )
+      raise e
     end
 
     render json: {

i18n/zammad.pot

@@ -1403,6 +1403,10 @@ msgstr ""
 msgid "Cannot follow-up on a closed ticket. Please create a new ticket."
 msgstr ""
 
+#: app/assets/javascripts/app/controllers/report.coffee
+msgid "Cannot generate report"
+msgstr ""
+
 #: app/assets/javascripts/app/lib/app_post/html5_upload.coffee
 msgid "Cannot upload file"
 msgstr ""
@@ -1867,6 +1871,14 @@ msgstr ""
 msgid "Confirmation failed."
 msgstr ""
 
+#: lib/search_index_backend.rb
+msgid "Conflicting date ranges"
+msgstr ""
+
+#: app/controllers/reports_controller.rb
+msgid "Conflicting date ranges. Please check your selected report profile."
+msgstr ""
+
 #: app/assets/javascripts/app/views/channel/email_account_wizard.jst.eco
 #: app/assets/javascripts/app/views/getting_started/email.jst.eco
 #: app/assets/javascripts/app/views/integration/exchange_wizard.jst.eco

lib/search_index_backend.rb

@@ -438,6 +438,8 @@ example for aggregations within one year
 
     data = selector2query(selectors, options, aggs_interval)
 
+    verify_date_range(url, data)
+
     response = make_request(url, data: data)
 
     if !response.success?
@@ -1208,4 +1210,87 @@ helper method for making HTTP calls and raising error if response was not succes
     )
   end
 
+  # verifies date range ElasticSearch payload
+  #
+  # @param url [String] of ElasticSearch
+  # @param payload [Hash] Elasticsearch query payload
+  #
+  # @return [Boolean] or raises error
+  def self.verify_date_range(url, payload)
+    ranges_payload = payload.dig(:query, :bool, :must)
+
+    return true if ranges_payload.nil?
+
+    ranges = ranges_payload
+      .select { |elem| elem.key? :range }
+      .map    { |elem| [elem[:range].keys.first, convert_es_date_range(elem)] }
+      .each_with_object({}) { |elem, sum| (sum[elem.first] ||= []) << elem.last }
+
+    return true if ranges.all? { |_, ranges_by_key| verify_single_key_range(ranges_by_key) }
+
+    error_prefix  = "Unable to process request to elasticsearch URL '#{url}'."
+    error_suffix  = "Payload:\n#{payload.to_json}"
+    error_message = __('Conflicting date ranges')
+
+    result = "#{error_prefix} #{error_message} #{error_suffix}"
+    Rails.logger.error result.first(40_000)
+
+    raise result
+  end
+
+  # checks if all ranges are overlaping
+  #
+  # @param ranges [Array<Range<DateTime>>] to use in search
+  #
+  # @return [Boolean]
+  def self.verify_single_key_range(ranges)
+    ranges
+      .each_with_index
+      .all? do |range, i|
+        ranges
+          .slice((i + 1)..)
+          .all? { |elem| elem.overlaps? range }
+      end
+  end
+
+  # Converts paylaod component to dates range
+  #
+  # @param elem [Hash] payload component
+  #
+  # @return [Range<DateTime>]
+  def self.convert_es_date_range(elem)
+    range = elem[:range].first.last
+    from  = parse_es_range_date range[:from] || range[:gt] || '-9999-01-01'
+    to    = parse_es_range_date range[:to] || range[:lt] || '9999-01-01'
+
+    from..to
+  end
+
+  # Parses absolute date or converts relative date
+  #
+  # @param input [String] string representation of date
+  #
+  # @return [Range<DateTime>]
+  def self.parse_es_range_date(input)
+    match = input.match(%r{^now(-|\+)(\d+)(\w{1})$})
+
+    return DateTime.parse input if !match
+
+    map = {
+      d: 'day',
+      y: 'year',
+      M: 'month',
+      h: 'hour',
+      m: 'minute',
+    }
+
+    range = match.captures[1].to_i.send map[match.captures[2].to_sym]
+
+    case match.captures[0]
+    when '-'
+      range.ago
+    when '+'
+      range.from_now
+    end
+  end
 end

spec/factories/report/profile.rb

@@ -6,5 +6,20 @@ FactoryBot.define do
     active { true }
     created_by_id   { 1 }
     updated_by_id   { 1 }
+
+    trait :condition_created_at do
+      transient do
+        ticket_created_at { nil }
+      end
+
+      condition do
+        {
+          'ticket.created_at' => {
+            operator: 'before (absolute)',
+            value:    ticket_created_at.iso8601
+          }
+        }
+      end
+    end
   end
 end

spec/lib/report/ticket_first_solution_spec.rb

@@ -0,0 +1,192 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+# rubocop:disable RSpec/ExampleLength
+
+require 'rails_helper'
+require 'lib/report_examples'
+
+RSpec.describe Report::TicketFirstSolution, searchindex: true do
+  include_examples 'with report examples'
+
+  describe '.aggs' do
+    it 'gets monthly aggregated results' do
+      result = described_class.aggs(
+        range_start: Time.zone.parse('2015-01-01T00:00:00Z'),
+        range_end:   Time.zone.parse('2015-12-31T23:59:59Z'),
+        interval:    'month',
+        selector:    {},
+      )
+
+      expect(result).to eq [0, 0, 0, 0, 0, 0, 0, 0, 0, 2, 1, 0]
+    end
+
+    it 'gets monthly aggregated results with high priority' do
+      result = described_class.aggs(
+        range_start: Time.zone.parse('2015-01-01T00:00:00Z'),
+        range_end:   Time.zone.parse('2015-12-31T23:59:59Z'),
+        interval:    'month',
+        selector:    {
+          'ticket.priority_id' => {
+            'operator' => 'is',
+            'value'    => [Ticket::Priority.lookup(name: '3 high').id],
+          }
+        },
+      )
+
+      expect(result).to eq [0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0]
+    end
+
+    it 'gets monthly aggregated results not in merged state' do
+      result = described_class.aggs(
+        range_start: Time.zone.parse('2015-01-01T00:00:00Z'),
+        range_end:   Time.zone.parse('2015-12-31T23:59:59Z'),
+        interval:    'month',
+        selector:    {
+          'ticket_state.name' => {
+            'operator' => 'is not',
+            'value'    => 'merged',
+          }
+        },
+      )
+
+      expect(result).to eq [0, 0, 0, 0, 0, 0, 0, 0, 0, 2, 1, 0]
+    end
+
+    it 'gets monthly aggregated results with not high priority' do
+      result = described_class.aggs(
+        range_start: Time.zone.parse('2015-01-01T00:00:00Z'),
+        range_end:   Time.zone.parse('2015-12-31T23:59:59Z'),
+        interval:    'month',
+        selector:    {
+          'ticket.priority_id' => {
+            'operator' => 'is not',
+            'value'    => [Ticket::Priority.lookup(name: '3 high').id],
+          }
+        },
+      )
+
+      expect(result).to eq [0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 0]
+    end
+
+    it 'gets weekly aggregated results' do
+      result = described_class.aggs(
+        range_start: Time.zone.parse('2015-10-26T00:00:00Z'),
+        range_end:   Time.zone.parse('2015-10-31T23:59:59Z'),
+        interval:    'week',
+        selector:    {},
+      )
+
+      expect(result).to eq [0, 0, 1, 0, 0, 1, 1]
+    end
+
+    it 'gets daily aggregated results' do
+      result = described_class.aggs(
+        range_start: Time.zone.parse('2015-10-01T00:00:00Z'),
+        range_end:   Time.zone.parse('2015-11-01T23:59:59Z'),
+        interval:    'day',
+        selector:    {},
+      )
+
+      expect(result).to eq [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 1]
+    end
+
+    it 'gets hourly aggregated results' do
+      result = described_class.aggs(
+        range_start: Time.zone.parse('2015-10-28T00:00:00Z'),
+        range_end:   Time.zone.parse('2015-10-28T23:59:59Z'),
+        interval:    'hour',
+        selector:    {},
+      )
+
+      expect(result).to eq [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
+    end
+  end
+
+  describe '.items' do
+    it 'gets items in year range' do
+      result = described_class.items(
+        range_start: Time.zone.parse('2015-01-01T00:00:00Z'),
+        range_end:   Time.zone.parse('2015-12-31T23:59:59Z'),
+        selector:    {},
+      )
+      expect(result).to match_tickets ticket_5, ticket_6, ticket_7
+    end
+
+    it 'gets items in year range with high priority' do
+      result = described_class.items(
+        range_start: Time.zone.parse('2015-01-01T00:00:00Z'),
+        range_end:   Time.zone.parse('2015-12-31T23:59:59Z'),
+        selector:    {
+          'ticket.priority_id' => {
+            'operator' => 'is',
+            'value'    => [Ticket::Priority.lookup(name: '3 high').id],
+          }
+        }
+      )
+
+      expect(result).to match_tickets ticket_5
+    end
+
+    it 'gets items in year range not in merged state' do
+      result = described_class.items(
+        range_start: Time.zone.parse('2015-01-01T00:00:00Z'),
+        range_end:   Time.zone.parse('2015-12-31T23:59:59Z'),
+        selector:    {
+          'ticket_state.name' => {
+            'operator' => 'is not',
+            'value'    => 'merged',
+          }
+        }
+      )
+
+      expect(result).to match_tickets ticket_5, ticket_6, ticket_7
+    end
+
+    it 'gets items in year range with not high priority' do
+      result = described_class.items(
+        range_start: Time.zone.parse('2015-01-01T00:00:00Z'),
+        range_end:   Time.zone.parse('2015-12-31T23:59:59Z'),
+        selector:    {
+          'ticket.priority_id' => {
+            'operator' => 'is not',
+            'value'    => [Ticket::Priority.lookup(name: '3 high').id],
+          }
+        }
+      )
+
+      expect(result).to match_tickets ticket_6, ticket_7
+    end
+
+    it 'gets items in week range' do
+      result = described_class.items(
+        range_start: Time.zone.parse('2015-10-26T00:00:00Z'),
+        range_end:   Time.zone.parse('2015-11-01T23:59:59Z'),
+        selector:    {}
+      )
+
+      expect(result).to match_tickets ticket_5, ticket_6, ticket_7
+    end
+
+    it 'gets items in day range' do
+      result = described_class.items(
+        range_start: Time.zone.parse('2015-10-01T00:00:00Z'),
+        range_end:   Time.zone.parse('2015-10-31T23:59:59Z'),
+        selector:    {}
+      )
+
+      expect(result).to match_tickets ticket_5, ticket_6
+    end
+
+    it 'gets items in hour range' do
+      result = described_class.items(
+        range_start: Time.zone.parse('2015-10-28T00:00:00Z'),
+        range_end:   Time.zone.parse('2015-10-28T23:59:59Z'),
+        interval:    'hour',
+        selector:    {},
+      )
+
+      expect(result).to match_tickets ticket_5
+    end
+  end
+end
+# rubocop:enable RSpec/ExampleLength