From 6cdbb3da33fb0b295a74b9cef8b33bd697e4d267 Mon Sep 17 00:00:00 2001 From: Dan Wells Date: Fri, 3 May 2019 13:17:50 -0400 Subject: [PATCH] LP#1796945 Reporter cloning and creation fixes This commit addresses a variety of issues with the webstaff reporter interface, particularly cases of cloning reports created in the XUL client. 1. The conversion process did not account for manually selected JOIN operations (aka nullability). These JOINs are now honored by the conversion code. 2. The conversion process did not account for aggregate filters. These filters are now converted where present. 3. The previous reporter interface attempted to intelligently apply LEFT and INNER JOINs by default. The new interface applied INNER joins exclusively by default, leading in many cases to different results. This commit reinstates the previous logic. One side effect of this change is that the IDL tree itself is no longer opinionated about JOIN type, and the default JOIN is undefined. 4. The nullability selector has been expanded to allow for manual selection of INNER joins, as they will longer be the default in some cases. 5. Cloned-converted reports did not retain column order. The order is now preserved. 6. Some templates created in the older interface could, in some cases, have aggregate values set as the string "undefined" rather than actually being undefined. This led to converted templates failing with "column [xxx] must appear in the GROUP BY clause...", as they were incorrectly converted as aggregates. The conversion code now accounts for this latent bug. Signed-off-by: Dan Wells Signed-off-by: Jason Boyer --- .../staff/reporter/t_edit_template.tt2 | 2 +- .../ui/default/staff/reporter/template/app.js | 88 +++++++++++-------- .../web/js/ui/default/staff/services/idl.js | 1 - 3 files changed, 51 insertions(+), 40 deletions(-) diff --git a/Open-ILS/src/templates/staff/reporter/t_edit_template.tt2 b/Open-ILS/src/templates/staff/reporter/t_edit_template.tt2 index cab7bd06bb..99bdfb0e37 100644 --- a/Open-ILS/src/templates/staff/reporter/t_edit_template.tt2 +++ b/Open-ILS/src/templates/staff/reporter/t_edit_template.tt2 @@ -66,7 +66,7 @@ {{ node.label || n.id }} diff --git a/Open-ILS/web/js/ui/default/staff/reporter/template/app.js b/Open-ILS/web/js/ui/default/staff/reporter/template/app.js index 5dde15936f..c9f6c28dae 100644 --- a/Open-ILS/web/js/ui/default/staff/reporter/template/app.js +++ b/Open-ILS/web/js/ui/default/staff/reporter/template/app.js @@ -113,6 +113,7 @@ function($scope , $q , $routeParams , $location , $timeout , $window, egCore , var t = tree; var join_path = ''; + var last_jtype = ''; item.path.forEach(function (p, i, a) { var alias; // unpredictable hashes are fine for intermediate tables @@ -138,9 +139,15 @@ function($scope , $q , $routeParams , $location , $timeout , $window, egCore , t = t.join[uplink_alias]; var djtype = 'inner'; - if (p.uplink.reltype != 'has_a') djtype = 'left'; + // we use LEFT JOINs for might_have and has_many, AND + // also if our previous JOIN was a LEFT JOIN + // + // The last piece prevents later joins from limiting those + // closer to the core table + if (p.uplink.reltype != 'has_a' || last_jtype == 'left') djtype = 'left'; t.type = p.jtype || djtype; + last_jtype = t.type; t.key = p.uplink.key; } else { join_path = p.classname + '-' + p.classname; @@ -271,11 +278,16 @@ function($scope , $q , $routeParams , $location , $timeout , $window, egCore , } } + // preserve the old select order for the display cols + var sel_order = {}; + template.data.select.map(function(val, idx) { + // set key to unique value easily derived from relcache + sel_order[val.relation + val.column.colname] = idx; + }); + template.data['display_cols'] = []; template.data['filter_cols'] = []; - var dispcol_index = 0; - function _convertPath(orig, rel) { var newPath = []; @@ -291,13 +303,16 @@ function($scope , $q , $routeParams , $location , $timeout , $window, egCore , label : egCore.idl.classes[cls].label } if (prev_link != '') { - args['from'] = prev_link.split(/-/)[0]; - var prev_col = prev_link.split(/-/)[1].split(/>/)[0]; + var link_parts = prev_link.split(/-/); + args['from'] = link_parts[0]; + var join_parts = link_parts[1].split(/>/); + var prev_col = join_parts[0]; egCore.idl.classes[prev_link.split(/-/)[0]].fields.forEach(function(f) { if (prev_col == f.name) { args['link'] = f; } }); + args['jtype'] = join_parts[1]; // frequently undefined } newPath.push(egCore.idl.classTree.buildNode(cls, args)); prev_link = link; @@ -306,55 +321,52 @@ function($scope , $q , $routeParams , $location , $timeout , $window, egCore , } - rels.map(function(rel) { - for (var col in rel.fields.dis_tab) { - var orig = rel.fields.dis_tab[col]; - var display_col = { - name : orig.colname, - path : _convertPath(orig, rel), - index : dispcol_index++, - label : orig.alias, - datatype : orig.datatype, - doc_text : orig.field_doc, - transform : { - label : orig.transform_label, - transform : orig.transform, - aggregate : orig.aggregate - }, - path_label : rel.label - }; - template.data.display_cols.push(display_col); + function _buildCols(rel, tab_type, col_idx) { + if (tab_type == 'dis_tab') { + col_type = 'display_cols'; + } else { + col_type = 'filter_cols'; } - }); - rels.map(function(rel) { - for (var col in rel.fields.filter_tab) { - var orig = rel.fields.filter_tab[col]; - var filter_col = { + for (var col in rel.fields[tab_type]) { + var orig = rel.fields[tab_type][col]; + var col = { name : orig.colname, path : _convertPath(orig, rel), - index : dispcol_index++, label : orig.alias, datatype : orig.datatype, doc_text : orig.field_doc, - operator : { - op : orig.op, - label : orig.op_label - }, transform : { label : orig.transform_label, transform : orig.transform, - aggregate : orig.aggregate + aggregate : (orig.aggregate == "undefined") ? undefined : orig.aggregate // old structure sometimes has undefined as a quoted string }, path_label : rel.label }; - if ('value' in orig.op_value) { - filter_col['value'] = orig.op_value.value; + if (col_type == 'filter_cols') { + col['operator'] = { + op : orig.op, + label : orig.op_label + }; + col['index'] = col_idx++; + if ('value' in orig.op_value) { + col['value'] = orig.op_value.value; + } + } else { // display + col['index'] = sel_order[rel.alias + orig.colname]; } - template.data.filter_cols.push(filter_col); + + template.data[col_type].push(col); } + } + + rels.map(function(rel) { + _buildCols(rel, 'dis_tab'); + _buildCols(rel, 'filter_tab', template.data.filter_cols.length); + _buildCols(rel, 'aggfilter_tab', template.data.filter_cols.length); }); + template.data['display_cols'].sort(function(a, b){return a.index - b.index}); } function loadTemplate () { @@ -675,7 +687,7 @@ function($scope , $q , $routeParams , $location , $timeout , $window, egCore , $scope.selected_source = node; $scope.currentPathLabel = $scope.currentPath.map(function(n,i){ var l = n.label - if (i) l += ' (' + n.jtype + ')'; + if (i && n.jtype) l += ' (' + n.jtype + ')'; return l; }).join( ' -> ' ); angular.forEach( node.fields, function (f) { diff --git a/Open-ILS/web/js/ui/default/staff/services/idl.js b/Open-ILS/web/js/ui/default/staff/services/idl.js index 629e5958e0..78faa9883c 100644 --- a/Open-ILS/web/js/ui/default/staff/services/idl.js +++ b/Open-ILS/web/js/ui/default/staff/services/idl.js @@ -328,7 +328,6 @@ angular.module('egCoreMod') return angular.extend( args, { idl : service[cls], - jtype : 'inner', uplink : args.link, classname: cls, struct : n, -- 2.43.2