public inbox for [email protected]help / color / mirror / Atom feed
[pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values 25+ messages / 3 participants [nested] [flat]
* [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-04-01 11:45 Surinder Kumar <[email protected]> 0 siblings, 2 replies; 25+ messages in thread From: Surinder Kumar @ 2017-04-01 11:45 UTC (permalink / raw) To: pgadmin-hackers Hi Issues fixed: 1. If a column is defined with a default modifier, there is now way to insert the row with those defaults. The column will be left blank and it will take default value automatically. 2. If a column has a not-null constraint then an error is returned and the row is not inserted. The column will be left blank The default values for new added rows will be displayed on refresh/execute. Please find attached patch and review. Thanks Surinder -- Sent via pgadmin-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers Attachments: [application/octet-stream] RM_2257.patch (7.6K, 3-RM_2257.patch) download | inline diff: diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql index 759e657..f3353d6 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql @@ -1,4 +1,5 @@ -SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype +SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype, +att.attnotnull as not_null, att.atthasdef as has_default_val FROM pg_attribute att JOIN pg_type ty ON ty.oid=atttypid JOIN pg_namespace tn ON tn.oid=ty.typnamespace diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql index 7536a9c..4f1de2a 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql @@ -1,4 +1,5 @@ -SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype +SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype, +att.attnotnull as not_null, att.atthasdef as has_default_val FROM pg_attribute att JOIN pg_type ty ON ty.oid=atttypid JOIN pg_namespace tn ON tn.oid=ty.typnamespace diff --git a/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js b/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js index b0ddfc5..fee6b4a 100644 --- a/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js +++ b/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js @@ -110,7 +110,12 @@ // When text editor opens this.loadValue = function (item) { - if (item[args.column.pos] === "") { + var col = args.column; + + if (_.isUndefined(item[args.column.pos]) && col.has_default_val) { + $input.val(""); + } + else if (item[args.column.pos] === "") { $input.val("''"); } else { @@ -120,11 +125,15 @@ }; this.serializeValue = function () { - var value = $input.val(); + var value = $input.val(), + col = args.column; // If empty return null - if (value === "") { + if (value === "" && !col.has_default_val) { return null; } + else if (value === "" && col.has_default_val) { + return ''; + } // single/double quotes represent an empty string // If found return '' else if (value === "''" || value === '""') { diff --git a/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js b/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js index b066095..c5d2057 100644 --- a/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js +++ b/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js @@ -19,7 +19,7 @@ }); function JsonFormatter(row, cell, value, columnDef, dataContext) { - if (value == null || value === "") { + if (_.isUndefined(value) || value == null || value === "") { return ""; } else { // Stringify only if it's json object @@ -42,11 +42,17 @@ } function NumbersFormatter(row, cell, value, columnDef, dataContext) { - if (_.isUndefined(value) || value === null) { - return "<span class='pull-right'>[null]</span>"; + // Don't display null if column: + // 1. value is undefined (No value entered by user) + // 2. If either column is not null or has default value. + if ( + ((_.isUndefined(value) || _.isNull(value)) && + (columnDef.has_default_val || columnDef.not_null)) + ) { + return ""; } - else if (value === "") { - return ''; + else if (_.isUndefined(value) || value === null) { + return "<span class='pull-right'>[null]</span>"; } else { return "<span style='float:right'>" + value + "</span>"; @@ -66,7 +72,16 @@ } function TextFormatter(row, cell, value, columnDef, dataContext) { - if (_.isUndefined(value) || value === null) { + // Don't display null if column: + // 1. value is undefined (No value entered by user) + // 2. If either column is not null or has default value. + if ( + ((_.isUndefined(value) || _.isNull(value)) && + (columnDef.has_default_val || columnDef.not_null)) + ) { + return ""; + } + else if (_.isUndefined(value) || value === null) { return "<span class='pull-left'>[null]</span>"; } else { diff --git a/web/pgadmin/tools/sqleditor/__init__.py b/web/pgadmin/tools/sqleditor/__init__.py index f08b02e..883080f 100644 --- a/web/pgadmin/tools/sqleditor/__init__.py +++ b/web/pgadmin/tools/sqleditor/__init__.py @@ -442,6 +442,19 @@ def get_columns(trans_id): primary_keys = None status, error_msg, conn, trans_obj, session_obj = check_transaction_status(trans_id) if status and conn is not None and session_obj is not None: + + ver = conn.manager.version + # Get the template path for the column + template_path = 'column/sql/#{0}#'.format(ver) + command_obj = pickle.loads(session_obj['command_obj']) + SQL = render_template("/".join([template_path, + 'nodes.sql']), + tid=command_obj.obj_id) + # rows with attribute not_null + status, rset = conn.execute_2darray(SQL) + if not status: + return internal_server_error(errormsg=rset) + # Check PK column info is available or not if 'primary_keys' in session_obj: primary_keys = session_obj['primary_keys'] @@ -449,10 +462,16 @@ def get_columns(trans_id): # Fetch column information columns_info = conn.get_column_info() if columns_info is not None: - for col in columns_info: + for key, col in enumerate(columns_info): col_type = dict() col_type['type_code'] = col['type_code'] col_type['type_name'] = None + col_type['not_null'] = col['not_null'] = \ + rset['rows'][key]['not_null'] + + col_type['has_default_val'] = col['has_default_val'] = \ + rset['rows'][key]['has_default_val'] + columns[col['name']] = col_type # As we changed the transaction object we need to diff --git a/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js b/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js index eed0e09..0bc3a1c 100644 --- a/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js +++ b/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js @@ -564,7 +564,9 @@ define( id: c.name, pos: c.pos, field: c.name, - name: c.label + name: c.label, + not_null: c.not_null, + has_default_val: c.has_default_val }; // Get the columns width based on data type @@ -2079,7 +2081,9 @@ define( 'label': column_label, 'cell': col_cell, 'can_edit': self.can_edit, - 'type': type + 'type': type, + 'not_null': c.not_null, + 'has_default_val': c.has_default_val }; columns.push(col); }); ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-04-04 16:37 Matthew Kleiman <[email protected]> parent: Surinder Kumar <[email protected]> 1 sibling, 1 reply; 25+ messages in thread From: Matthew Kleiman @ 2017-04-04 16:37 UTC (permalink / raw) To: Surinder Kumar <[email protected]>; +Cc: pgadmin-hackers; George Gelashvili <[email protected]> Hi Surinder, We looked at your fix for default values in the query tool editor. We think the user experience could be further improved by seeing the actual default value instead of the empty cell. As an intermediate step, there would be user value in seeing "[default]" instead of the blank cell (or what used to be "[null]"). Thanks, George and Matt On Sat, Apr 1, 2017 at 7:45 AM, Surinder Kumar < [email protected]> wrote: > Hi > > Issues fixed: > > 1. If a column is defined with a default modifier, there is now way to > insert the row with those defaults. > The column will be left blank and it will take default value automatically. > > 2. If a column has a not-null constraint then an error is returned and > the row is not inserted. > The column will be left blank > > > The default values for new added rows will be displayed on > refresh/execute. > > Please find attached patch and review. > > > Thanks > Surinder > > > -- > Sent via pgadmin-hackers mailing list ([email protected]) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgadmin-hackers > > ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-04-07 08:51 Dave Page <[email protected]> parent: Surinder Kumar <[email protected]> 1 sibling, 1 reply; 25+ messages in thread From: Dave Page @ 2017-04-07 08:51 UTC (permalink / raw) To: Surinder Kumar <[email protected]>; +Cc: pgadmin-hackers Hi On Sat, Apr 1, 2017 at 12:45 PM, Surinder Kumar <[email protected]> wrote: > Hi > > Issues fixed: > > 1. If a column is defined with a default modifier, there is now way to > insert the row with those defaults. > The column will be left blank and it will take default value automatically. > > 2. If a column has a not-null constraint then an error is returned and the > row is not inserted. > The column will be left blank > > The default values for new added rows will be displayed on refresh/execute. > > Please find attached patch and review. This largely works as expected, but there is some weirdness. I have a test table that looks like this: CREATE TABLE public.defaults ( id bigint NOT NULL DEFAULT nextval('defaults_id_seq'::regclass), data_default_nulls text COLLATE pg_catalog."default" DEFAULT 'abc123'::text, data_default_no_nulls text COLLATE pg_catalog."default" NOT NULL DEFAULT 'def456'::text, data_nulls text COLLATE pg_catalog."default", data_no_nulls text COLLATE pg_catalog."default" NOT NULL, CONSTRAINT defaults_pkey PRIMARY KEY (id) ) Remember that the expected behaviour is: - Set a value to empty to update the column to null. - Set a value to '' to update the column to an empty string - Set a value to anything else to update the column to that value 1) In a row with values in each column, if I try to set the value of data_default_nulls to null, the query executed is: UPDATE public.defaults SET data_default_nulls = '' WHERE id = '2'; 2) If I do the same in the data_nulls column, the value is immediately shown as [null] and the query executed is: UPDATE public.defaults SET data_nulls = NULL WHERE id = '2'; 3) If I then edit the value in data_default_nulls, it shows the current value as ''. Removing the quotes (to set it to null) doesn't get detected as a change. 4) When I manually executed "update defaults set data_default_nulls = null where id = 2" in a query tool window, I got: 2017-04-07 09:43:02,987: INFO werkzeug: 127.0.0.1 - - [07/Apr/2017 09:43:02] "GET /sqleditor/columns/8745675 HTTP/1.1" 500 - Traceback (most recent call last): File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py", line 2000, in __call__ return self.wsgi_app(environ, start_response) File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py", line 1991, in wsgi_app response = self.make_response(self.handle_exception(e)) File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py", line 1567, in handle_exception reraise(exc_type, exc_value, tb) File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py", line 1988, in wsgi_app response = self.full_dispatch_request() File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py", line 1641, in full_dispatch_request rv = self.handle_user_exception(e) File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py", line 1544, in handle_user_exception reraise(exc_type, exc_value, tb) File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py", line 1639, in full_dispatch_request rv = self.dispatch_request() File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py", line 1625, in dispatch_request return self.view_functions[rule.endpoint](**req.view_args) File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask_login.py", line 792, in decorated_view return func(*args, **kwargs) File "/Users/dpage/git/pgadmin4/web/pgadmin/tools/sqleditor/__init__.py", line 452, in get_columns tid=command_obj.obj_id) AttributeError: 'QueryToolCommand' object has no attribute 'obj_id' 5) When I run the query again in pgAdmin III, then refresh the data in pgAdmin 4, the data_default_nulls column is displayed without the [null] marker (despite having a null value, which I confirmed in pgAdmin 3). I'm sure there are other combinations of issues here. Please fix and thoroughly re-test to ensure behaviour is consistent - and to avoid future issues, please add some appropriate feature tests to check nulls, defaults and empty strings are properly handled in view, insert and updates. Murtuza recently wrote some feature tests for the query tool - you should be able to use those as a starting point. Thanks. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgadmin-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-04-07 08:53 Dave Page <[email protected]> parent: Matthew Kleiman <[email protected]> 0 siblings, 0 replies; 25+ messages in thread From: Dave Page @ 2017-04-07 08:53 UTC (permalink / raw) To: Matthew Kleiman <[email protected]>; +Cc: Surinder Kumar <[email protected]>; pgadmin-hackers; George Gelashvili <[email protected]> On Tue, Apr 4, 2017 at 5:37 PM, Matthew Kleiman <[email protected]> wrote: > Hi Surinder, > > We looked at your fix for default values in the query tool editor. > > We think the user experience could be further improved by seeing the actual > default value instead of the empty cell. Agreed. > As an intermediate step, there would be user value in seeing "[default]" > instead of the blank cell (or what used to be "[null]"). Also agreed. Surinder, please add the [default] label in your next update. We'll look at immediately returning values in the future, though that may not actually be possible without saving the row (defaults may not be constant, they could be expressions). -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgadmin-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-04-28 09:19 Surinder Kumar <[email protected]> parent: Dave Page <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Surinder Kumar @ 2017-04-28 09:19 UTC (permalink / raw) To: Dave Page <[email protected]>; +Cc: pgadmin-hackers Hi Dave, Please find updated patch for RM case and a separate patch for Feature tests. *Python:* - Added [default] label for cells with default values while inserting a new row. - Introduced a FieldValidator function for cells that don't allow null values. If user tries to insert null value, field with be highlighted with red borders around. - If a cell contains blank string('') and when we set it to null, the change into the cell is not detected. It was because the comparison for (defaultValue == null) return true if defaultValue is undefined. Hence _.isNull(value) is used to fix this. *Feature Test cases:* - Introduced a new method create_table_with_query(server, db_name, query) in test_utils.py which executes the given query on connected server. - Added a new file test_data.json that has test data for test cases. On Fri, Apr 7, 2017 at 2:21 PM, Dave Page <[email protected]> wrote: > Hi > > On Sat, Apr 1, 2017 at 12:45 PM, Surinder Kumar > <[email protected]> wrote: > > Hi > > > > Issues fixed: > > > > 1. If a column is defined with a default modifier, there is now way to > > insert the row with those defaults. > > The column will be left blank and it will take default value > automatically. > > > > 2. If a column has a not-null constraint then an error is returned and > the > > row is not inserted. > > The column will be left blank > > > > The default values for new added rows will be displayed on > refresh/execute. > > > > Please find attached patch and review. > > This largely works as expected, but there is some weirdness. I have a > test table that looks like this: > > CREATE TABLE public.defaults > ( > id bigint NOT NULL DEFAULT nextval('defaults_id_seq'::regclass), > data_default_nulls text COLLATE pg_catalog."default" DEFAULT > 'abc123'::text, > data_default_no_nulls text COLLATE pg_catalog."default" NOT NULL > DEFAULT 'def456'::text, > data_nulls text COLLATE pg_catalog."default", > data_no_nulls text COLLATE pg_catalog."default" NOT NULL, > CONSTRAINT defaults_pkey PRIMARY KEY (id) > ) > > Remember that the expected behaviour is: > > - Set a value to empty to update the column to null. > - Set a value to '' to update the column to an empty string > - Set a value to anything else to update the column to that value > > 1) In a row with values in each column, if I try to set the value of > data_default_nulls to null, the query executed is: > > UPDATE public.defaults SET > data_default_nulls = '' WHERE > id = '2'; > > 2) If I do the same in the data_nulls column, the value is immediately > shown as [null] and the query executed is: > > UPDATE public.defaults SET > data_nulls = NULL WHERE > id = '2'; > > 3) If I then edit the value in data_default_nulls, it shows the > current value as ''. Removing the quotes (to set it to null) doesn't > get detected as a change. > Taken care. > > 4) When I manually executed "update defaults set data_default_nulls = > null where id = 2" in a query tool window, I got: > > 2017-04-07 09:43:02,987: INFO werkzeug: 127.0.0.1 - - [07/Apr/2017 > 09:43:02] "GET /sqleditor/columns/8745675 HTTP/1.1" 500 - > Traceback (most recent call last): > File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site- > packages/flask/app.py", > line 2000, in __call__ > return self.wsgi_app(environ, start_response) > File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site- > packages/flask/app.py", > line 1991, in wsgi_app > response = self.make_response(self.handle_exception(e)) > File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site- > packages/flask/app.py", > line 1567, in handle_exception > reraise(exc_type, exc_value, tb) > File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site- > packages/flask/app.py", > line 1988, in wsgi_app > response = self.full_dispatch_request() > File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site- > packages/flask/app.py", > line 1641, in full_dispatch_request > rv = self.handle_user_exception(e) > File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site- > packages/flask/app.py", > line 1544, in handle_user_exception > reraise(exc_type, exc_value, tb) > File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site- > packages/flask/app.py", > line 1639, in full_dispatch_request > rv = self.dispatch_request() > File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site- > packages/flask/app.py", > line 1625, in dispatch_request > return self.view_functions[rule.endpoint](**req.view_args) > File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site- > packages/flask_login.py", > line 792, in decorated_view > return func(*args, **kwargs) > File "/Users/dpage/git/pgadmin4/web/pgadmin/tools/sqleditor/__ > init__.py", > line 452, in get_columns > tid=command_obj.obj_id) > AttributeError: 'QueryToolCommand' object has no attribute 'obj_id' > Fixed. > > 5) When I run the query again in pgAdmin III, then refresh the data in > pgAdmin 4, the data_default_nulls column is displayed without the > [null] marker (despite having a null value, which I confirmed in > pgAdmin 3). > Fixed. > > I'm sure there are other combinations of issues here. Please fix and > thoroughly re-test to ensure behaviour is consistent - and to avoid > future issues, please add some appropriate feature tests to check > nulls, defaults and empty strings are properly handled in view, insert > and updates. Murtuza recently wrote some feature tests for the query > tool - you should be able to use those as a starting point. > Added feature tests > > Thanks. > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > -- Sent via pgadmin-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers Attachments: [application/octet-stream] RM_2257_v1.patch (10.3K, 3-RM_2257_v1.patch) download | inline diff: diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql index 759e657..f3353d6 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql @@ -1,4 +1,5 @@ -SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype +SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype, +att.attnotnull as not_null, att.atthasdef as has_default_val FROM pg_attribute att JOIN pg_type ty ON ty.oid=atttypid JOIN pg_namespace tn ON tn.oid=ty.typnamespace diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql index 7536a9c..4f1de2a 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql @@ -1,4 +1,5 @@ -SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype +SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype, +att.attnotnull as not_null, att.atthasdef as has_default_val FROM pg_attribute att JOIN pg_type ty ON ty.oid=atttypid JOIN pg_namespace tn ON tn.oid=ty.typnamespace diff --git a/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js b/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js index cdfba4d..0505f40 100644 --- a/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js +++ b/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js @@ -110,7 +110,12 @@ // When text editor opens this.loadValue = function (item) { - if (item[args.column.pos] === "") { + var col = args.column; + + if (_.isUndefined(item[args.column.pos]) && col.has_default_val) { + $input.val(""); + } + else if (item[args.column.pos] === "") { $input.val("''"); } else { @@ -145,7 +150,10 @@ }; this.isValueChanged = function () { - return (!($input.val() == "" && defaultValue == null)) && ($input.val() != defaultValue); + // Use _.isNull(value) for comparison for null instead of + // defaultValue == null, because it returns true for undefined value. + return (!($input.val() == "" && _.isNull(defaultValue))) && + ($input.val() != defaultValue); }; this.validate = function () { diff --git a/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js b/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js index 290bddd..bfd66e8 100644 --- a/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js +++ b/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js @@ -66,7 +66,14 @@ } function TextFormatter(row, cell, value, columnDef, dataContext) { - if (_.isUndefined(value) || value === null) { + // If column has default value, set placeholder + if (_.isUndefined(value) && columnDef.has_default_val) { + return "<span class='pull-left'>[default]</span>"; + } + else if (_.isUndefined(value) && columnDef.not_null) { + return ''; // If null value not allowed, set cell to blank + } + else if (_.isUndefined(value) || _.isNull(value)) { return "<span class='pull-left'>[null]</span>"; } else { diff --git a/web/pgadmin/tools/sqleditor/__init__.py b/web/pgadmin/tools/sqleditor/__init__.py index d114988..f7466d8 100644 --- a/web/pgadmin/tools/sqleditor/__init__.py +++ b/web/pgadmin/tools/sqleditor/__init__.py @@ -440,8 +440,23 @@ def get_columns(trans_id): columns = dict() columns_info = None primary_keys = None + rset = None status, error_msg, conn, trans_obj, session_obj = check_transaction_status(trans_id) if status and conn is not None and session_obj is not None: + + ver = conn.manager.version + # Get the template path for the column + template_path = 'column/sql/#{0}#'.format(ver) + command_obj = pickle.loads(session_obj['command_obj']) + if hasattr(command_obj, 'obj_id'): + SQL = render_template("/".join([template_path, + 'nodes.sql']), + tid=command_obj.obj_id) + # rows with attribute not_null + status, rset = conn.execute_2darray(SQL) + if not status: + return internal_server_error(errormsg=rset) + # Check PK column info is available or not if 'primary_keys' in session_obj: primary_keys = session_obj['primary_keys'] @@ -449,10 +464,17 @@ def get_columns(trans_id): # Fetch column information columns_info = conn.get_column_info() if columns_info is not None: - for col in columns_info: + for key, col in enumerate(columns_info): col_type = dict() col_type['type_code'] = col['type_code'] col_type['type_name'] = None + if rset: + col_type['not_null'] = col['not_null'] = \ + rset['rows'][key]['not_null'] + + col_type['has_default_val'] = col['has_default_val'] = \ + rset['rows'][key]['has_default_val'] + columns[col['name']] = col_type # As we changed the transaction object we need to @@ -602,6 +624,7 @@ def save(trans_id): status, error_msg, conn, trans_obj, session_obj = check_transaction_status(trans_id) if status and conn is not None \ and trans_obj is not None and session_obj is not None: + setattr(trans_obj, 'columns_info', session_obj['columns_info']) # If there is no primary key found then return from the function. if len(session_obj['primary_keys']) <= 0 or len(changed_data) <= 0: diff --git a/web/pgadmin/tools/sqleditor/command.py b/web/pgadmin/tools/sqleditor/command.py index be7f21f..a9ff617 100644 --- a/web/pgadmin/tools/sqleditor/command.py +++ b/web/pgadmin/tools/sqleditor/command.py @@ -442,6 +442,23 @@ class TableCommand(GridCommand): # For newly added rows if of_type == 'added': + + # When new rows are added, only changed columns data is + # sent from client side. But if column is not_null and has + # no_default_value, set column to blank, instead + # of not null which is set by default. + column_data = {} + column_type = {} + for each_col in self.columns_info: + if ( + self.columns_info[each_col]['not_null'] and + not self.columns_info[each_col][ + 'has_default_val'] + ): + column_data[each_col] = "" + column_type[each_col] =\ + self.columns_info[each_col]['type_name'] + for each_row in changed_data[of_type]: data = changed_data[of_type][each_row]['data'] # Remove our unique tracking key @@ -450,12 +467,18 @@ class TableCommand(GridCommand): data_type = set_column_names(changed_data[of_type][each_row]['data_type']) list_of_rowid.append(data.get('__temp_PK')) + # Update columns value and data type + # with columns having not_null=False and has + # no default value + column_data.update(data) + column_type.update(data_type) + sql = render_template("/".join([self.sql_path, 'insert.sql']), - data_to_be_saved=data, + data_to_be_saved=column_data, primary_keys=None, object_name=self.object_name, nsp_name=self.nsp_name, - data_type=data_type) + data_type=column_type) list_of_sql.append(sql) # For updated rows diff --git a/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js b/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js index 2062aa2..ad9a565 100644 --- a/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js +++ b/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js @@ -526,6 +526,13 @@ define( render_grid: function(collection, columns, is_editable) { var self = this; + var requiredFieldValidator = function (value) { + if (value == null || value == undefined || !value.length) { + return {valid: false, msg: "This is a required field"}; + } else { + return {valid: true, msg: null}; + } + } // This will work as data store and holds all the // inserted/updated/deleted data from grid self.handler.data_store = { @@ -557,7 +564,10 @@ define( id: c.name, pos: c.pos, field: c.name, - name: c.label + name: c.label, + not_null: c.not_null, + has_default_val: c.has_default_val, + validator: c.not_null ? requiredFieldValidator : null }; // Get the columns width based on data type @@ -2077,7 +2087,9 @@ define( 'label': column_label, 'cell': col_cell, 'can_edit': self.can_edit, - 'type': type + 'type': type, + 'not_null': c.not_null, + 'has_default_val': c.has_default_val }; columns.push(col); }); [application/octet-stream] features_test_cases_RM_2257.patch (15.7K, 4-features_test_cases_RM_2257.patch) download | inline diff: diff --git a/web/pgadmin/feature_tests/test_data.json b/web/pgadmin/feature_tests/test_data.json new file mode 100644 index 0000000..7100048 --- /dev/null +++ b/web/pgadmin/feature_tests/test_data.json @@ -0,0 +1,23 @@ +{ + "table_insert_update_cases": { + "insert": { + "insert_with_defaults": { + "id": "1", + "data_default_nulls": "abc123", + "data_default_no_nulls": "def456" + } + }, + "update": { + "update_with_null_value": { + "id": "3", + "data_default_nulls": "", + "data_nulls": "" + }, + "update_with_empty_string": { + "id": "2", + "data_default_nulls": "''", + "data_nulls": "''" + } + } + } +} \ No newline at end of file diff --git a/web/pgadmin/feature_tests/view_data_dml_queries.py b/web/pgadmin/feature_tests/view_data_dml_queries.py new file mode 100644 index 0000000..5c45071 --- /dev/null +++ b/web/pgadmin/feature_tests/view_data_dml_queries.py @@ -0,0 +1,301 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2017, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +import json +import os +from selenium.webdriver import ActionChains +from regression.python_test_utils import test_utils +from regression.feature_utils.base_feature_test import BaseFeatureTest +import time +from selenium.webdriver.common.keys import Keys + + +CURRENT_PATH = os.path.dirname(os.path.realpath(__file__)) + +try: + with open(CURRENT_PATH + '/test_data.json') as data_file: + config_data = json.load(data_file)['table_insert_update_cases'] +except Exception as e: + print(str(e)) + + +class CheckForViewDataTest(BaseFeatureTest): + """ + Test cases to validate insert, update operations in table + with input test data + + First of all, the test data is inserted/updated into table and then + inserted data is compared with original data to check if expected data + is returned from table or not. + + We will cover test cases for, + 1) Insert with default values + 2) Update with null values + 3) Update with blank string + 4) Delete table row + """ + + scenarios = [ + ("Validate Insert, Update operations in View data with given test " + "data", + dict()) + ] + + # To create column id with nextval, first a sequence must be created. + create_sequence = """ +CREATE SEQUENCE public.defaults_id_seq + INCREMENT 1 + START 9 + MINVALUE 1 + MAXVALUE 9223372036854775807 + CACHE 1; + +ALTER SEQUENCE public.defaults_id_seq + OWNER TO postgres; + """ + + # query for creating 'defaults' table + create_table_query = """ +CREATE TABLE public.defaults +( + id bigint NOT NULL DEFAULT nextval('defaults_id_seq'::regclass), + data_default_nulls text COLLATE pg_catalog."default" DEFAULT 'abc123'::text, + data_default_no_nulls text COLLATE pg_catalog."default" NOT NULL +DEFAULT 'def456'::text, + data_nulls text COLLATE pg_catalog."default", + data_no_nulls text COLLATE pg_catalog."default" NOT NULL, + CONSTRAINT defaults_pkey PRIMARY KEY (id) +) + """ + + def before(self): + connection = test_utils.get_db_connection(self.server['db'], + self.server['username'], + self.server['db_password'], + self.server['host'], + self.server['port']) + test_utils.drop_database(connection, "acceptance_test_db") + test_utils.create_database(self.server, "acceptance_test_db") + test_utils.create_table_with_query( + self.server, + "acceptance_test_db", + CheckForViewDataTest.create_sequence) + + test_utils.create_table_with_query( + self.server, + "acceptance_test_db", + CheckForViewDataTest.create_table_query) + + def runTest(self): + self.page.wait_for_spinner_to_disappear() + self._connects_to_server() + self._tables_node_expandable() + + # Open Object -> View data + self._check_xss_in_view_data() + + # Run test to insert a new row in table with default values + self._insert_row_in_table() + + # Run test to update a row in table with null values + self._update_row_in_table() + + # Run test case to remove existed row + self._remove_row() + + def after(self): + time.sleep(1) + self.page.remove_server(self.server) + connection = test_utils.get_db_connection(self.server['db'], + self.server['username'], + self.server['db_password'], + self.server['host'], + self.server['port']) + test_utils.drop_database(connection, "acceptance_test_db") + + def _connects_to_server(self): + self.page.find_by_xpath("//*[@class='aciTreeText' and .='Servers']").click() + self.page.driver.find_element_by_link_text("Object").click() + ActionChains(self.page.driver) \ + .move_to_element(self.page.driver.find_element_by_link_text("Create")) \ + .perform() + self.page.find_by_partial_link_text("Server...").click() + + server_config = self.server + self.page.fill_input_by_field_name("name", server_config['name']) + self.page.find_by_partial_link_text("Connection").click() + self.page.fill_input_by_field_name("host", server_config['host']) + self.page.fill_input_by_field_name("port", server_config['port']) + self.page.fill_input_by_field_name("username", server_config['username']) + self.page.fill_input_by_field_name("password", server_config['db_password']) + self.page.find_by_xpath("//button[contains(.,'Save')]").click() + + def _tables_node_expandable(self): + self.page.toggle_open_tree_item(self.server['name']) + self.page.toggle_open_tree_item('Databases') + self.page.toggle_open_tree_item('acceptance_test_db') + self.page.toggle_open_tree_item('Schemas') + self.page.toggle_open_tree_item('public') + self.page.toggle_open_tree_item('Tables') + self.page.select_tree_item("defaults") + + def _check_xss_in_view_data(self): + self.page.driver.find_element_by_link_text("Object").click() + ActionChains(self.page.driver) \ + .move_to_element( + self.page.driver.find_element_by_link_text("View Data")) \ + .perform() + self.page.find_by_partial_link_text("View All Rows").click() + time.sleep(3) + self.page.driver.switch_to.frame( + self.page.driver.find_element_by_tag_name('iframe') + ) + + def _insert_row_in_table(self): + xpath_col1 = "//div[contains(@class, 'new-row')]//div[" \ + "contains(@class, 'r1')]" + time.sleep(1) + new_row = self.page.find_by_xpath(xpath_col1) + new_row.click() + field = new_row.find_element_by_xpath(".//input") + field.click() + ActionChains(self.driver).send_keys( + config_data['insert']['insert_with_defaults']['id'] + ).perform() + field.send_keys(Keys.TAB) + self.page.find_by_id("btn-save").click() + self._verify_insert_data() + + def _verify_insert_data(self): + time.sleep(0.5) + self.page.find_by_id("btn-flash").click() + time.sleep(1) + main_el = self.page.find_by_xpath('//*[@id="datagrid"]') + cell1 = main_el.find_element_by_xpath( + './/div[contains(@class, "r1")]//span' + ).get_attribute('innerHTML') + cell2 = main_el.find_element_by_xpath( + './/div[contains(@class, "r2")]' + ).get_attribute('innerHTML') + cell3 = main_el.find_element_by_xpath( + './/div[contains(@class, "r3")]' + ).get_attribute('innerHTML') + + test_verify_data = config_data['insert']['insert_with_defaults'] + + # compare updated cell values with original values + self.assertEquals(cell1, test_verify_data['id']) + self.assertEquals(cell2, test_verify_data['data_default_nulls']) + self.assertEquals(cell3, test_verify_data['data_default_no_nulls']) + + def _update_row_in_table(self): + xpath_cell1 = "//div[contains(@class, 'even')]//div[" \ + "contains(@class, 'r1')]" + xpath_cell2 = "//div[contains(@class, 'even')]//div[" \ + "contains(@class, 'r2')]" + xpath_cell3 = "//div[contains(@class, 'even')]//div[" \ + "contains(@class, 'r4')]" + for value in config_data['update']: + cell1 = config_data['update'][value]['id'] + cell2 = config_data['update'][value]['data_default_nulls'] + cell3 = config_data['update'][value]['data_nulls'] + + # Search cell 1 and update with given data + time.sleep(2) + cell1_el = self.page.find_by_xpath(xpath_cell1) + ActionChains(self.driver).move_to_element(cell1_el).double_click( + cell1_el + ).perform() + field = cell1_el.find_element_by_xpath(".//input") + field.clear() + field.click() + ActionChains(self.driver).send_keys(cell1).perform() + field.send_keys(Keys.TAB) # Press tab key + + # Search cell 2 and update with given data + cell2_el = self.page.find_by_xpath(xpath_cell2) + ActionChains(self.driver).move_to_element(cell2_el).double_click( + cell2_el).perform() + field = self.page.driver.find_element_by_css_selector( + "div[style*='z-index: 1000'] textarea" + ) + field.clear() + field.click() + time.sleep(1) + ActionChains(self.driver).send_keys(cell2).perform() + time.sleep(1) + self.page.driver.find_element_by_css_selector( + "div[style*='z-index: 1000'] div button:first-child" + ).click() # Click on editor's Save button + + # Search cell 3 and update with given data + cell3_el = self.page.find_by_xpath(xpath_cell3) + ActionChains(self.driver).move_to_element(cell3_el).double_click( + cell3_el).perform() + field = self.page.driver.find_element_by_css_selector( + "div[style*='z-index: 1000'] textarea" + ) + field.clear() + field.click() + time.sleep(0.5) + ActionChains(self.driver).send_keys(cell3).perform() + time.sleep(0.5) + self.page.driver.find_element_by_css_selector( + "div[style*='z-index: 1000'] div button:first-child" + ).click() # Click on editor's Save button + self.page.find_by_id("btn-save").click() # Save data + self._verify_update_data(value) # Verify updated data with original + + def _verify_update_data(self, test_case): + time.sleep(0.5) + self.page.find_by_id("btn-flash").click() + time.sleep(1) + main_el = self.page.find_by_xpath('//*[@id="datagrid"]') + cell1 = main_el.find_element_by_xpath( + './/div[contains(@class, "r1")]//span' + ).get_attribute('innerHTML') + + from selenium.common.exceptions import NoSuchElementException + try: + cell2 = main_el.find_element_by_xpath( + './/div[contains(@class, "r2")]//span' + ).get_attribute('innerHTML') + except NoSuchElementException: + # if [null] not found, then it is an empty string + cell2 = "''" + + try: + cell4 = main_el.find_element_by_xpath( + './/div[contains(@class, "r4")]//span' + ).get_attribute('innerHTML') + except NoSuchElementException: + # if [null] not found, then it is an empty string + cell4 = "''" + + # Handle cells having [null] values + # If cell has [null] replace it with '' single quotes + cell1 = '' if cell1 == '[null]' else cell1 + cell2 = '' if cell2 == '[null]' else cell2 + cell4 = '' if cell4 == '[null]' else cell4 + + test_verify_data = config_data['update'][test_case] + + # compare updated cell values with original values + self.assertEquals(cell1, test_verify_data['id']) + self.assertEquals(cell2, test_verify_data['data_default_nulls']) + self.assertEquals(cell4, test_verify_data['data_nulls']) + + def _remove_row(self): + xpath_col0 = "//div[contains(@class, 'ui-widget')]//div[" \ + "contains(@class, 'r0')]" + self.page.find_by_xpath(xpath_col0).click() + time.sleep(1) + self.page.find_by_xpath('//*[@id="btn-toolbar"]//button[' + '@id="btn-delete-row"]').click() + self.page.click_modal_ok('Yes') diff --git a/web/regression/feature_utils/pgadmin_page.py b/web/regression/feature_utils/pgadmin_page.py index f5d0ac7..4bb7d0f 100644 --- a/web/regression/feature_utils/pgadmin_page.py +++ b/web/regression/feature_utils/pgadmin_page.py @@ -33,11 +33,11 @@ class PgadminPage: self.click_modal_ok() self.wait_for_reloading_indicator_to_disappear() - def click_modal_ok(self): + def click_modal_ok(self, btn_label='OK'): time.sleep(0.5) # Find active alertify dialog in case of multiple alertify dialog & click on that dialog self.click_element( - self.find_by_xpath("//div[contains(@class, 'alertify') and not(contains(@class, 'ajs-hidden'))]//button[.='OK']") + self.find_by_xpath("//*//div[contains(@class, 'alertify') and not(contains(@class, 'ajs-hidden'))]//button[.='{0}']".format(btn_label)) ) def add_server(self, server_config): diff --git a/web/regression/python_test_utils/test_utils.py b/web/regression/python_test_utils/test_utils.py index 2b7c695..c50dd31 100644 --- a/web/regression/python_test_utils/test_utils.py +++ b/web/regression/python_test_utils/test_utils.py @@ -172,6 +172,35 @@ def create_table(server, db_name, table_name): except Exception: traceback.print_exc(file=sys.stderr) + +def create_table_with_query(server, db_name, query): + """ + This function create the table in given database name + :param server: server details + :type server: dict + :param db_name: database name + :type db_name: str + :param query: create table query + :type query: str + :return: None + """ + try: + connection = get_db_connection(db_name, + server['username'], + server['db_password'], + server['host'], + server['port']) + old_isolation_level = connection.isolation_level + connection.set_isolation_level(0) + pg_cursor = connection.cursor() + pg_cursor.execute(query) + connection.set_isolation_level(old_isolation_level) + connection.commit() + + except Exception: + traceback.print_exc(file=sys.stderr) + + def create_constraint( server, db_name, table_name, constraint_type="unique", constraint_name="test_unique"): @@ -274,7 +303,6 @@ def drop_database(connection, database_name): connection.commit() connection.close() - def drop_tablespace(connection): """This function used to drop the tablespace""" pg_cursor = connection.cursor() ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-02 11:51 Dave Page <[email protected]> parent: Surinder Kumar <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Dave Page @ 2017-05-02 11:51 UTC (permalink / raw) To: Surinder Kumar <[email protected]>; +Cc: pgadmin-hackers Hi, This is looking much better now :-). Couple of thoughts and a bug: - Only the TextFormatter seems to handle both [null] and [default] values. Shouldn't all formatters do so (including Json and Checkmark)? For example, "serial" columns currently get displayed as [null] when left blank, but I would expect to see [default]. - I would suggest we put [null] and [default] in a lighter colour - #999999. - With the feature test patch added, I seem to be consistently getting the following failure (immediately after your new tests run): ====================================================================== ERROR: runTest (pgadmin.feature_tests.xss_checks_panels_and_query_tool_test.CheckForXssFeatureTest) Test XSS check for panels and query tool ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", line 42, in setUp self._screenshot() File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", line 92, in _screenshot python_version)) File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", line 802, in get_screenshot_as_file png = self.get_screenshot_as_png() File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", line 821, in get_screenshot_as_png return base64.b64decode(self.get_screenshot_as_base64().encode('ascii')) File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", line 831, in get_screenshot_as_base64 return self.execute(Command.SCREENSHOT)['value'] File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", line 238, in execute self.error_handler.check_response(response) File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/errorhandler.py", line 193, in check_response raise exception_class(message, screen, stacktrace) UnexpectedAlertPresentException: Alert Text: None Message: unexpected alert open: {Alert text : Are you sure you wish to close the pgAdmin 4 browser?} (Session info: chrome=58.0.3029.81) (Driver info: chromedriver=2.29.461585 (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac OS X 10.12.3 x86_64) ====================================================================== ERROR: runTest (pgadmin.feature_tests.xss_checks_pgadmin_debugger_test.CheckDebuggerForXssFeatureTest) Test table DDL generation ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", line 42, in setUp self._screenshot() File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", line 92, in _screenshot python_version)) File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", line 802, in get_screenshot_as_file png = self.get_screenshot_as_png() File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", line 821, in get_screenshot_as_png return base64.b64decode(self.get_screenshot_as_base64().encode('ascii')) File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", line 831, in get_screenshot_as_base64 return self.execute(Command.SCREENSHOT)['value'] File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", line 238, in execute self.error_handler.check_response(response) File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/selenium/webdriver/remote/errorhandler.py", line 193, in check_response raise exception_class(message, screen, stacktrace) UnexpectedAlertPresentException: Alert Text: None Message: unexpected alert open: {Alert text : Are you sure you wish to close the pgAdmin 4 browser?} (Session info: chrome=58.0.3029.81) (Driver info: chromedriver=2.29.461585 (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac OS X 10.12.3 x86_64) Thanks! On Fri, Apr 28, 2017 at 10:19 AM, Surinder Kumar < [email protected]> wrote: > Hi Dave, > > Please find updated patch for RM case and a separate patch for Feature > tests. > > *Python:* > > - Added [default] label for cells with default values while inserting a > new row. > > - Introduced a FieldValidator function for cells that don't allow null > values. If user tries to insert null value, field with be highlighted with > red borders around. > > - > If a cell contains blank string('') and when we set it to null, the change > into the cell is not detected. It was because the comparison > for (defaultValue == null) return true if defaultValue is undefined. Hence > _.isNull(value) is used to fix this. > > *Feature Test cases:* > > - Introduced a new method create_table_with_query(server, db_name, query) > in test_utils.py which executes the given query on connected server. > > - Added a new file test_data.json that has test data for test cases. > > > On Fri, Apr 7, 2017 at 2:21 PM, Dave Page <[email protected]> wrote: > >> Hi >> >> On Sat, Apr 1, 2017 at 12:45 PM, Surinder Kumar >> <[email protected]> wrote: >> > Hi >> > >> > Issues fixed: >> > >> > 1. If a column is defined with a default modifier, there is now way to >> > insert the row with those defaults. >> > The column will be left blank and it will take default value >> automatically. >> > >> > 2. If a column has a not-null constraint then an error is returned and >> the >> > row is not inserted. >> > The column will be left blank >> > >> > The default values for new added rows will be displayed on >> refresh/execute. >> > >> > Please find attached patch and review. >> >> This largely works as expected, but there is some weirdness. I have a >> test table that looks like this: >> >> CREATE TABLE public.defaults >> ( >> id bigint NOT NULL DEFAULT nextval('defaults_id_seq'::regclass), >> data_default_nulls text COLLATE pg_catalog."default" DEFAULT >> 'abc123'::text, >> data_default_no_nulls text COLLATE pg_catalog."default" NOT NULL >> DEFAULT 'def456'::text, >> data_nulls text COLLATE pg_catalog."default", >> data_no_nulls text COLLATE pg_catalog."default" NOT NULL, >> CONSTRAINT defaults_pkey PRIMARY KEY (id) >> ) >> >> Remember that the expected behaviour is: >> >> - Set a value to empty to update the column to null. >> - Set a value to '' to update the column to an empty string >> - Set a value to anything else to update the column to that value >> >> 1) In a row with values in each column, if I try to set the value of >> data_default_nulls to null, the query executed is: >> >> UPDATE public.defaults SET >> data_default_nulls = '' WHERE >> id = '2'; >> >> 2) If I do the same in the data_nulls column, the value is immediately >> shown as [null] and the query executed is: >> >> UPDATE public.defaults SET >> data_nulls = NULL WHERE >> id = '2'; >> >> 3) If I then edit the value in data_default_nulls, it shows the >> current value as ''. Removing the quotes (to set it to null) doesn't >> get detected as a change. >> > Taken care. > >> >> 4) When I manually executed "update defaults set data_default_nulls = >> null where id = 2" in a query tool window, I got: >> >> 2017-04-07 09:43:02,987: INFO werkzeug: 127.0.0.1 - - [07/Apr/2017 >> 09:43:02] "GET /sqleditor/columns/8745675 HTTP/1.1" 500 - >> Traceback (most recent call last): >> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >> ges/flask/app.py", >> line 2000, in __call__ >> return self.wsgi_app(environ, start_response) >> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >> ges/flask/app.py", >> line 1991, in wsgi_app >> response = self.make_response(self.handle_exception(e)) >> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >> ges/flask/app.py", >> line 1567, in handle_exception >> reraise(exc_type, exc_value, tb) >> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >> ges/flask/app.py", >> line 1988, in wsgi_app >> response = self.full_dispatch_request() >> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >> ges/flask/app.py", >> line 1641, in full_dispatch_request >> rv = self.handle_user_exception(e) >> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >> ges/flask/app.py", >> line 1544, in handle_user_exception >> reraise(exc_type, exc_value, tb) >> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >> ges/flask/app.py", >> line 1639, in full_dispatch_request >> rv = self.dispatch_request() >> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >> ges/flask/app.py", >> line 1625, in dispatch_request >> return self.view_functions[rule.endpoint](**req.view_args) >> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >> ges/flask_login.py", >> line 792, in decorated_view >> return func(*args, **kwargs) >> File "/Users/dpage/git/pgadmin4/web/pgadmin/tools/sqleditor/__ini >> t__.py", >> line 452, in get_columns >> tid=command_obj.obj_id) >> AttributeError: 'QueryToolCommand' object has no attribute 'obj_id' >> > Fixed. > >> >> 5) When I run the query again in pgAdmin III, then refresh the data in >> pgAdmin 4, the data_default_nulls column is displayed without the >> [null] marker (despite having a null value, which I confirmed in >> pgAdmin 3). >> > Fixed. > >> >> I'm sure there are other combinations of issues here. Please fix and >> thoroughly re-test to ensure behaviour is consistent - and to avoid >> future issues, please add some appropriate feature tests to check >> nulls, defaults and empty strings are properly handled in view, insert >> and updates. Murtuza recently wrote some feature tests for the query >> tool - you should be able to use those as a starting point. >> > Added feature tests > >> >> Thanks. >> >> -- >> Dave Page >> Blog: http://pgsnake.blogspot.com >> Twitter: @pgsnake >> >> EnterpriseDB UK: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-02 12:27 Surinder Kumar <[email protected]> parent: Dave Page <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Surinder Kumar @ 2017-05-02 12:27 UTC (permalink / raw) To: Dave Page <[email protected]>; +Cc: pgadmin-hackers Hi Dave, On Tue, May 2, 2017 at 5:21 PM, Dave Page <[email protected]> wrote: > Hi, > > This is looking much better now :-). Couple of thoughts and a bug: > > - Only the TextFormatter seems to handle both [null] and [default] values. > Shouldn't all formatters do so (including Json and Checkmark)? > Yes, I will apply the same changes for other formatters too. > For example, "serial" columns currently get displayed as [null] when left > blank, but I would expect to see [default]. > > - I would suggest we put [null] and [default] in a lighter colour - > #999999. > > - With the feature test patch added, I seem to be consistently getting the > following failure (immediately after your new tests run): > > ====================================================================== > ERROR: runTest (pgadmin.feature_tests.xss_checks_panels_and_query_tool_ > test.CheckForXssFeatureTest) > Test XSS check for panels and query tool > ---------------------------------------------------------------------- > Traceback (most recent call last): > File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", > line 42, in setUp > self._screenshot() > File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", > line 92, in _screenshot > python_version)) > File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site- > packages/selenium/webdriver/remote/webdriver.py", line 802, in > get_screenshot_as_file > png = self.get_screenshot_as_png() > File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site- > packages/selenium/webdriver/remote/webdriver.py", line 821, in > get_screenshot_as_png > return base64.b64decode(self.get_screenshot_as_base64().encode( > 'ascii')) > File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site- > packages/selenium/webdriver/remote/webdriver.py", line 831, in > get_screenshot_as_base64 > return self.execute(Command.SCREENSHOT)['value'] > File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site- > packages/selenium/webdriver/remote/webdriver.py", line 238, in execute > self.error_handler.check_response(response) > File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site- > packages/selenium/webdriver/remote/errorhandler.py", line 193, in > check_response > raise exception_class(message, screen, stacktrace) > UnexpectedAlertPresentException: Alert Text: None > Message: unexpected alert open: {Alert text : Are you sure you wish to > close the pgAdmin 4 browser?} > (Session info: chrome=58.0.3029.81) > (Driver info: chromedriver=2.29.461585 (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac > OS X 10.12.3 x86_64) > > > ====================================================================== > ERROR: runTest (pgadmin.feature_tests.xss_checks_pgadmin_debugger_test. > CheckDebuggerForXssFeatureTest) > Test table DDL generation > ---------------------------------------------------------------------- > Traceback (most recent call last): > File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", > line 42, in setUp > self._screenshot() > File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", > line 92, in _screenshot > python_version)) > File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site- > packages/selenium/webdriver/remote/webdriver.py", line 802, in > get_screenshot_as_file > png = self.get_screenshot_as_png() > File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site- > packages/selenium/webdriver/remote/webdriver.py", line 821, in > get_screenshot_as_png > return base64.b64decode(self.get_screenshot_as_base64().encode( > 'ascii')) > File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site- > packages/selenium/webdriver/remote/webdriver.py", line 831, in > get_screenshot_as_base64 > return self.execute(Command.SCREENSHOT)['value'] > File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site- > packages/selenium/webdriver/remote/webdriver.py", line 238, in execute > self.error_handler.check_response(response) > File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site- > packages/selenium/webdriver/remote/errorhandler.py", line 193, in > check_response > raise exception_class(message, screen, stacktrace) > UnexpectedAlertPresentException: Alert Text: None > Message: unexpected alert open: {Alert text : Are you sure you wish to > close the pgAdmin 4 browser?} > (Session info: chrome=58.0.3029.81) > (Driver info: chromedriver=2.29.461585 (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac > OS X 10.12.3 x86_64) > Sure. I will fix this. > > Thanks! > > > On Fri, Apr 28, 2017 at 10:19 AM, Surinder Kumar < > [email protected]> wrote: > >> Hi Dave, >> >> Please find updated patch for RM case and a separate patch for Feature >> tests. >> >> *Python:* >> >> - Added [default] label for cells with default values while inserting a >> new row. >> >> - Introduced a FieldValidator function for cells that don't allow null >> values. If user tries to insert null value, field with be highlighted with >> red borders around. >> >> - >> If a cell contains blank string('') and when we set it to null, the >> change into the cell is not detected. It was because the comparison >> for (defaultValue == null) return true if defaultValue is undefined. >> Hence _.isNull(value) is used to fix this. >> >> *Feature Test cases:* >> >> - Introduced a new method create_table_with_query(server, db_name, >> query) in test_utils.py which executes the given query on connected >> server. >> >> - Added a new file test_data.json that has test data for test cases. >> >> >> On Fri, Apr 7, 2017 at 2:21 PM, Dave Page <[email protected]> wrote: >> >>> Hi >>> >>> On Sat, Apr 1, 2017 at 12:45 PM, Surinder Kumar >>> <[email protected]> wrote: >>> > Hi >>> > >>> > Issues fixed: >>> > >>> > 1. If a column is defined with a default modifier, there is now way to >>> > insert the row with those defaults. >>> > The column will be left blank and it will take default value >>> automatically. >>> > >>> > 2. If a column has a not-null constraint then an error is returned and >>> the >>> > row is not inserted. >>> > The column will be left blank >>> > >>> > The default values for new added rows will be displayed on >>> refresh/execute. >>> > >>> > Please find attached patch and review. >>> >>> This largely works as expected, but there is some weirdness. I have a >>> test table that looks like this: >>> >>> CREATE TABLE public.defaults >>> ( >>> id bigint NOT NULL DEFAULT nextval('defaults_id_seq'::regclass), >>> data_default_nulls text COLLATE pg_catalog."default" DEFAULT >>> 'abc123'::text, >>> data_default_no_nulls text COLLATE pg_catalog."default" NOT NULL >>> DEFAULT 'def456'::text, >>> data_nulls text COLLATE pg_catalog."default", >>> data_no_nulls text COLLATE pg_catalog."default" NOT NULL, >>> CONSTRAINT defaults_pkey PRIMARY KEY (id) >>> ) >>> >>> Remember that the expected behaviour is: >>> >>> - Set a value to empty to update the column to null. >>> - Set a value to '' to update the column to an empty string >>> - Set a value to anything else to update the column to that value >>> >>> 1) In a row with values in each column, if I try to set the value of >>> data_default_nulls to null, the query executed is: >>> >>> UPDATE public.defaults SET >>> data_default_nulls = '' WHERE >>> id = '2'; >>> >>> 2) If I do the same in the data_nulls column, the value is immediately >>> shown as [null] and the query executed is: >>> >>> UPDATE public.defaults SET >>> data_nulls = NULL WHERE >>> id = '2'; >>> >>> 3) If I then edit the value in data_default_nulls, it shows the >>> current value as ''. Removing the quotes (to set it to null) doesn't >>> get detected as a change. >>> >> Taken care. >> >>> >>> 4) When I manually executed "update defaults set data_default_nulls = >>> null where id = 2" in a query tool window, I got: >>> >>> 2017-04-07 09:43:02,987: INFO werkzeug: 127.0.0.1 - - [07/Apr/2017 >>> 09:43:02] "GET /sqleditor/columns/8745675 HTTP/1.1" 500 - >>> Traceback (most recent call last): >>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>> ges/flask/app.py", >>> line 2000, in __call__ >>> return self.wsgi_app(environ, start_response) >>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>> ges/flask/app.py", >>> line 1991, in wsgi_app >>> response = self.make_response(self.handle_exception(e)) >>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>> ges/flask/app.py", >>> line 1567, in handle_exception >>> reraise(exc_type, exc_value, tb) >>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>> ges/flask/app.py", >>> line 1988, in wsgi_app >>> response = self.full_dispatch_request() >>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>> ges/flask/app.py", >>> line 1641, in full_dispatch_request >>> rv = self.handle_user_exception(e) >>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>> ges/flask/app.py", >>> line 1544, in handle_user_exception >>> reraise(exc_type, exc_value, tb) >>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>> ges/flask/app.py", >>> line 1639, in full_dispatch_request >>> rv = self.dispatch_request() >>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>> ges/flask/app.py", >>> line 1625, in dispatch_request >>> return self.view_functions[rule.endpoint](**req.view_args) >>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>> ges/flask_login.py", >>> line 792, in decorated_view >>> return func(*args, **kwargs) >>> File "/Users/dpage/git/pgadmin4/web/pgadmin/tools/sqleditor/__ini >>> t__.py", >>> line 452, in get_columns >>> tid=command_obj.obj_id) >>> AttributeError: 'QueryToolCommand' object has no attribute 'obj_id' >>> >> Fixed. >> >>> >>> 5) When I run the query again in pgAdmin III, then refresh the data in >>> pgAdmin 4, the data_default_nulls column is displayed without the >>> [null] marker (despite having a null value, which I confirmed in >>> pgAdmin 3). >>> >> Fixed. >> >>> >>> I'm sure there are other combinations of issues here. Please fix and >>> thoroughly re-test to ensure behaviour is consistent - and to avoid >>> future issues, please add some appropriate feature tests to check >>> nulls, defaults and empty strings are properly handled in view, insert >>> and updates. Murtuza recently wrote some feature tests for the query >>> tool - you should be able to use those as a starting point. >>> >> Added feature tests >> >>> >>> Thanks. >>> >>> -- >>> Dave Page >>> Blog: http://pgsnake.blogspot.com >>> Twitter: @pgsnake >>> >>> EnterpriseDB UK: http://www.enterprisedb.com >>> The Enterprise PostgreSQL Company >>> >> >> > > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-05 11:52 Surinder Kumar <[email protected]> parent: Surinder Kumar <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Surinder Kumar @ 2017-05-05 11:52 UTC (permalink / raw) To: Dave Page <[email protected]>; +Cc: pgadmin-hackers Hi Dave, The support to handle [null] and [default] values is added for following formatters: - JsonFormatter - NumbersFormatter - CheckmarkFormatter - TextFormatter Previously when a new row is added, it was not validating each and every cell for columns with attribute not_null = true. Introduced a new function validateRow(...) which is called on adding a new row, it validates each cell with column having attribute(not_null=true). the corresponding cell will highlighted and save button will be disabled if value is [null]. Now I will add more feature test cases for remaining formatters. Will send separate patch for feature test cases once completed. Please review updated patch. On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar < [email protected]> wrote: > Hi Dave, > > On Tue, May 2, 2017 at 5:21 PM, Dave Page <[email protected]> wrote: > >> Hi, >> >> This is looking much better now :-). Couple of thoughts and a bug: >> >> - Only the TextFormatter seems to handle both [null] and [default] >> values. Shouldn't all formatters do so (including Json and Checkmark)? >> > Yes, I will apply the same changes for other formatters too. > >> For example, "serial" columns currently get displayed as [null] when left >> blank, but I would expect to see [default]. >> >> - I would suggest we put [null] and [default] in a lighter colour - >> #999999. >> >> - With the feature test patch added, I seem to be consistently getting >> the following failure (immediately after your new tests run): >> >> ====================================================================== >> ERROR: runTest (pgadmin.feature_tests.xss_che >> cks_panels_and_query_tool_test.CheckForXssFeatureTest) >> Test XSS check for panels and query tool >> ---------------------------------------------------------------------- >> Traceback (most recent call last): >> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >> line 42, in setUp >> self._screenshot() >> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >> line 92, in _screenshot >> python_version)) >> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >> ges/selenium/webdriver/remote/webdriver.py", line 802, in >> get_screenshot_as_file >> png = self.get_screenshot_as_png() >> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >> ges/selenium/webdriver/remote/webdriver.py", line 821, in >> get_screenshot_as_png >> return base64.b64decode(self.get_screenshot_as_base64().encode(' >> ascii')) >> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >> ges/selenium/webdriver/remote/webdriver.py", line 831, in >> get_screenshot_as_base64 >> return self.execute(Command.SCREENSHOT)['value'] >> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >> ges/selenium/webdriver/remote/webdriver.py", line 238, in execute >> self.error_handler.check_response(response) >> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >> ges/selenium/webdriver/remote/errorhandler.py", line 193, in >> check_response >> raise exception_class(message, screen, stacktrace) >> UnexpectedAlertPresentException: Alert Text: None >> Message: unexpected alert open: {Alert text : Are you sure you wish to >> close the pgAdmin 4 browser?} >> (Session info: chrome=58.0.3029.81) >> (Driver info: chromedriver=2.29.461585 (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac >> OS X 10.12.3 x86_64) >> >> >> ====================================================================== >> ERROR: runTest (pgadmin.feature_tests.xss_che >> cks_pgadmin_debugger_test.CheckDebuggerForXssFeatureTest) >> Test table DDL generation >> ---------------------------------------------------------------------- >> Traceback (most recent call last): >> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >> line 42, in setUp >> self._screenshot() >> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >> line 92, in _screenshot >> python_version)) >> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >> ges/selenium/webdriver/remote/webdriver.py", line 802, in >> get_screenshot_as_file >> png = self.get_screenshot_as_png() >> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >> ges/selenium/webdriver/remote/webdriver.py", line 821, in >> get_screenshot_as_png >> return base64.b64decode(self.get_screenshot_as_base64().encode(' >> ascii')) >> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >> ges/selenium/webdriver/remote/webdriver.py", line 831, in >> get_screenshot_as_base64 >> return self.execute(Command.SCREENSHOT)['value'] >> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >> ges/selenium/webdriver/remote/webdriver.py", line 238, in execute >> self.error_handler.check_response(response) >> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >> ges/selenium/webdriver/remote/errorhandler.py", line 193, in >> check_response >> raise exception_class(message, screen, stacktrace) >> UnexpectedAlertPresentException: Alert Text: None >> Message: unexpected alert open: {Alert text : Are you sure you wish to >> close the pgAdmin 4 browser?} >> (Session info: chrome=58.0.3029.81) >> (Driver info: chromedriver=2.29.461585 (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac >> OS X 10.12.3 x86_64) >> > Sure. I will fix this. > >> >> Thanks! >> >> >> On Fri, Apr 28, 2017 at 10:19 AM, Surinder Kumar < >> [email protected]> wrote: >> >>> Hi Dave, >>> >>> Please find updated patch for RM case and a separate patch for Feature >>> tests. >>> >>> *Python:* >>> >>> - Added [default] label for cells with default values while inserting a >>> new row. >>> >>> - Introduced a FieldValidator function for cells that don't allow null >>> values. If user tries to insert null value, field with be highlighted with >>> red borders around. >>> >>> - >>> If a cell contains blank string('') and when we set it to null, the >>> change into the cell is not detected. It was because the comparison >>> for (defaultValue == null) return true if defaultValue is undefined. >>> Hence _.isNull(value) is used to fix this. >>> >>> *Feature Test cases:* >>> >>> - Introduced a new method create_table_with_query(server, db_name, >>> query) in test_utils.py which executes the given query on connected >>> server. >>> >>> - Added a new file test_data.json that has test data for test cases. >>> >>> >>> On Fri, Apr 7, 2017 at 2:21 PM, Dave Page <[email protected]> wrote: >>> >>>> Hi >>>> >>>> On Sat, Apr 1, 2017 at 12:45 PM, Surinder Kumar >>>> <[email protected]> wrote: >>>> > Hi >>>> > >>>> > Issues fixed: >>>> > >>>> > 1. If a column is defined with a default modifier, there is now way to >>>> > insert the row with those defaults. >>>> > The column will be left blank and it will take default value >>>> automatically. >>>> > >>>> > 2. If a column has a not-null constraint then an error is returned >>>> and the >>>> > row is not inserted. >>>> > The column will be left blank >>>> > >>>> > The default values for new added rows will be displayed on >>>> refresh/execute. >>>> > >>>> > Please find attached patch and review. >>>> >>>> This largely works as expected, but there is some weirdness. I have a >>>> test table that looks like this: >>>> >>>> CREATE TABLE public.defaults >>>> ( >>>> id bigint NOT NULL DEFAULT nextval('defaults_id_seq'::regclass), >>>> data_default_nulls text COLLATE pg_catalog."default" DEFAULT >>>> 'abc123'::text, >>>> data_default_no_nulls text COLLATE pg_catalog."default" NOT NULL >>>> DEFAULT 'def456'::text, >>>> data_nulls text COLLATE pg_catalog."default", >>>> data_no_nulls text COLLATE pg_catalog."default" NOT NULL, >>>> CONSTRAINT defaults_pkey PRIMARY KEY (id) >>>> ) >>>> >>>> Remember that the expected behaviour is: >>>> >>>> - Set a value to empty to update the column to null. >>>> - Set a value to '' to update the column to an empty string >>>> - Set a value to anything else to update the column to that value >>>> >>>> 1) In a row with values in each column, if I try to set the value of >>>> data_default_nulls to null, the query executed is: >>>> >>>> UPDATE public.defaults SET >>>> data_default_nulls = '' WHERE >>>> id = '2'; >>>> >>>> 2) If I do the same in the data_nulls column, the value is immediately >>>> shown as [null] and the query executed is: >>>> >>>> UPDATE public.defaults SET >>>> data_nulls = NULL WHERE >>>> id = '2'; >>>> >>>> 3) If I then edit the value in data_default_nulls, it shows the >>>> current value as ''. Removing the quotes (to set it to null) doesn't >>>> get detected as a change. >>>> >>> Taken care. >>> >>>> >>>> 4) When I manually executed "update defaults set data_default_nulls = >>>> null where id = 2" in a query tool window, I got: >>>> >>>> 2017-04-07 09:43:02,987: INFO werkzeug: 127.0.0.1 - - [07/Apr/2017 >>>> 09:43:02] "GET /sqleditor/columns/8745675 HTTP/1.1" 500 - >>>> Traceback (most recent call last): >>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>> ges/flask/app.py", >>>> line 2000, in __call__ >>>> return self.wsgi_app(environ, start_response) >>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>> ges/flask/app.py", >>>> line 1991, in wsgi_app >>>> response = self.make_response(self.handle_exception(e)) >>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>> ges/flask/app.py", >>>> line 1567, in handle_exception >>>> reraise(exc_type, exc_value, tb) >>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>> ges/flask/app.py", >>>> line 1988, in wsgi_app >>>> response = self.full_dispatch_request() >>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>> ges/flask/app.py", >>>> line 1641, in full_dispatch_request >>>> rv = self.handle_user_exception(e) >>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>> ges/flask/app.py", >>>> line 1544, in handle_user_exception >>>> reraise(exc_type, exc_value, tb) >>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>> ges/flask/app.py", >>>> line 1639, in full_dispatch_request >>>> rv = self.dispatch_request() >>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>> ges/flask/app.py", >>>> line 1625, in dispatch_request >>>> return self.view_functions[rule.endpoint](**req.view_args) >>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>> ges/flask_login.py", >>>> line 792, in decorated_view >>>> return func(*args, **kwargs) >>>> File "/Users/dpage/git/pgadmin4/web/pgadmin/tools/sqleditor/__ini >>>> t__.py", >>>> line 452, in get_columns >>>> tid=command_obj.obj_id) >>>> AttributeError: 'QueryToolCommand' object has no attribute 'obj_id' >>>> >>> Fixed. >>> >>>> >>>> 5) When I run the query again in pgAdmin III, then refresh the data in >>>> pgAdmin 4, the data_default_nulls column is displayed without the >>>> [null] marker (despite having a null value, which I confirmed in >>>> pgAdmin 3). >>>> >>> Fixed. >>> >>>> >>>> I'm sure there are other combinations of issues here. Please fix and >>>> thoroughly re-test to ensure behaviour is consistent - and to avoid >>>> future issues, please add some appropriate feature tests to check >>>> nulls, defaults and empty strings are properly handled in view, insert >>>> and updates. Murtuza recently wrote some feature tests for the query >>>> tool - you should be able to use those as a starting point. >>>> >>> Added feature tests >>> >>>> >>>> Thanks. >>>> >>>> -- >>>> Dave Page >>>> Blog: http://pgsnake.blogspot.com >>>> Twitter: @pgsnake >>>> >>>> EnterpriseDB UK: http://www.enterprisedb.com >>>> The Enterprise PostgreSQL Company >>>> >>> >>> >> >> >> -- >> Dave Page >> Blog: http://pgsnake.blogspot.com >> Twitter: @pgsnake >> >> EnterpriseDB UK: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > > -- Sent via pgadmin-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers Attachments: [application/octet-stream] RM_2257_v2.patch (15.7K, 3-RM_2257_v2.patch) download | inline diff: diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql index 759e657..f3353d6 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql @@ -1,4 +1,5 @@ -SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype +SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype, +att.attnotnull as not_null, att.atthasdef as has_default_val FROM pg_attribute att JOIN pg_type ty ON ty.oid=atttypid JOIN pg_namespace tn ON tn.oid=ty.typnamespace diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql index 7536a9c..4f1de2a 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql @@ -1,4 +1,5 @@ -SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype +SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype, +att.attnotnull as not_null, att.atthasdef as has_default_val FROM pg_attribute att JOIN pg_type ty ON ty.oid=atttypid JOIN pg_namespace tn ON tn.oid=ty.typnamespace diff --git a/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js b/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js index cdfba4d..0bdd1ab 100644 --- a/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js +++ b/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js @@ -110,7 +110,12 @@ // When text editor opens this.loadValue = function (item) { - if (item[args.column.pos] === "") { + var col = args.column; + + if (_.isUndefined(item[args.column.pos]) && col.has_default_val) { + $input.val(""); + } + else if (item[args.column.pos] === "") { $input.val("''"); } else { @@ -145,7 +150,10 @@ }; this.isValueChanged = function () { - return (!($input.val() == "" && defaultValue == null)) && ($input.val() != defaultValue); + // Use _.isNull(value) for comparison for null instead of + // defaultValue == null, because it returns true for undefined value. + return (!($input.val() == "" && _.isNull(defaultValue))) && + ($input.val() != defaultValue); }; this.validate = function () { @@ -253,7 +261,7 @@ this.loadValue = function (item) { var data = defaultValue = item[args.column.pos]; - if (typeof data === "object" && !Array.isArray(data)) { + if (data && typeof data === "object" && !Array.isArray(data)) { data = JSON.stringify(data); } else if (Array.isArray(data)) { var temp = []; @@ -282,7 +290,7 @@ }; this.isValueChanged = function () { - return (!($input.val() == "" && defaultValue == null)) && ($input.val() != defaultValue); + return (!($input.val() == "" && _.isNull(defaultValue))) && ($input.val() != defaultValue); }; this.validate = function () { @@ -498,6 +506,12 @@ }; this.validate = function () { + if (args.column.validator) { + var validationResults = args.column.validator(this.serializeValue()); + if (!validationResults.valid) { + return validationResults; + } + } return { valid: true, msg: null @@ -837,7 +851,12 @@ }; this.isValueChanged = function () { - return (!($input.val() == "" && defaultValue == null)) && ($input.val() != defaultValue); + if ($input.val() == "" && defaultValue == "") { + return true; + } else { + return (!($input.val() == "" && _.isNull(defaultValue ))) && + ($input.val() != defaultValue); + } }; this.validate = function () { diff --git a/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js b/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js index 290bddd..61b1fc1 100644 --- a/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js +++ b/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js @@ -19,8 +19,15 @@ }); function JsonFormatter(row, cell, value, columnDef, dataContext) { - if (value == null || value === "") { - return ""; + // If column has default value, set placeholder + if (_.isUndefined(value) && columnDef.has_default_val) { + return "<span class='pull-left'>[default]</span>"; + } + else if (_.isUndefined(value) && columnDef.not_null) { + return null; // If null value not allowed, set cell to blank + } + else if (_.isUndefined(value) || value === null) { + return "<span class='pull-left'>[null]</span>"; } else { // Stringify only if it's json object if (typeof value === "object" && !Array.isArray(value)) { @@ -42,7 +49,14 @@ } function NumbersFormatter(row, cell, value, columnDef, dataContext) { - if (_.isUndefined(value) || value === null) { + // If column has default value, set placeholder + if (_.isUndefined(value) && columnDef.has_default_val) { + return "<span class='pull-right'>[default]</span>"; + } + else if (_.isUndefined(value) && columnDef.not_null) { + return ''; // If null value not allowed, set cell to blank + } + else if (_.isUndefined(value) || value === null) { return "<span class='pull-right'>[null]</span>"; } else if (value === "") { @@ -57,16 +71,29 @@ /* Checkbox has 3 states * 1) checked=true * 2) unchecked=false - * 3) indeterminate=null/'' + * 3) indeterminate=null */ - if (value == null || value === "") { + if (_.isUndefined(value) && columnDef.has_default_val) { + return "<span class='pull-left'>[default]</span>"; + } + else if (_.isUndefined(value) && columnDef.not_null) { + return ''; // If null value not allowed, set cell to blank + } + else if (value == null || value === "") { return "<span class='pull-left'>[null]</span>"; } return value ? "true" : "false"; } function TextFormatter(row, cell, value, columnDef, dataContext) { - if (_.isUndefined(value) || value === null) { + // If column has default value, set placeholder + if (_.isUndefined(value) && columnDef.has_default_val) { + return "<span class='pull-left'>[default]</span>"; + } + else if (_.isUndefined(value) && columnDef.not_null) { + return ''; // If null value not allowed, set cell to blank + } + else if (_.isUndefined(value) || _.isNull(value)) { return "<span class='pull-left'>[null]</span>"; } else { diff --git a/web/pgadmin/tools/sqleditor/__init__.py b/web/pgadmin/tools/sqleditor/__init__.py index d114988..f7466d8 100644 --- a/web/pgadmin/tools/sqleditor/__init__.py +++ b/web/pgadmin/tools/sqleditor/__init__.py @@ -440,8 +440,23 @@ def get_columns(trans_id): columns = dict() columns_info = None primary_keys = None + rset = None status, error_msg, conn, trans_obj, session_obj = check_transaction_status(trans_id) if status and conn is not None and session_obj is not None: + + ver = conn.manager.version + # Get the template path for the column + template_path = 'column/sql/#{0}#'.format(ver) + command_obj = pickle.loads(session_obj['command_obj']) + if hasattr(command_obj, 'obj_id'): + SQL = render_template("/".join([template_path, + 'nodes.sql']), + tid=command_obj.obj_id) + # rows with attribute not_null + status, rset = conn.execute_2darray(SQL) + if not status: + return internal_server_error(errormsg=rset) + # Check PK column info is available or not if 'primary_keys' in session_obj: primary_keys = session_obj['primary_keys'] @@ -449,10 +464,17 @@ def get_columns(trans_id): # Fetch column information columns_info = conn.get_column_info() if columns_info is not None: - for col in columns_info: + for key, col in enumerate(columns_info): col_type = dict() col_type['type_code'] = col['type_code'] col_type['type_name'] = None + if rset: + col_type['not_null'] = col['not_null'] = \ + rset['rows'][key]['not_null'] + + col_type['has_default_val'] = col['has_default_val'] = \ + rset['rows'][key]['has_default_val'] + columns[col['name']] = col_type # As we changed the transaction object we need to @@ -602,6 +624,7 @@ def save(trans_id): status, error_msg, conn, trans_obj, session_obj = check_transaction_status(trans_id) if status and conn is not None \ and trans_obj is not None and session_obj is not None: + setattr(trans_obj, 'columns_info', session_obj['columns_info']) # If there is no primary key found then return from the function. if len(session_obj['primary_keys']) <= 0 or len(changed_data) <= 0: diff --git a/web/pgadmin/tools/sqleditor/command.py b/web/pgadmin/tools/sqleditor/command.py index 1795155..e396981 100644 --- a/web/pgadmin/tools/sqleditor/command.py +++ b/web/pgadmin/tools/sqleditor/command.py @@ -442,6 +442,23 @@ class TableCommand(GridCommand): # For newly added rows if of_type == 'added': + + # When new rows are added, only changed columns data is + # sent from client side. But if column is not_null and has + # no_default_value, set column to blank, instead + # of not null which is set by default. + column_data = {} + column_type = {} + for each_col in self.columns_info: + if ( + self.columns_info[each_col]['not_null'] and + not self.columns_info[each_col][ + 'has_default_val'] + ): + column_data[each_col] = "" + column_type[each_col] =\ + self.columns_info[each_col]['type_name'] + for each_row in changed_data[of_type]: data = changed_data[of_type][each_row]['data'] # Remove our unique tracking key @@ -450,12 +467,18 @@ class TableCommand(GridCommand): data_type = set_column_names(changed_data[of_type][each_row]['data_type']) list_of_rowid.append(data.get('__temp_PK')) + # Update columns value and data type + # with columns having not_null=False and has + # no default value + column_data.update(data) + column_type.update(data_type) + sql = render_template("/".join([self.sql_path, 'insert.sql']), - data_to_be_saved=data, + data_to_be_saved=column_data, primary_keys=None, object_name=self.object_name, nsp_name=self.nsp_name, - data_type=data_type) + data_type=column_type) list_of_sql.append(sql) # For updated rows diff --git a/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js b/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js index 2062aa2..6915bb1 100644 --- a/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js +++ b/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js @@ -526,6 +526,19 @@ define( render_grid: function(collection, columns, is_editable) { var self = this; + var requiredFieldValidator = function (value) { + // If value if of boolean type, + // the comparision for value(false) == '' returns true + // so set it to some another value. + if (typeof(value) === 'boolean') + value = 2; + + if (_.isUndefined(value) || _.isNull(value) || value == '') { + return {valid: false, msg: "This is a required field"}; + } else { + return {valid: true, msg: null}; + } + } // This will work as data store and holds all the // inserted/updated/deleted data from grid self.handler.data_store = { @@ -557,7 +570,10 @@ define( id: c.name, pos: c.pos, field: c.name, - name: c.label + name: c.label, + not_null: c.not_null, + has_default_val: c.has_default_val, + validator: c.not_null ? requiredFieldValidator : null }; // Get the columns width based on data type @@ -806,6 +822,32 @@ define( $("#btn-save").prop('disabled', false); }.bind(editor_data)); + // Validate each cell of new added row. + var validateRow = function(grid, rowIdx) { + var is_valid = true; + $.each(grid.getColumns(), function(colIdx, column) { + // iterate through editable cells + // Skip first column - Select all + if (colIdx && colIdx !==0) { + var item = grid.getData()[rowIdx][colIdx-1] || ''; + + if (column.editor && column.validator && column.not_null && + !column.has_default_val) { + var validationResults = column.validator(item); + if (!validationResults.valid) { + // show editor + grid.gotoCell(rowIdx, colIdx, true); + // validate (it will fail) + grid.getEditorLock().commitCurrentEdit(); + // stop iteration + is_valid = false; + return is_valid; + } + } + } + }); + return is_valid; + } // Listener function which will be called when user adds new rows grid.onAddNewRow.subscribe(function (e, args) { // self.handler.data_store.added will holds all the newly added rows/data @@ -826,8 +868,10 @@ define( grid.invalidateRows([collection.length - 1]); grid.updateRowCount(); grid.render(); + // Once new row is rendered, validate row + var is_valid = validateRow(args.grid, args.grid.getActiveCell().row); // Enable save button - $("#btn-save").prop('disabled', false); + $("#btn-save").prop('disabled', !is_valid); }.bind(editor_data)); // Resize SlickGrid when window resize @@ -2077,7 +2121,9 @@ define( 'label': column_label, 'cell': col_cell, 'can_edit': self.can_edit, - 'type': type + 'type': type, + 'not_null': c.not_null, + 'has_default_val': c.has_default_val }; columns.push(col); }); ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-08 09:58 Dave Page <[email protected]> parent: Surinder Kumar <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Dave Page @ 2017-05-08 09:58 UTC (permalink / raw) To: Surinder Kumar <[email protected]>; +Cc: pgadmin-hackers Hi On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar < [email protected]> wrote: > Hi Dave, > > The support to handle [null] and [default] values is added for following > formatters: > - JsonFormatter > - NumbersFormatter > - CheckmarkFormatter > - TextFormatter > > Previously when a new row is added, it was not validating each and every > cell for columns with attribute not_null = true. > Introduced a new function validateRow(...) which is called on adding a > new row, it validates each cell with column having > attribute(not_null=true). the corresponding cell will highlighted and save > button will be disabled if value is [null]. > I'm not sure that behaviour is right. What I now see (given the table below) is that: - If I click in the id field of a new row, it forces me to enter a value or hit escape. Why is it trying to force me? It's a serial field, so will get a value on save anyway. - If I do enter a value in the ID field, focus jumps over all the other fields with either a default or no 'not null' option to the data_no_nulls column. Focus should always go to the next field. I think this addition can just be removed - I'm pretty sure the previous behaviour would have been what we want, with the additional formatters fixed. > > Now I will add more feature test cases for remaining formatters. Will send > separate patch for feature test cases once completed. > Thanks. > > Please review updated patch. > > > On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar < > [email protected]> wrote: > >> Hi Dave, >> >> On Tue, May 2, 2017 at 5:21 PM, Dave Page <[email protected]> wrote: >> >>> Hi, >>> >>> This is looking much better now :-). Couple of thoughts and a bug: >>> >>> - Only the TextFormatter seems to handle both [null] and [default] >>> values. Shouldn't all formatters do so (including Json and Checkmark)? >>> >> Yes, I will apply the same changes for other formatters too. >> >>> For example, "serial" columns currently get displayed as [null] when >>> left blank, but I would expect to see [default]. >>> >>> - I would suggest we put [null] and [default] in a lighter colour - >>> #999999. >>> >>> - With the feature test patch added, I seem to be consistently getting >>> the following failure (immediately after your new tests run): >>> >>> ====================================================================== >>> ERROR: runTest (pgadmin.feature_tests.xss_che >>> cks_panels_and_query_tool_test.CheckForXssFeatureTest) >>> Test XSS check for panels and query tool >>> ---------------------------------------------------------------------- >>> Traceback (most recent call last): >>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>> line 42, in setUp >>> self._screenshot() >>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>> line 92, in _screenshot >>> python_version)) >>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>> ges/selenium/webdriver/remote/webdriver.py", line 802, in >>> get_screenshot_as_file >>> png = self.get_screenshot_as_png() >>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>> ges/selenium/webdriver/remote/webdriver.py", line 821, in >>> get_screenshot_as_png >>> return base64.b64decode(self.get_screenshot_as_base64().encode('asc >>> ii')) >>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>> ges/selenium/webdriver/remote/webdriver.py", line 831, in >>> get_screenshot_as_base64 >>> return self.execute(Command.SCREENSHOT)['value'] >>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>> ges/selenium/webdriver/remote/webdriver.py", line 238, in execute >>> self.error_handler.check_response(response) >>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>> ges/selenium/webdriver/remote/errorhandler.py", line 193, in >>> check_response >>> raise exception_class(message, screen, stacktrace) >>> UnexpectedAlertPresentException: Alert Text: None >>> Message: unexpected alert open: {Alert text : Are you sure you wish to >>> close the pgAdmin 4 browser?} >>> (Session info: chrome=58.0.3029.81) >>> (Driver info: chromedriver=2.29.461585 (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac >>> OS X 10.12.3 x86_64) >>> >>> >>> ====================================================================== >>> ERROR: runTest (pgadmin.feature_tests.xss_che >>> cks_pgadmin_debugger_test.CheckDebuggerForXssFeatureTest) >>> Test table DDL generation >>> ---------------------------------------------------------------------- >>> Traceback (most recent call last): >>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>> line 42, in setUp >>> self._screenshot() >>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>> line 92, in _screenshot >>> python_version)) >>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>> ges/selenium/webdriver/remote/webdriver.py", line 802, in >>> get_screenshot_as_file >>> png = self.get_screenshot_as_png() >>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>> ges/selenium/webdriver/remote/webdriver.py", line 821, in >>> get_screenshot_as_png >>> return base64.b64decode(self.get_screenshot_as_base64().encode('asc >>> ii')) >>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>> ges/selenium/webdriver/remote/webdriver.py", line 831, in >>> get_screenshot_as_base64 >>> return self.execute(Command.SCREENSHOT)['value'] >>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>> ges/selenium/webdriver/remote/webdriver.py", line 238, in execute >>> self.error_handler.check_response(response) >>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>> ges/selenium/webdriver/remote/errorhandler.py", line 193, in >>> check_response >>> raise exception_class(message, screen, stacktrace) >>> UnexpectedAlertPresentException: Alert Text: None >>> Message: unexpected alert open: {Alert text : Are you sure you wish to >>> close the pgAdmin 4 browser?} >>> (Session info: chrome=58.0.3029.81) >>> (Driver info: chromedriver=2.29.461585 (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac >>> OS X 10.12.3 x86_64) >>> >> Sure. I will fix this. >> >>> >>> Thanks! >>> >>> >>> On Fri, Apr 28, 2017 at 10:19 AM, Surinder Kumar < >>> [email protected]> wrote: >>> >>>> Hi Dave, >>>> >>>> Please find updated patch for RM case and a separate patch for Feature >>>> tests. >>>> >>>> *Python:* >>>> >>>> - Added [default] label for cells with default values while inserting >>>> a new row. >>>> >>>> - Introduced a FieldValidator function for cells that don't allow null >>>> values. If user tries to insert null value, field with be highlighted with >>>> red borders around. >>>> >>>> - >>>> If a cell contains blank string('') and when we set it to null, the >>>> change into the cell is not detected. It was because the comparison >>>> for (defaultValue == null) return true if defaultValue is undefined. >>>> Hence _.isNull(value) is used to fix this. >>>> >>>> *Feature Test cases:* >>>> >>>> - Introduced a new method create_table_with_query(server, db_name, >>>> query) in test_utils.py which executes the given query on connected >>>> server. >>>> >>>> - Added a new file test_data.json that has test data for test cases. >>>> >>>> >>>> On Fri, Apr 7, 2017 at 2:21 PM, Dave Page <[email protected]> wrote: >>>> >>>>> Hi >>>>> >>>>> On Sat, Apr 1, 2017 at 12:45 PM, Surinder Kumar >>>>> <[email protected]> wrote: >>>>> > Hi >>>>> > >>>>> > Issues fixed: >>>>> > >>>>> > 1. If a column is defined with a default modifier, there is now way >>>>> to >>>>> > insert the row with those defaults. >>>>> > The column will be left blank and it will take default value >>>>> automatically. >>>>> > >>>>> > 2. If a column has a not-null constraint then an error is returned >>>>> and the >>>>> > row is not inserted. >>>>> > The column will be left blank >>>>> > >>>>> > The default values for new added rows will be displayed on >>>>> refresh/execute. >>>>> > >>>>> > Please find attached patch and review. >>>>> >>>>> This largely works as expected, but there is some weirdness. I have a >>>>> test table that looks like this: >>>>> >>>>> CREATE TABLE public.defaults >>>>> ( >>>>> id bigint NOT NULL DEFAULT nextval('defaults_id_seq'::regclass), >>>>> data_default_nulls text COLLATE pg_catalog."default" DEFAULT >>>>> 'abc123'::text, >>>>> data_default_no_nulls text COLLATE pg_catalog."default" NOT NULL >>>>> DEFAULT 'def456'::text, >>>>> data_nulls text COLLATE pg_catalog."default", >>>>> data_no_nulls text COLLATE pg_catalog."default" NOT NULL, >>>>> CONSTRAINT defaults_pkey PRIMARY KEY (id) >>>>> ) >>>>> >>>>> Remember that the expected behaviour is: >>>>> >>>>> - Set a value to empty to update the column to null. >>>>> - Set a value to '' to update the column to an empty string >>>>> - Set a value to anything else to update the column to that value >>>>> >>>>> 1) In a row with values in each column, if I try to set the value of >>>>> data_default_nulls to null, the query executed is: >>>>> >>>>> UPDATE public.defaults SET >>>>> data_default_nulls = '' WHERE >>>>> id = '2'; >>>>> >>>>> 2) If I do the same in the data_nulls column, the value is immediately >>>>> shown as [null] and the query executed is: >>>>> >>>>> UPDATE public.defaults SET >>>>> data_nulls = NULL WHERE >>>>> id = '2'; >>>>> >>>>> 3) If I then edit the value in data_default_nulls, it shows the >>>>> current value as ''. Removing the quotes (to set it to null) doesn't >>>>> get detected as a change. >>>>> >>>> Taken care. >>>> >>>>> >>>>> 4) When I manually executed "update defaults set data_default_nulls = >>>>> null where id = 2" in a query tool window, I got: >>>>> >>>>> 2017-04-07 09:43:02,987: INFO werkzeug: 127.0.0.1 - - [07/Apr/2017 >>>>> 09:43:02] "GET /sqleditor/columns/8745675 HTTP/1.1" 500 - >>>>> Traceback (most recent call last): >>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>> ges/flask/app.py", >>>>> line 2000, in __call__ >>>>> return self.wsgi_app(environ, start_response) >>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>> ges/flask/app.py", >>>>> line 1991, in wsgi_app >>>>> response = self.make_response(self.handle_exception(e)) >>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>> ges/flask/app.py", >>>>> line 1567, in handle_exception >>>>> reraise(exc_type, exc_value, tb) >>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>> ges/flask/app.py", >>>>> line 1988, in wsgi_app >>>>> response = self.full_dispatch_request() >>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>> ges/flask/app.py", >>>>> line 1641, in full_dispatch_request >>>>> rv = self.handle_user_exception(e) >>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>> ges/flask/app.py", >>>>> line 1544, in handle_user_exception >>>>> reraise(exc_type, exc_value, tb) >>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>> ges/flask/app.py", >>>>> line 1639, in full_dispatch_request >>>>> rv = self.dispatch_request() >>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>> ges/flask/app.py", >>>>> line 1625, in dispatch_request >>>>> return self.view_functions[rule.endpoint](**req.view_args) >>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>> ges/flask_login.py", >>>>> line 792, in decorated_view >>>>> return func(*args, **kwargs) >>>>> File "/Users/dpage/git/pgadmin4/web/pgadmin/tools/sqleditor/__ini >>>>> t__.py", >>>>> line 452, in get_columns >>>>> tid=command_obj.obj_id) >>>>> AttributeError: 'QueryToolCommand' object has no attribute 'obj_id' >>>>> >>>> Fixed. >>>> >>>>> >>>>> 5) When I run the query again in pgAdmin III, then refresh the data in >>>>> pgAdmin 4, the data_default_nulls column is displayed without the >>>>> [null] marker (despite having a null value, which I confirmed in >>>>> pgAdmin 3). >>>>> >>>> Fixed. >>>> >>>>> >>>>> I'm sure there are other combinations of issues here. Please fix and >>>>> thoroughly re-test to ensure behaviour is consistent - and to avoid >>>>> future issues, please add some appropriate feature tests to check >>>>> nulls, defaults and empty strings are properly handled in view, insert >>>>> and updates. Murtuza recently wrote some feature tests for the query >>>>> tool - you should be able to use those as a starting point. >>>>> >>>> Added feature tests >>>> >>>>> >>>>> Thanks. >>>>> >>>>> -- >>>>> Dave Page >>>>> Blog: http://pgsnake.blogspot.com >>>>> Twitter: @pgsnake >>>>> >>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>> The Enterprise PostgreSQL Company >>>>> >>>> >>>> >>> >>> >>> -- >>> Dave Page >>> Blog: http://pgsnake.blogspot.com >>> Twitter: @pgsnake >>> >>> EnterpriseDB UK: http://www.enterprisedb.com >>> The Enterprise PostgreSQL Company >>> >> >> > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-08 10:13 Surinder Kumar <[email protected]> parent: Dave Page <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Surinder Kumar @ 2017-05-08 10:13 UTC (permalink / raw) To: Dave Page <[email protected]>; +Cc: pgadmin-hackers Hi Dave, On Mon, May 8, 2017 at 3:28 PM, Dave Page <[email protected]> wrote: > Hi > > On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar < > [email protected]> wrote: > >> Hi Dave, >> >> The support to handle [null] and [default] values is added for following >> formatters: >> - JsonFormatter >> - NumbersFormatter >> - CheckmarkFormatter >> - TextFormatter >> >> Previously when a new row is added, it was not validating each and every >> cell for columns with attribute not_null = true. >> Introduced a new function validateRow(...) which is called on adding a >> new row, it validates each cell with column having >> attribute(not_null=true). the corresponding cell will highlighted and save >> button will be disabled if value is [null]. >> > > I'm not sure that behaviour is right. What I now see (given the table > below) is that: > > - If I click in the id field of a new row, it forces me to enter a value > or hit escape. Why is it trying to force me? It's a serial field, so will > get a value on save anyway. > Yes, It is because I am validating all cell values after it renders. It will reverted back. > > - If I do enter a value in the ID field, focus jumps over all the other > fields with either a default or no 'not null' option to the data_no_nulls > column. Focus should always go to the next field. > > I think this addition can just be removed - I'm pretty sure the previous > behaviour would have been what we want, with the additional formatters > fixed. > But If i remove this addition, the value for column like 'data_no_nulls' will be set to '' (blank string), then on save its value will be validated on the server side and whatever the error message is will be returned back (the same behaviour as in pgAdmin3) > > >> >> Now I will add more feature test cases for remaining formatters. Will >> send separate patch for feature test cases once completed. >> > > Thanks. > > >> >> Please review updated patch. >> >> >> On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar < >> [email protected]> wrote: >> >>> Hi Dave, >>> >>> On Tue, May 2, 2017 at 5:21 PM, Dave Page <[email protected]> wrote: >>> >>>> Hi, >>>> >>>> This is looking much better now :-). Couple of thoughts and a bug: >>>> >>>> - Only the TextFormatter seems to handle both [null] and [default] >>>> values. Shouldn't all formatters do so (including Json and Checkmark)? >>>> >>> Yes, I will apply the same changes for other formatters too. >>> >>>> For example, "serial" columns currently get displayed as [null] when >>>> left blank, but I would expect to see [default]. >>>> >>>> - I would suggest we put [null] and [default] in a lighter colour - >>>> #999999. >>>> >>>> - With the feature test patch added, I seem to be consistently getting >>>> the following failure (immediately after your new tests run): >>>> >>>> ====================================================================== >>>> ERROR: runTest (pgadmin.feature_tests.xss_che >>>> cks_panels_and_query_tool_test.CheckForXssFeatureTest) >>>> Test XSS check for panels and query tool >>>> ---------------------------------------------------------------------- >>>> Traceback (most recent call last): >>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>> line 42, in setUp >>>> self._screenshot() >>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>> line 92, in _screenshot >>>> python_version)) >>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>> ges/selenium/webdriver/remote/webdriver.py", line 802, in >>>> get_screenshot_as_file >>>> png = self.get_screenshot_as_png() >>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>> ges/selenium/webdriver/remote/webdriver.py", line 821, in >>>> get_screenshot_as_png >>>> return base64.b64decode(self.get_screenshot_as_base64().encode('asc >>>> ii')) >>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>> ges/selenium/webdriver/remote/webdriver.py", line 831, in >>>> get_screenshot_as_base64 >>>> return self.execute(Command.SCREENSHOT)['value'] >>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>> ges/selenium/webdriver/remote/webdriver.py", line 238, in execute >>>> self.error_handler.check_response(response) >>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>> ges/selenium/webdriver/remote/errorhandler.py", line 193, in >>>> check_response >>>> raise exception_class(message, screen, stacktrace) >>>> UnexpectedAlertPresentException: Alert Text: None >>>> Message: unexpected alert open: {Alert text : Are you sure you wish to >>>> close the pgAdmin 4 browser?} >>>> (Session info: chrome=58.0.3029.81) >>>> (Driver info: chromedriver=2.29.461585 (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac >>>> OS X 10.12.3 x86_64) >>>> >>>> >>>> ====================================================================== >>>> ERROR: runTest (pgadmin.feature_tests.xss_che >>>> cks_pgadmin_debugger_test.CheckDebuggerForXssFeatureTest) >>>> Test table DDL generation >>>> ---------------------------------------------------------------------- >>>> Traceback (most recent call last): >>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>> line 42, in setUp >>>> self._screenshot() >>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>> line 92, in _screenshot >>>> python_version)) >>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>> ges/selenium/webdriver/remote/webdriver.py", line 802, in >>>> get_screenshot_as_file >>>> png = self.get_screenshot_as_png() >>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>> ges/selenium/webdriver/remote/webdriver.py", line 821, in >>>> get_screenshot_as_png >>>> return base64.b64decode(self.get_screenshot_as_base64().encode('asc >>>> ii')) >>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>> ges/selenium/webdriver/remote/webdriver.py", line 831, in >>>> get_screenshot_as_base64 >>>> return self.execute(Command.SCREENSHOT)['value'] >>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>> ges/selenium/webdriver/remote/webdriver.py", line 238, in execute >>>> self.error_handler.check_response(response) >>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>> ges/selenium/webdriver/remote/errorhandler.py", line 193, in >>>> check_response >>>> raise exception_class(message, screen, stacktrace) >>>> UnexpectedAlertPresentException: Alert Text: None >>>> Message: unexpected alert open: {Alert text : Are you sure you wish to >>>> close the pgAdmin 4 browser?} >>>> (Session info: chrome=58.0.3029.81) >>>> (Driver info: chromedriver=2.29.461585 (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac >>>> OS X 10.12.3 x86_64) >>>> >>> Sure. I will fix this. >>> >>>> >>>> Thanks! >>>> >>>> >>>> On Fri, Apr 28, 2017 at 10:19 AM, Surinder Kumar < >>>> [email protected]> wrote: >>>> >>>>> Hi Dave, >>>>> >>>>> Please find updated patch for RM case and a separate patch for Feature >>>>> tests. >>>>> >>>>> *Python:* >>>>> >>>>> - Added [default] label for cells with default values while inserting >>>>> a new row. >>>>> >>>>> - Introduced a FieldValidator function for cells that don't allow null >>>>> values. If user tries to insert null value, field with be highlighted with >>>>> red borders around. >>>>> >>>>> - >>>>> If a cell contains blank string('') and when we set it to null, the >>>>> change into the cell is not detected. It was because the comparison >>>>> for (defaultValue == null) return true if defaultValue is undefined. >>>>> Hence _.isNull(value) is used to fix this. >>>>> >>>>> *Feature Test cases:* >>>>> >>>>> - Introduced a new method create_table_with_query(server, db_name, >>>>> query) in test_utils.py which executes the given query on connected >>>>> server. >>>>> >>>>> - Added a new file test_data.json that has test data for test cases. >>>>> >>>>> >>>>> On Fri, Apr 7, 2017 at 2:21 PM, Dave Page <[email protected]> wrote: >>>>> >>>>>> Hi >>>>>> >>>>>> On Sat, Apr 1, 2017 at 12:45 PM, Surinder Kumar >>>>>> <[email protected]> wrote: >>>>>> > Hi >>>>>> > >>>>>> > Issues fixed: >>>>>> > >>>>>> > 1. If a column is defined with a default modifier, there is now way >>>>>> to >>>>>> > insert the row with those defaults. >>>>>> > The column will be left blank and it will take default value >>>>>> automatically. >>>>>> > >>>>>> > 2. If a column has a not-null constraint then an error is returned >>>>>> and the >>>>>> > row is not inserted. >>>>>> > The column will be left blank >>>>>> > >>>>>> > The default values for new added rows will be displayed on >>>>>> refresh/execute. >>>>>> > >>>>>> > Please find attached patch and review. >>>>>> >>>>>> This largely works as expected, but there is some weirdness. I have a >>>>>> test table that looks like this: >>>>>> >>>>>> CREATE TABLE public.defaults >>>>>> ( >>>>>> id bigint NOT NULL DEFAULT nextval('defaults_id_seq'::regclass), >>>>>> data_default_nulls text COLLATE pg_catalog."default" DEFAULT >>>>>> 'abc123'::text, >>>>>> data_default_no_nulls text COLLATE pg_catalog."default" NOT NULL >>>>>> DEFAULT 'def456'::text, >>>>>> data_nulls text COLLATE pg_catalog."default", >>>>>> data_no_nulls text COLLATE pg_catalog."default" NOT NULL, >>>>>> CONSTRAINT defaults_pkey PRIMARY KEY (id) >>>>>> ) >>>>>> >>>>>> Remember that the expected behaviour is: >>>>>> >>>>>> - Set a value to empty to update the column to null. >>>>>> - Set a value to '' to update the column to an empty string >>>>>> - Set a value to anything else to update the column to that value >>>>>> >>>>>> 1) In a row with values in each column, if I try to set the value of >>>>>> data_default_nulls to null, the query executed is: >>>>>> >>>>>> UPDATE public.defaults SET >>>>>> data_default_nulls = '' WHERE >>>>>> id = '2'; >>>>>> >>>>>> 2) If I do the same in the data_nulls column, the value is immediately >>>>>> shown as [null] and the query executed is: >>>>>> >>>>>> UPDATE public.defaults SET >>>>>> data_nulls = NULL WHERE >>>>>> id = '2'; >>>>>> >>>>>> 3) If I then edit the value in data_default_nulls, it shows the >>>>>> current value as ''. Removing the quotes (to set it to null) doesn't >>>>>> get detected as a change. >>>>>> >>>>> Taken care. >>>>> >>>>>> >>>>>> 4) When I manually executed "update defaults set data_default_nulls = >>>>>> null where id = 2" in a query tool window, I got: >>>>>> >>>>>> 2017-04-07 09:43:02,987: INFO werkzeug: 127.0.0.1 - - [07/Apr/2017 >>>>>> 09:43:02] "GET /sqleditor/columns/8745675 HTTP/1.1" 500 - >>>>>> Traceback (most recent call last): >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>> ges/flask/app.py", >>>>>> line 2000, in __call__ >>>>>> return self.wsgi_app(environ, start_response) >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>> ges/flask/app.py", >>>>>> line 1991, in wsgi_app >>>>>> response = self.make_response(self.handle_exception(e)) >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>> ges/flask/app.py", >>>>>> line 1567, in handle_exception >>>>>> reraise(exc_type, exc_value, tb) >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>> ges/flask/app.py", >>>>>> line 1988, in wsgi_app >>>>>> response = self.full_dispatch_request() >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>> ges/flask/app.py", >>>>>> line 1641, in full_dispatch_request >>>>>> rv = self.handle_user_exception(e) >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>> ges/flask/app.py", >>>>>> line 1544, in handle_user_exception >>>>>> reraise(exc_type, exc_value, tb) >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>> ges/flask/app.py", >>>>>> line 1639, in full_dispatch_request >>>>>> rv = self.dispatch_request() >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>> ges/flask/app.py", >>>>>> line 1625, in dispatch_request >>>>>> return self.view_functions[rule.endpoint](**req.view_args) >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>> ges/flask_login.py", >>>>>> line 792, in decorated_view >>>>>> return func(*args, **kwargs) >>>>>> File "/Users/dpage/git/pgadmin4/web/pgadmin/tools/sqleditor/__ini >>>>>> t__.py", >>>>>> line 452, in get_columns >>>>>> tid=command_obj.obj_id) >>>>>> AttributeError: 'QueryToolCommand' object has no attribute 'obj_id' >>>>>> >>>>> Fixed. >>>>> >>>>>> >>>>>> 5) When I run the query again in pgAdmin III, then refresh the data in >>>>>> pgAdmin 4, the data_default_nulls column is displayed without the >>>>>> [null] marker (despite having a null value, which I confirmed in >>>>>> pgAdmin 3). >>>>>> >>>>> Fixed. >>>>> >>>>>> >>>>>> I'm sure there are other combinations of issues here. Please fix and >>>>>> thoroughly re-test to ensure behaviour is consistent - and to avoid >>>>>> future issues, please add some appropriate feature tests to check >>>>>> nulls, defaults and empty strings are properly handled in view, insert >>>>>> and updates. Murtuza recently wrote some feature tests for the query >>>>>> tool - you should be able to use those as a starting point. >>>>>> >>>>> Added feature tests >>>>> >>>>>> >>>>>> Thanks. >>>>>> >>>>>> -- >>>>>> Dave Page >>>>>> Blog: http://pgsnake.blogspot.com >>>>>> Twitter: @pgsnake >>>>>> >>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>> The Enterprise PostgreSQL Company >>>>>> >>>>> >>>>> >>>> >>>> >>>> -- >>>> Dave Page >>>> Blog: http://pgsnake.blogspot.com >>>> Twitter: @pgsnake >>>> >>>> EnterpriseDB UK: http://www.enterprisedb.com >>>> The Enterprise PostgreSQL Company >>>> >>> >>> >> > > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-08 10:21 Dave Page <[email protected]> parent: Surinder Kumar <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Dave Page @ 2017-05-08 10:21 UTC (permalink / raw) To: Surinder Kumar <[email protected]>; +Cc: pgadmin-hackers Hi On Mon, May 8, 2017 at 11:13 AM, Surinder Kumar < [email protected]> wrote: > Hi Dave, > > On Mon, May 8, 2017 at 3:28 PM, Dave Page <[email protected]> wrote: > >> Hi >> >> On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar < >> [email protected]> wrote: >> >>> Hi Dave, >>> >>> The support to handle [null] and [default] values is added for following >>> formatters: >>> - JsonFormatter >>> - NumbersFormatter >>> - CheckmarkFormatter >>> - TextFormatter >>> >>> Previously when a new row is added, it was not validating each and every >>> cell for columns with attribute not_null = true. >>> Introduced a new function validateRow(...) which is called on adding a >>> new row, it validates each cell with column having >>> attribute(not_null=true). the corresponding cell will highlighted and save >>> button will be disabled if value is [null]. >>> >> >> I'm not sure that behaviour is right. What I now see (given the table >> below) is that: >> >> - If I click in the id field of a new row, it forces me to enter a value >> or hit escape. Why is it trying to force me? It's a serial field, so will >> get a value on save anyway. >> > Yes, It is because I am validating all cell values after it renders. It > will reverted back. > >> >> - If I do enter a value in the ID field, focus jumps over all the other >> fields with either a default or no 'not null' option to the data_no_nulls >> column. Focus should always go to the next field. >> >> I think this addition can just be removed - I'm pretty sure the previous >> behaviour would have been what we want, with the additional formatters >> fixed. >> > But If i remove this addition, the value for column like 'data_no_nulls' > will be set to '' (blank string), then on save its value will be validated > on the server side and whatever the error message is will be returned back > (the same behaviour as in pgAdmin3) > Which is fine I think. If you want to leave the validation there, that's also fine - but, it a) shouldn't require me to press Esc if I decide not to fill in that value yet, and b) shouldn't change the tab order of the fields. It's also broken of course, in that it was trying to force me to enter a value for a not-null field with a default. > >> >>> >>> Now I will add more feature test cases for remaining formatters. Will >>> send separate patch for feature test cases once completed. >>> >> >> Thanks. >> >> >>> >>> Please review updated patch. >>> >>> >>> On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar < >>> [email protected]> wrote: >>> >>>> Hi Dave, >>>> >>>> On Tue, May 2, 2017 at 5:21 PM, Dave Page <[email protected]> wrote: >>>> >>>>> Hi, >>>>> >>>>> This is looking much better now :-). Couple of thoughts and a bug: >>>>> >>>>> - Only the TextFormatter seems to handle both [null] and [default] >>>>> values. Shouldn't all formatters do so (including Json and Checkmark)? >>>>> >>>> Yes, I will apply the same changes for other formatters too. >>>> >>>>> For example, "serial" columns currently get displayed as [null] when >>>>> left blank, but I would expect to see [default]. >>>>> >>>>> - I would suggest we put [null] and [default] in a lighter colour - >>>>> #999999. >>>>> >>>>> - With the feature test patch added, I seem to be consistently getting >>>>> the following failure (immediately after your new tests run): >>>>> >>>>> ====================================================================== >>>>> ERROR: runTest (pgadmin.feature_tests.xss_che >>>>> cks_panels_and_query_tool_test.CheckForXssFeatureTest) >>>>> Test XSS check for panels and query tool >>>>> ---------------------------------------------------------------------- >>>>> Traceback (most recent call last): >>>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>>> line 42, in setUp >>>>> self._screenshot() >>>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>>> line 92, in _screenshot >>>>> python_version)) >>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>> ges/selenium/webdriver/remote/webdriver.py", line 802, in >>>>> get_screenshot_as_file >>>>> png = self.get_screenshot_as_png() >>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>> ges/selenium/webdriver/remote/webdriver.py", line 821, in >>>>> get_screenshot_as_png >>>>> return base64.b64decode(self.get_scre >>>>> enshot_as_base64().encode('ascii')) >>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>> ges/selenium/webdriver/remote/webdriver.py", line 831, in >>>>> get_screenshot_as_base64 >>>>> return self.execute(Command.SCREENSHOT)['value'] >>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>> ges/selenium/webdriver/remote/webdriver.py", line 238, in execute >>>>> self.error_handler.check_response(response) >>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>> ges/selenium/webdriver/remote/errorhandler.py", line 193, in >>>>> check_response >>>>> raise exception_class(message, screen, stacktrace) >>>>> UnexpectedAlertPresentException: Alert Text: None >>>>> Message: unexpected alert open: {Alert text : Are you sure you wish to >>>>> close the pgAdmin 4 browser?} >>>>> (Session info: chrome=58.0.3029.81) >>>>> (Driver info: chromedriver=2.29.461585 (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac >>>>> OS X 10.12.3 x86_64) >>>>> >>>>> >>>>> ====================================================================== >>>>> ERROR: runTest (pgadmin.feature_tests.xss_che >>>>> cks_pgadmin_debugger_test.CheckDebuggerForXssFeatureTest) >>>>> Test table DDL generation >>>>> ---------------------------------------------------------------------- >>>>> Traceback (most recent call last): >>>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>>> line 42, in setUp >>>>> self._screenshot() >>>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>>> line 92, in _screenshot >>>>> python_version)) >>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>> ges/selenium/webdriver/remote/webdriver.py", line 802, in >>>>> get_screenshot_as_file >>>>> png = self.get_screenshot_as_png() >>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>> ges/selenium/webdriver/remote/webdriver.py", line 821, in >>>>> get_screenshot_as_png >>>>> return base64.b64decode(self.get_scre >>>>> enshot_as_base64().encode('ascii')) >>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>> ges/selenium/webdriver/remote/webdriver.py", line 831, in >>>>> get_screenshot_as_base64 >>>>> return self.execute(Command.SCREENSHOT)['value'] >>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>> ges/selenium/webdriver/remote/webdriver.py", line 238, in execute >>>>> self.error_handler.check_response(response) >>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>> ges/selenium/webdriver/remote/errorhandler.py", line 193, in >>>>> check_response >>>>> raise exception_class(message, screen, stacktrace) >>>>> UnexpectedAlertPresentException: Alert Text: None >>>>> Message: unexpected alert open: {Alert text : Are you sure you wish to >>>>> close the pgAdmin 4 browser?} >>>>> (Session info: chrome=58.0.3029.81) >>>>> (Driver info: chromedriver=2.29.461585 (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac >>>>> OS X 10.12.3 x86_64) >>>>> >>>> Sure. I will fix this. >>>> >>>>> >>>>> Thanks! >>>>> >>>>> >>>>> On Fri, Apr 28, 2017 at 10:19 AM, Surinder Kumar < >>>>> [email protected]> wrote: >>>>> >>>>>> Hi Dave, >>>>>> >>>>>> Please find updated patch for RM case and a separate patch for >>>>>> Feature tests. >>>>>> >>>>>> *Python:* >>>>>> >>>>>> - Added [default] label for cells with default values while >>>>>> inserting a new row. >>>>>> >>>>>> - Introduced a FieldValidator function for cells that don't allow >>>>>> null values. If user tries to insert null value, field with be highlighted >>>>>> with red borders around. >>>>>> >>>>>> - >>>>>> If a cell contains blank string('') and when we set it to null, the >>>>>> change into the cell is not detected. It was because the comparison >>>>>> for (defaultValue == null) return true if defaultValue is undefined. >>>>>> Hence _.isNull(value) is used to fix this. >>>>>> >>>>>> *Feature Test cases:* >>>>>> >>>>>> - Introduced a new method create_table_with_query(server, db_name, >>>>>> query) in test_utils.py which executes the given query on connected >>>>>> server. >>>>>> >>>>>> - Added a new file test_data.json that has test data for test cases. >>>>>> >>>>>> >>>>>> On Fri, Apr 7, 2017 at 2:21 PM, Dave Page <[email protected]> wrote: >>>>>> >>>>>>> Hi >>>>>>> >>>>>>> On Sat, Apr 1, 2017 at 12:45 PM, Surinder Kumar >>>>>>> <[email protected]> wrote: >>>>>>> > Hi >>>>>>> > >>>>>>> > Issues fixed: >>>>>>> > >>>>>>> > 1. If a column is defined with a default modifier, there is now >>>>>>> way to >>>>>>> > insert the row with those defaults. >>>>>>> > The column will be left blank and it will take default value >>>>>>> automatically. >>>>>>> > >>>>>>> > 2. If a column has a not-null constraint then an error is returned >>>>>>> and the >>>>>>> > row is not inserted. >>>>>>> > The column will be left blank >>>>>>> > >>>>>>> > The default values for new added rows will be displayed on >>>>>>> refresh/execute. >>>>>>> > >>>>>>> > Please find attached patch and review. >>>>>>> >>>>>>> This largely works as expected, but there is some weirdness. I have a >>>>>>> test table that looks like this: >>>>>>> >>>>>>> CREATE TABLE public.defaults >>>>>>> ( >>>>>>> id bigint NOT NULL DEFAULT nextval('defaults_id_seq'::regclass), >>>>>>> data_default_nulls text COLLATE pg_catalog."default" DEFAULT >>>>>>> 'abc123'::text, >>>>>>> data_default_no_nulls text COLLATE pg_catalog."default" NOT NULL >>>>>>> DEFAULT 'def456'::text, >>>>>>> data_nulls text COLLATE pg_catalog."default", >>>>>>> data_no_nulls text COLLATE pg_catalog."default" NOT NULL, >>>>>>> CONSTRAINT defaults_pkey PRIMARY KEY (id) >>>>>>> ) >>>>>>> >>>>>>> Remember that the expected behaviour is: >>>>>>> >>>>>>> - Set a value to empty to update the column to null. >>>>>>> - Set a value to '' to update the column to an empty string >>>>>>> - Set a value to anything else to update the column to that value >>>>>>> >>>>>>> 1) In a row with values in each column, if I try to set the value of >>>>>>> data_default_nulls to null, the query executed is: >>>>>>> >>>>>>> UPDATE public.defaults SET >>>>>>> data_default_nulls = '' WHERE >>>>>>> id = '2'; >>>>>>> >>>>>>> 2) If I do the same in the data_nulls column, the value is >>>>>>> immediately >>>>>>> shown as [null] and the query executed is: >>>>>>> >>>>>>> UPDATE public.defaults SET >>>>>>> data_nulls = NULL WHERE >>>>>>> id = '2'; >>>>>>> >>>>>>> 3) If I then edit the value in data_default_nulls, it shows the >>>>>>> current value as ''. Removing the quotes (to set it to null) doesn't >>>>>>> get detected as a change. >>>>>>> >>>>>> Taken care. >>>>>> >>>>>>> >>>>>>> 4) When I manually executed "update defaults set data_default_nulls = >>>>>>> null where id = 2" in a query tool window, I got: >>>>>>> >>>>>>> 2017-04-07 09:43:02,987: INFO werkzeug: 127.0.0.1 - - [07/Apr/2017 >>>>>>> 09:43:02] "GET /sqleditor/columns/8745675 HTTP/1.1" 500 - >>>>>>> Traceback (most recent call last): >>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>> ges/flask/app.py", >>>>>>> line 2000, in __call__ >>>>>>> return self.wsgi_app(environ, start_response) >>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>> ges/flask/app.py", >>>>>>> line 1991, in wsgi_app >>>>>>> response = self.make_response(self.handle_exception(e)) >>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>> ges/flask/app.py", >>>>>>> line 1567, in handle_exception >>>>>>> reraise(exc_type, exc_value, tb) >>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>> ges/flask/app.py", >>>>>>> line 1988, in wsgi_app >>>>>>> response = self.full_dispatch_request() >>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>> ges/flask/app.py", >>>>>>> line 1641, in full_dispatch_request >>>>>>> rv = self.handle_user_exception(e) >>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>> ges/flask/app.py", >>>>>>> line 1544, in handle_user_exception >>>>>>> reraise(exc_type, exc_value, tb) >>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>> ges/flask/app.py", >>>>>>> line 1639, in full_dispatch_request >>>>>>> rv = self.dispatch_request() >>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>> ges/flask/app.py", >>>>>>> line 1625, in dispatch_request >>>>>>> return self.view_functions[rule.endpoint](**req.view_args) >>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>> ges/flask_login.py", >>>>>>> line 792, in decorated_view >>>>>>> return func(*args, **kwargs) >>>>>>> File "/Users/dpage/git/pgadmin4/web/pgadmin/tools/sqleditor/__ini >>>>>>> t__.py", >>>>>>> line 452, in get_columns >>>>>>> tid=command_obj.obj_id) >>>>>>> AttributeError: 'QueryToolCommand' object has no attribute 'obj_id' >>>>>>> >>>>>> Fixed. >>>>>> >>>>>>> >>>>>>> 5) When I run the query again in pgAdmin III, then refresh the data >>>>>>> in >>>>>>> pgAdmin 4, the data_default_nulls column is displayed without the >>>>>>> [null] marker (despite having a null value, which I confirmed in >>>>>>> pgAdmin 3). >>>>>>> >>>>>> Fixed. >>>>>> >>>>>>> >>>>>>> I'm sure there are other combinations of issues here. Please fix and >>>>>>> thoroughly re-test to ensure behaviour is consistent - and to avoid >>>>>>> future issues, please add some appropriate feature tests to check >>>>>>> nulls, defaults and empty strings are properly handled in view, >>>>>>> insert >>>>>>> and updates. Murtuza recently wrote some feature tests for the query >>>>>>> tool - you should be able to use those as a starting point. >>>>>>> >>>>>> Added feature tests >>>>>> >>>>>>> >>>>>>> Thanks. >>>>>>> >>>>>>> -- >>>>>>> Dave Page >>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>> Twitter: @pgsnake >>>>>>> >>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>> The Enterprise PostgreSQL Company >>>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>>> -- >>>>> Dave Page >>>>> Blog: http://pgsnake.blogspot.com >>>>> Twitter: @pgsnake >>>>> >>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>> The Enterprise PostgreSQL Company >>>>> >>>> >>>> >>> >> >> >> -- >> Dave Page >> Blog: http://pgsnake.blogspot.com >> Twitter: @pgsnake >> >> EnterpriseDB UK: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-08 10:51 Surinder Kumar <[email protected]> parent: Dave Page <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Surinder Kumar @ 2017-05-08 10:51 UTC (permalink / raw) To: Dave Page <[email protected]>; +Cc: pgadmin-hackers Hi On Mon, May 8, 2017 at 3:51 PM, Dave Page <[email protected]> wrote: > Hi > > On Mon, May 8, 2017 at 11:13 AM, Surinder Kumar < > [email protected]> wrote: > >> Hi Dave, >> >> On Mon, May 8, 2017 at 3:28 PM, Dave Page <[email protected]> wrote: >> >>> Hi >>> >>> On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar < >>> [email protected]> wrote: >>> >>>> Hi Dave, >>>> >>>> The support to handle [null] and [default] values is added for >>>> following formatters: >>>> - JsonFormatter >>>> - NumbersFormatter >>>> - CheckmarkFormatter >>>> - TextFormatter >>>> >>>> Previously when a new row is added, it was not validating each and >>>> every cell for columns with attribute not_null = true. >>>> Introduced a new function validateRow(...) which is called on adding a >>>> new row, it validates each cell with column having >>>> attribute(not_null=true). the corresponding cell will highlighted and save >>>> button will be disabled if value is [null]. >>>> >>> >>> I'm not sure that behaviour is right. What I now see (given the table >>> below) is that: >>> >>> - If I click in the id field of a new row, it forces me to enter a value >>> or hit escape. Why is it trying to force me? It's a serial field, so will >>> get a value on save anyway. >>> >> Yes, It is because I am validating all cell values after it renders. It >> will reverted back. >> >>> >>> - If I do enter a value in the ID field, focus jumps over all the other >>> fields with either a default or no 'not null' option to the data_no_nulls >>> column. Focus should always go to the next field. >>> >>> I think this addition can just be removed - I'm pretty sure the previous >>> behaviour would have been what we want, with the additional formatters >>> fixed. >>> >> But If i remove this addition, the value for column like 'data_no_nulls' >> will be set to '' (blank string), then on save its value will be validated >> on the server side and whatever the error message is will be returned back >> (the same behaviour as in pgAdmin3) >> > > Which is fine I think. If you want to leave the validation there, that's > also fine - but, it a) shouldn't require me to press Esc if I decide not to > fill in that value yet, and b) shouldn't change the tab order of the > fields. It's also broken of course, in that it was trying to force me to > enter a value for a not-null field with a default. > Yes, I will check if we just highlight the field and don't force the user to enter value. This will enable user to edit any field using TAB key even if required field is highlighted red. Should the save button remains disable untill user enters any valid value in 'data_no_nulls' column ? > > >>> >>>> >>>> Now I will add more feature test cases for remaining formatters. Will >>>> send separate patch for feature test cases once completed. >>>> >>> >>> Thanks. >>> >>> >>>> >>>> Please review updated patch. >>>> >>>> >>>> On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar < >>>> [email protected]> wrote: >>>> >>>>> Hi Dave, >>>>> >>>>> On Tue, May 2, 2017 at 5:21 PM, Dave Page <[email protected]> wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> This is looking much better now :-). Couple of thoughts and a bug: >>>>>> >>>>>> - Only the TextFormatter seems to handle both [null] and [default] >>>>>> values. Shouldn't all formatters do so (including Json and Checkmark)? >>>>>> >>>>> Yes, I will apply the same changes for other formatters too. >>>>> >>>>>> For example, "serial" columns currently get displayed as [null] when >>>>>> left blank, but I would expect to see [default]. >>>>>> >>>>>> - I would suggest we put [null] and [default] in a lighter colour - >>>>>> #999999. >>>>>> >>>>>> - With the feature test patch added, I seem to be consistently >>>>>> getting the following failure (immediately after your new tests run): >>>>>> >>>>>> ============================================================ >>>>>> ========== >>>>>> ERROR: runTest (pgadmin.feature_tests.xss_che >>>>>> cks_panels_and_query_tool_test.CheckForXssFeatureTest) >>>>>> Test XSS check for panels and query tool >>>>>> ------------------------------------------------------------ >>>>>> ---------- >>>>>> Traceback (most recent call last): >>>>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>>>> line 42, in setUp >>>>>> self._screenshot() >>>>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>>>> line 92, in _screenshot >>>>>> python_version)) >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>> ges/selenium/webdriver/remote/webdriver.py", line 802, in >>>>>> get_screenshot_as_file >>>>>> png = self.get_screenshot_as_png() >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>> ges/selenium/webdriver/remote/webdriver.py", line 821, in >>>>>> get_screenshot_as_png >>>>>> return base64.b64decode(self.get_scre >>>>>> enshot_as_base64().encode('ascii')) >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>> ges/selenium/webdriver/remote/webdriver.py", line 831, in >>>>>> get_screenshot_as_base64 >>>>>> return self.execute(Command.SCREENSHOT)['value'] >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>> ges/selenium/webdriver/remote/webdriver.py", line 238, in execute >>>>>> self.error_handler.check_response(response) >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>> ges/selenium/webdriver/remote/errorhandler.py", line 193, in >>>>>> check_response >>>>>> raise exception_class(message, screen, stacktrace) >>>>>> UnexpectedAlertPresentException: Alert Text: None >>>>>> Message: unexpected alert open: {Alert text : Are you sure you wish >>>>>> to close the pgAdmin 4 browser?} >>>>>> (Session info: chrome=58.0.3029.81) >>>>>> (Driver info: chromedriver=2.29.461585 >>>>>> (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac OS X 10.12.3 >>>>>> x86_64) >>>>>> >>>>>> >>>>>> ============================================================ >>>>>> ========== >>>>>> ERROR: runTest (pgadmin.feature_tests.xss_che >>>>>> cks_pgadmin_debugger_test.CheckDebuggerForXssFeatureTest) >>>>>> Test table DDL generation >>>>>> ------------------------------------------------------------ >>>>>> ---------- >>>>>> Traceback (most recent call last): >>>>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>>>> line 42, in setUp >>>>>> self._screenshot() >>>>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>>>> line 92, in _screenshot >>>>>> python_version)) >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>> ges/selenium/webdriver/remote/webdriver.py", line 802, in >>>>>> get_screenshot_as_file >>>>>> png = self.get_screenshot_as_png() >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>> ges/selenium/webdriver/remote/webdriver.py", line 821, in >>>>>> get_screenshot_as_png >>>>>> return base64.b64decode(self.get_scre >>>>>> enshot_as_base64().encode('ascii')) >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>> ges/selenium/webdriver/remote/webdriver.py", line 831, in >>>>>> get_screenshot_as_base64 >>>>>> return self.execute(Command.SCREENSHOT)['value'] >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>> ges/selenium/webdriver/remote/webdriver.py", line 238, in execute >>>>>> self.error_handler.check_response(response) >>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>> ges/selenium/webdriver/remote/errorhandler.py", line 193, in >>>>>> check_response >>>>>> raise exception_class(message, screen, stacktrace) >>>>>> UnexpectedAlertPresentException: Alert Text: None >>>>>> Message: unexpected alert open: {Alert text : Are you sure you wish >>>>>> to close the pgAdmin 4 browser?} >>>>>> (Session info: chrome=58.0.3029.81) >>>>>> (Driver info: chromedriver=2.29.461585 >>>>>> (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac OS X 10.12.3 >>>>>> x86_64) >>>>>> >>>>> Sure. I will fix this. >>>>> >>>>>> >>>>>> Thanks! >>>>>> >>>>>> >>>>>> On Fri, Apr 28, 2017 at 10:19 AM, Surinder Kumar < >>>>>> [email protected]> wrote: >>>>>> >>>>>>> Hi Dave, >>>>>>> >>>>>>> Please find updated patch for RM case and a separate patch for >>>>>>> Feature tests. >>>>>>> >>>>>>> *Python:* >>>>>>> >>>>>>> - Added [default] label for cells with default values while >>>>>>> inserting a new row. >>>>>>> >>>>>>> - Introduced a FieldValidator function for cells that don't allow >>>>>>> null values. If user tries to insert null value, field with be highlighted >>>>>>> with red borders around. >>>>>>> >>>>>>> - >>>>>>> If a cell contains blank string('') and when we set it to null, the >>>>>>> change into the cell is not detected. It was because the comparison >>>>>>> for (defaultValue == null) return true if defaultValue is undefined. >>>>>>> Hence _.isNull(value) is used to fix this. >>>>>>> >>>>>>> *Feature Test cases:* >>>>>>> >>>>>>> - Introduced a new method create_table_with_query(server, db_name, >>>>>>> query) in test_utils.py which executes the given query on >>>>>>> connected server. >>>>>>> >>>>>>> - Added a new file test_data.json that has test data for test cases. >>>>>>> >>>>>>> >>>>>>> On Fri, Apr 7, 2017 at 2:21 PM, Dave Page <[email protected]> wrote: >>>>>>> >>>>>>>> Hi >>>>>>>> >>>>>>>> On Sat, Apr 1, 2017 at 12:45 PM, Surinder Kumar >>>>>>>> <[email protected]> wrote: >>>>>>>> > Hi >>>>>>>> > >>>>>>>> > Issues fixed: >>>>>>>> > >>>>>>>> > 1. If a column is defined with a default modifier, there is now >>>>>>>> way to >>>>>>>> > insert the row with those defaults. >>>>>>>> > The column will be left blank and it will take default value >>>>>>>> automatically. >>>>>>>> > >>>>>>>> > 2. If a column has a not-null constraint then an error is >>>>>>>> returned and the >>>>>>>> > row is not inserted. >>>>>>>> > The column will be left blank >>>>>>>> > >>>>>>>> > The default values for new added rows will be displayed on >>>>>>>> refresh/execute. >>>>>>>> > >>>>>>>> > Please find attached patch and review. >>>>>>>> >>>>>>>> This largely works as expected, but there is some weirdness. I have >>>>>>>> a >>>>>>>> test table that looks like this: >>>>>>>> >>>>>>>> CREATE TABLE public.defaults >>>>>>>> ( >>>>>>>> id bigint NOT NULL DEFAULT nextval('defaults_id_seq'::reg >>>>>>>> class), >>>>>>>> data_default_nulls text COLLATE pg_catalog."default" DEFAULT >>>>>>>> 'abc123'::text, >>>>>>>> data_default_no_nulls text COLLATE pg_catalog."default" NOT NULL >>>>>>>> DEFAULT 'def456'::text, >>>>>>>> data_nulls text COLLATE pg_catalog."default", >>>>>>>> data_no_nulls text COLLATE pg_catalog."default" NOT NULL, >>>>>>>> CONSTRAINT defaults_pkey PRIMARY KEY (id) >>>>>>>> ) >>>>>>>> >>>>>>>> Remember that the expected behaviour is: >>>>>>>> >>>>>>>> - Set a value to empty to update the column to null. >>>>>>>> - Set a value to '' to update the column to an empty string >>>>>>>> - Set a value to anything else to update the column to that value >>>>>>>> >>>>>>>> 1) In a row with values in each column, if I try to set the value of >>>>>>>> data_default_nulls to null, the query executed is: >>>>>>>> >>>>>>>> UPDATE public.defaults SET >>>>>>>> data_default_nulls = '' WHERE >>>>>>>> id = '2'; >>>>>>>> >>>>>>>> 2) If I do the same in the data_nulls column, the value is >>>>>>>> immediately >>>>>>>> shown as [null] and the query executed is: >>>>>>>> >>>>>>>> UPDATE public.defaults SET >>>>>>>> data_nulls = NULL WHERE >>>>>>>> id = '2'; >>>>>>>> >>>>>>>> 3) If I then edit the value in data_default_nulls, it shows the >>>>>>>> current value as ''. Removing the quotes (to set it to null) doesn't >>>>>>>> get detected as a change. >>>>>>>> >>>>>>> Taken care. >>>>>>> >>>>>>>> >>>>>>>> 4) When I manually executed "update defaults set data_default_nulls >>>>>>>> = >>>>>>>> null where id = 2" in a query tool window, I got: >>>>>>>> >>>>>>>> 2017-04-07 09:43:02,987: INFO werkzeug: 127.0.0.1 - - [07/Apr/2017 >>>>>>>> 09:43:02] "GET /sqleditor/columns/8745675 HTTP/1.1" 500 - >>>>>>>> Traceback (most recent call last): >>>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>>> ges/flask/app.py", >>>>>>>> line 2000, in __call__ >>>>>>>> return self.wsgi_app(environ, start_response) >>>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>>> ges/flask/app.py", >>>>>>>> line 1991, in wsgi_app >>>>>>>> response = self.make_response(self.handle_exception(e)) >>>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>>> ges/flask/app.py", >>>>>>>> line 1567, in handle_exception >>>>>>>> reraise(exc_type, exc_value, tb) >>>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>>> ges/flask/app.py", >>>>>>>> line 1988, in wsgi_app >>>>>>>> response = self.full_dispatch_request() >>>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>>> ges/flask/app.py", >>>>>>>> line 1641, in full_dispatch_request >>>>>>>> rv = self.handle_user_exception(e) >>>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>>> ges/flask/app.py", >>>>>>>> line 1544, in handle_user_exception >>>>>>>> reraise(exc_type, exc_value, tb) >>>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>>> ges/flask/app.py", >>>>>>>> line 1639, in full_dispatch_request >>>>>>>> rv = self.dispatch_request() >>>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>>> ges/flask/app.py", >>>>>>>> line 1625, in dispatch_request >>>>>>>> return self.view_functions[rule.endpoint](**req.view_args) >>>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>>> ges/flask_login.py", >>>>>>>> line 792, in decorated_view >>>>>>>> return func(*args, **kwargs) >>>>>>>> File "/Users/dpage/git/pgadmin4/web/pgadmin/tools/sqleditor/__ini >>>>>>>> t__.py", >>>>>>>> line 452, in get_columns >>>>>>>> tid=command_obj.obj_id) >>>>>>>> AttributeError: 'QueryToolCommand' object has no attribute 'obj_id' >>>>>>>> >>>>>>> Fixed. >>>>>>> >>>>>>>> >>>>>>>> 5) When I run the query again in pgAdmin III, then refresh the data >>>>>>>> in >>>>>>>> pgAdmin 4, the data_default_nulls column is displayed without the >>>>>>>> [null] marker (despite having a null value, which I confirmed in >>>>>>>> pgAdmin 3). >>>>>>>> >>>>>>> Fixed. >>>>>>> >>>>>>>> >>>>>>>> I'm sure there are other combinations of issues here. Please fix and >>>>>>>> thoroughly re-test to ensure behaviour is consistent - and to avoid >>>>>>>> future issues, please add some appropriate feature tests to check >>>>>>>> nulls, defaults and empty strings are properly handled in view, >>>>>>>> insert >>>>>>>> and updates. Murtuza recently wrote some feature tests for the query >>>>>>>> tool - you should be able to use those as a starting point. >>>>>>>> >>>>>>> Added feature tests >>>>>>> >>>>>>>> >>>>>>>> Thanks. >>>>>>>> >>>>>>>> -- >>>>>>>> Dave Page >>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>> Twitter: @pgsnake >>>>>>>> >>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>> The Enterprise PostgreSQL Company >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Dave Page >>>>>> Blog: http://pgsnake.blogspot.com >>>>>> Twitter: @pgsnake >>>>>> >>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>> The Enterprise PostgreSQL Company >>>>>> >>>>> >>>>> >>>> >>> >>> >>> -- >>> Dave Page >>> Blog: http://pgsnake.blogspot.com >>> Twitter: @pgsnake >>> >>> EnterpriseDB UK: http://www.enterprisedb.com >>> The Enterprise PostgreSQL Company >>> >> >> > > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-08 11:07 Dave Page <[email protected]> parent: Surinder Kumar <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Dave Page @ 2017-05-08 11:07 UTC (permalink / raw) To: Surinder Kumar <[email protected]>; +Cc: pgadmin-hackers On Mon, May 8, 2017 at 11:51 AM, Surinder Kumar < [email protected]> wrote: > Hi > > On Mon, May 8, 2017 at 3:51 PM, Dave Page <[email protected]> wrote: > >> Hi >> >> On Mon, May 8, 2017 at 11:13 AM, Surinder Kumar < >> [email protected]> wrote: >> >>> Hi Dave, >>> >>> On Mon, May 8, 2017 at 3:28 PM, Dave Page <[email protected]> wrote: >>> >>>> Hi >>>> >>>> On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar < >>>> [email protected]> wrote: >>>> >>>>> Hi Dave, >>>>> >>>>> The support to handle [null] and [default] values is added for >>>>> following formatters: >>>>> - JsonFormatter >>>>> - NumbersFormatter >>>>> - CheckmarkFormatter >>>>> - TextFormatter >>>>> >>>>> Previously when a new row is added, it was not validating each and >>>>> every cell for columns with attribute not_null = true. >>>>> Introduced a new function validateRow(...) which is called on adding >>>>> a new row, it validates each cell with column having >>>>> attribute(not_null=true). the corresponding cell will highlighted and save >>>>> button will be disabled if value is [null]. >>>>> >>>> >>>> I'm not sure that behaviour is right. What I now see (given the table >>>> below) is that: >>>> >>>> - If I click in the id field of a new row, it forces me to enter a >>>> value or hit escape. Why is it trying to force me? It's a serial field, so >>>> will get a value on save anyway. >>>> >>> Yes, It is because I am validating all cell values after it renders. It >>> will reverted back. >>> >>>> >>>> - If I do enter a value in the ID field, focus jumps over all the other >>>> fields with either a default or no 'not null' option to the data_no_nulls >>>> column. Focus should always go to the next field. >>>> >>>> I think this addition can just be removed - I'm pretty sure the >>>> previous behaviour would have been what we want, with the additional >>>> formatters fixed. >>>> >>> But If i remove this addition, the value for column like >>> 'data_no_nulls' will be set to '' (blank string), then on save its value >>> will be validated on the server side and whatever the error message is will >>> be returned back (the same behaviour as in pgAdmin3) >>> >> >> Which is fine I think. If you want to leave the validation there, that's >> also fine - but, it a) shouldn't require me to press Esc if I decide not to >> fill in that value yet, and b) shouldn't change the tab order of the >> fields. It's also broken of course, in that it was trying to force me to >> enter a value for a not-null field with a default. >> > Yes, I will check if we just highlight the field and don't force the user > to enter value. > This will enable user to edit any field using TAB key even if required > field is highlighted red. > Should the save button remains disable untill user enters any valid value > in 'data_no_nulls' column ? > I think that's fine, yes - as long as we get the validation right :-) > >> > >>>> >>>>> >>>>> Now I will add more feature test cases for remaining formatters. Will >>>>> send separate patch for feature test cases once completed. >>>>> >>>> >>>> Thanks. >>>> >>>> >>>>> >>>>> Please review updated patch. >>>>> >>>>> >>>>> On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar < >>>>> [email protected]> wrote: >>>>> >>>>>> Hi Dave, >>>>>> >>>>>> On Tue, May 2, 2017 at 5:21 PM, Dave Page <[email protected]> wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> This is looking much better now :-). Couple of thoughts and a bug: >>>>>>> >>>>>>> - Only the TextFormatter seems to handle both [null] and [default] >>>>>>> values. Shouldn't all formatters do so (including Json and Checkmark)? >>>>>>> >>>>>> Yes, I will apply the same changes for other formatters too. >>>>>> >>>>>>> For example, "serial" columns currently get displayed as [null] when >>>>>>> left blank, but I would expect to see [default]. >>>>>>> >>>>>>> - I would suggest we put [null] and [default] in a lighter colour - >>>>>>> #999999. >>>>>>> >>>>>>> - With the feature test patch added, I seem to be consistently >>>>>>> getting the following failure (immediately after your new tests run): >>>>>>> >>>>>>> ============================================================ >>>>>>> ========== >>>>>>> ERROR: runTest (pgadmin.feature_tests.xss_che >>>>>>> cks_panels_and_query_tool_test.CheckForXssFeatureTest) >>>>>>> Test XSS check for panels and query tool >>>>>>> ------------------------------------------------------------ >>>>>>> ---------- >>>>>>> Traceback (most recent call last): >>>>>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>>>>> line 42, in setUp >>>>>>> self._screenshot() >>>>>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>>>>> line 92, in _screenshot >>>>>>> python_version)) >>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>> ges/selenium/webdriver/remote/webdriver.py", line 802, in >>>>>>> get_screenshot_as_file >>>>>>> png = self.get_screenshot_as_png() >>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>> ges/selenium/webdriver/remote/webdriver.py", line 821, in >>>>>>> get_screenshot_as_png >>>>>>> return base64.b64decode(self.get_scre >>>>>>> enshot_as_base64().encode('ascii')) >>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>> ges/selenium/webdriver/remote/webdriver.py", line 831, in >>>>>>> get_screenshot_as_base64 >>>>>>> return self.execute(Command.SCREENSHOT)['value'] >>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>> ges/selenium/webdriver/remote/webdriver.py", line 238, in execute >>>>>>> self.error_handler.check_response(response) >>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>> ges/selenium/webdriver/remote/errorhandler.py", line 193, in >>>>>>> check_response >>>>>>> raise exception_class(message, screen, stacktrace) >>>>>>> UnexpectedAlertPresentException: Alert Text: None >>>>>>> Message: unexpected alert open: {Alert text : Are you sure you wish >>>>>>> to close the pgAdmin 4 browser?} >>>>>>> (Session info: chrome=58.0.3029.81) >>>>>>> (Driver info: chromedriver=2.29.461585 >>>>>>> (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac OS X >>>>>>> 10.12.3 x86_64) >>>>>>> >>>>>>> >>>>>>> ============================================================ >>>>>>> ========== >>>>>>> ERROR: runTest (pgadmin.feature_tests.xss_che >>>>>>> cks_pgadmin_debugger_test.CheckDebuggerForXssFeatureTest) >>>>>>> Test table DDL generation >>>>>>> ------------------------------------------------------------ >>>>>>> ---------- >>>>>>> Traceback (most recent call last): >>>>>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>>>>> line 42, in setUp >>>>>>> self._screenshot() >>>>>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>>>>> line 92, in _screenshot >>>>>>> python_version)) >>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>> ges/selenium/webdriver/remote/webdriver.py", line 802, in >>>>>>> get_screenshot_as_file >>>>>>> png = self.get_screenshot_as_png() >>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>> ges/selenium/webdriver/remote/webdriver.py", line 821, in >>>>>>> get_screenshot_as_png >>>>>>> return base64.b64decode(self.get_scre >>>>>>> enshot_as_base64().encode('ascii')) >>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>> ges/selenium/webdriver/remote/webdriver.py", line 831, in >>>>>>> get_screenshot_as_base64 >>>>>>> return self.execute(Command.SCREENSHOT)['value'] >>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>> ges/selenium/webdriver/remote/webdriver.py", line 238, in execute >>>>>>> self.error_handler.check_response(response) >>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>> ges/selenium/webdriver/remote/errorhandler.py", line 193, in >>>>>>> check_response >>>>>>> raise exception_class(message, screen, stacktrace) >>>>>>> UnexpectedAlertPresentException: Alert Text: None >>>>>>> Message: unexpected alert open: {Alert text : Are you sure you wish >>>>>>> to close the pgAdmin 4 browser?} >>>>>>> (Session info: chrome=58.0.3029.81) >>>>>>> (Driver info: chromedriver=2.29.461585 >>>>>>> (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac OS X >>>>>>> 10.12.3 x86_64) >>>>>>> >>>>>> Sure. I will fix this. >>>>>> >>>>>>> >>>>>>> Thanks! >>>>>>> >>>>>>> >>>>>>> On Fri, Apr 28, 2017 at 10:19 AM, Surinder Kumar < >>>>>>> [email protected]> wrote: >>>>>>> >>>>>>>> Hi Dave, >>>>>>>> >>>>>>>> Please find updated patch for RM case and a separate patch for >>>>>>>> Feature tests. >>>>>>>> >>>>>>>> *Python:* >>>>>>>> >>>>>>>> - Added [default] label for cells with default values while >>>>>>>> inserting a new row. >>>>>>>> >>>>>>>> - Introduced a FieldValidator function for cells that don't allow >>>>>>>> null values. If user tries to insert null value, field with be highlighted >>>>>>>> with red borders around. >>>>>>>> >>>>>>>> - >>>>>>>> If a cell contains blank string('') and when we set it to null, the >>>>>>>> change into the cell is not detected. It was because the comparison >>>>>>>> for (defaultValue == null) return true if defaultValue is >>>>>>>> undefined. Hence _.isNull(value) is used to fix this. >>>>>>>> >>>>>>>> *Feature Test cases:* >>>>>>>> >>>>>>>> - Introduced a new method create_table_with_query(server, >>>>>>>> db_name, query) in test_utils.py which executes the given query >>>>>>>> on connected server. >>>>>>>> >>>>>>>> - Added a new file test_data.json that has test data for test >>>>>>>> cases. >>>>>>>> >>>>>>>> >>>>>>>> On Fri, Apr 7, 2017 at 2:21 PM, Dave Page <[email protected]> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> Hi >>>>>>>>> >>>>>>>>> On Sat, Apr 1, 2017 at 12:45 PM, Surinder Kumar >>>>>>>>> <[email protected]> wrote: >>>>>>>>> > Hi >>>>>>>>> > >>>>>>>>> > Issues fixed: >>>>>>>>> > >>>>>>>>> > 1. If a column is defined with a default modifier, there is now >>>>>>>>> way to >>>>>>>>> > insert the row with those defaults. >>>>>>>>> > The column will be left blank and it will take default value >>>>>>>>> automatically. >>>>>>>>> > >>>>>>>>> > 2. If a column has a not-null constraint then an error is >>>>>>>>> returned and the >>>>>>>>> > row is not inserted. >>>>>>>>> > The column will be left blank >>>>>>>>> > >>>>>>>>> > The default values for new added rows will be displayed on >>>>>>>>> refresh/execute. >>>>>>>>> > >>>>>>>>> > Please find attached patch and review. >>>>>>>>> >>>>>>>>> This largely works as expected, but there is some weirdness. I >>>>>>>>> have a >>>>>>>>> test table that looks like this: >>>>>>>>> >>>>>>>>> CREATE TABLE public.defaults >>>>>>>>> ( >>>>>>>>> id bigint NOT NULL DEFAULT nextval('defaults_id_seq'::reg >>>>>>>>> class), >>>>>>>>> data_default_nulls text COLLATE pg_catalog."default" DEFAULT >>>>>>>>> 'abc123'::text, >>>>>>>>> data_default_no_nulls text COLLATE pg_catalog."default" NOT >>>>>>>>> NULL >>>>>>>>> DEFAULT 'def456'::text, >>>>>>>>> data_nulls text COLLATE pg_catalog."default", >>>>>>>>> data_no_nulls text COLLATE pg_catalog."default" NOT NULL, >>>>>>>>> CONSTRAINT defaults_pkey PRIMARY KEY (id) >>>>>>>>> ) >>>>>>>>> >>>>>>>>> Remember that the expected behaviour is: >>>>>>>>> >>>>>>>>> - Set a value to empty to update the column to null. >>>>>>>>> - Set a value to '' to update the column to an empty string >>>>>>>>> - Set a value to anything else to update the column to that value >>>>>>>>> >>>>>>>>> 1) In a row with values in each column, if I try to set the value >>>>>>>>> of >>>>>>>>> data_default_nulls to null, the query executed is: >>>>>>>>> >>>>>>>>> UPDATE public.defaults SET >>>>>>>>> data_default_nulls = '' WHERE >>>>>>>>> id = '2'; >>>>>>>>> >>>>>>>>> 2) If I do the same in the data_nulls column, the value is >>>>>>>>> immediately >>>>>>>>> shown as [null] and the query executed is: >>>>>>>>> >>>>>>>>> UPDATE public.defaults SET >>>>>>>>> data_nulls = NULL WHERE >>>>>>>>> id = '2'; >>>>>>>>> >>>>>>>>> 3) If I then edit the value in data_default_nulls, it shows the >>>>>>>>> current value as ''. Removing the quotes (to set it to null) >>>>>>>>> doesn't >>>>>>>>> get detected as a change. >>>>>>>>> >>>>>>>> Taken care. >>>>>>>> >>>>>>>>> >>>>>>>>> 4) When I manually executed "update defaults set >>>>>>>>> data_default_nulls = >>>>>>>>> null where id = 2" in a query tool window, I got: >>>>>>>>> >>>>>>>>> 2017-04-07 09:43:02,987: INFO werkzeug: 127.0.0.1 - - [07/Apr/2017 >>>>>>>>> 09:43:02] "GET /sqleditor/columns/8745675 HTTP/1.1" 500 - >>>>>>>>> Traceback (most recent call last): >>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>> line 2000, in __call__ >>>>>>>>> return self.wsgi_app(environ, start_response) >>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>> line 1991, in wsgi_app >>>>>>>>> response = self.make_response(self.handle_exception(e)) >>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>> line 1567, in handle_exception >>>>>>>>> reraise(exc_type, exc_value, tb) >>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>> line 1988, in wsgi_app >>>>>>>>> response = self.full_dispatch_request() >>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>> line 1641, in full_dispatch_request >>>>>>>>> rv = self.handle_user_exception(e) >>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>> line 1544, in handle_user_exception >>>>>>>>> reraise(exc_type, exc_value, tb) >>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>> line 1639, in full_dispatch_request >>>>>>>>> rv = self.dispatch_request() >>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>> line 1625, in dispatch_request >>>>>>>>> return self.view_functions[rule.endpoint](**req.view_args) >>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>> dmin4/lib/python2.7/site-packages/flask_login.py", >>>>>>>>> line 792, in decorated_view >>>>>>>>> return func(*args, **kwargs) >>>>>>>>> File "/Users/dpage/git/pgadmin4/web >>>>>>>>> /pgadmin/tools/sqleditor/__init__.py", >>>>>>>>> line 452, in get_columns >>>>>>>>> tid=command_obj.obj_id) >>>>>>>>> AttributeError: 'QueryToolCommand' object has no attribute 'obj_id' >>>>>>>>> >>>>>>>> Fixed. >>>>>>>> >>>>>>>>> >>>>>>>>> 5) When I run the query again in pgAdmin III, then refresh the >>>>>>>>> data in >>>>>>>>> pgAdmin 4, the data_default_nulls column is displayed without the >>>>>>>>> [null] marker (despite having a null value, which I confirmed in >>>>>>>>> pgAdmin 3). >>>>>>>>> >>>>>>>> Fixed. >>>>>>>> >>>>>>>>> >>>>>>>>> I'm sure there are other combinations of issues here. Please fix >>>>>>>>> and >>>>>>>>> thoroughly re-test to ensure behaviour is consistent - and to avoid >>>>>>>>> future issues, please add some appropriate feature tests to check >>>>>>>>> nulls, defaults and empty strings are properly handled in view, >>>>>>>>> insert >>>>>>>>> and updates. Murtuza recently wrote some feature tests for the >>>>>>>>> query >>>>>>>>> tool - you should be able to use those as a starting point. >>>>>>>>> >>>>>>>> Added feature tests >>>>>>>> >>>>>>>>> >>>>>>>>> Thanks. >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Dave Page >>>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>>> Twitter: @pgsnake >>>>>>>>> >>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>>> The Enterprise PostgreSQL Company >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Dave Page >>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>> Twitter: @pgsnake >>>>>>> >>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>> The Enterprise PostgreSQL Company >>>>>>> >>>>>> >>>>>> >>>>> >>>> >>>> >>>> -- >>>> Dave Page >>>> Blog: http://pgsnake.blogspot.com >>>> Twitter: @pgsnake >>>> >>>> EnterpriseDB UK: http://www.enterprisedb.com >>>> The Enterprise PostgreSQL Company >>>> >>> >>> >> >> >> -- >> Dave Page >> Blog: http://pgsnake.blogspot.com >> Twitter: @pgsnake >> >> EnterpriseDB UK: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-09 10:03 Surinder Kumar <[email protected]> parent: Dave Page <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Surinder Kumar @ 2017-05-09 10:03 UTC (permalink / raw) To: Dave Page <[email protected]>; +Cc: pgadmin-hackers Hi Dave, On Mon, May 8, 2017 at 4:37 PM, Dave Page <[email protected]> wrote: > > > On Mon, May 8, 2017 at 11:51 AM, Surinder Kumar < > [email protected]> wrote: > >> Hi >> >> On Mon, May 8, 2017 at 3:51 PM, Dave Page <[email protected]> wrote: >> >>> Hi >>> >>> On Mon, May 8, 2017 at 11:13 AM, Surinder Kumar < >>> [email protected]> wrote: >>> >>>> Hi Dave, >>>> >>>> On Mon, May 8, 2017 at 3:28 PM, Dave Page <[email protected]> wrote: >>>> >>>>> Hi >>>>> >>>>> On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar < >>>>> [email protected]> wrote: >>>>> >>>>>> Hi Dave, >>>>>> >>>>>> The support to handle [null] and [default] values is added for >>>>>> following formatters: >>>>>> - JsonFormatter >>>>>> - NumbersFormatter >>>>>> - CheckmarkFormatter >>>>>> - TextFormatter >>>>>> >>>>>> Previously when a new row is added, it was not validating each and >>>>>> every cell for columns with attribute not_null = true. >>>>>> Introduced a new function validateRow(...) which is called on adding >>>>>> a new row, it validates each cell with column having >>>>>> attribute(not_null=true). the corresponding cell will highlighted and save >>>>>> button will be disabled if value is [null]. >>>>>> >>>>> >>>>> I'm not sure that behaviour is right. What I now see (given the table >>>>> below) is that: >>>>> >>>>> - If I click in the id field of a new row, it forces me to enter a >>>>> value or hit escape. Why is it trying to force me? It's a serial field, so >>>>> will get a value on save anyway. >>>>> >>>> Yes, It is because I am validating all cell values after it renders. >>>> It will reverted back. >>>> >>>>> >>>>> - If I do enter a value in the ID field, focus jumps over all the >>>>> other fields with either a default or no 'not null' option to the >>>>> data_no_nulls column. Focus should always go to the next field. >>>>> >>>>> I think this addition can just be removed - I'm pretty sure the >>>>> previous behaviour would have been what we want, with the additional >>>>> formatters fixed. >>>>> >>>> But If i remove this addition, the value for column like >>>> 'data_no_nulls' will be set to '' (blank string), then on save its value >>>> will be validated on the server side and whatever the error message is will >>>> be returned back (the same behaviour as in pgAdmin3) >>>> >>> >>> Which is fine I think. If you want to leave the validation there, that's >>> also fine - but, it a) shouldn't require me to press Esc if I decide not to >>> fill in that value yet, and b) shouldn't change the tab order of the >>> fields. It's also broken of course, in that it was trying to force me to >>> enter a value for a not-null field with a default. >>> >> Yes, I will check if we just highlight the field and don't force the >> user to enter value. >> This will enable user to edit any field using TAB key even if required >> field is highlighted red. >> Should the save button remains disable untill user enters any valid value >> in 'data_no_nulls' column ? >> > > I think that's fine, yes - as long as we get the validation right :-) > For highlighting the error field and enable user to navigate to other cells using TAB key, I spend some time looking and debugging into the code and found that it requires lot of changes in slick.grid.js core functions as there are no event listeners provided to listen and changes in core file is not preferred. It may also break other functionalities as code is quite complex. So, I think we should validate data on server side and display any error messages on UI. > >>> >> >>>>> >>>>>> >>>>>> Now I will add more feature test cases for remaining formatters. Will >>>>>> send separate patch for feature test cases once completed. >>>>>> >>>>> >>>>> Thanks. >>>>> >>>>> >>>>>> >>>>>> Please review updated patch. >>>>>> >>>>>> >>>>>> On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar < >>>>>> [email protected]> wrote: >>>>>> >>>>>>> Hi Dave, >>>>>>> >>>>>>> On Tue, May 2, 2017 at 5:21 PM, Dave Page <[email protected]> wrote: >>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>> This is looking much better now :-). Couple of thoughts and a bug: >>>>>>>> >>>>>>>> - Only the TextFormatter seems to handle both [null] and [default] >>>>>>>> values. Shouldn't all formatters do so (including Json and Checkmark)? >>>>>>>> >>>>>>> Yes, I will apply the same changes for other formatters too. >>>>>>> >>>>>>>> For example, "serial" columns currently get displayed as [null] >>>>>>>> when left blank, but I would expect to see [default]. >>>>>>>> >>>>>>>> - I would suggest we put [null] and [default] in a lighter colour - >>>>>>>> #999999. >>>>>>>> >>>>>>>> - With the feature test patch added, I seem to be consistently >>>>>>>> getting the following failure (immediately after your new tests run): >>>>>>>> >>>>>>>> ============================================================ >>>>>>>> ========== >>>>>>>> ERROR: runTest (pgadmin.feature_tests.xss_che >>>>>>>> cks_panels_and_query_tool_test.CheckForXssFeatureTest) >>>>>>>> Test XSS check for panels and query tool >>>>>>>> ------------------------------------------------------------ >>>>>>>> ---------- >>>>>>>> Traceback (most recent call last): >>>>>>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>>>>>> line 42, in setUp >>>>>>>> self._screenshot() >>>>>>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>>>>>> line 92, in _screenshot >>>>>>>> python_version)) >>>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>>> ges/selenium/webdriver/remote/webdriver.py", line 802, in >>>>>>>> get_screenshot_as_file >>>>>>>> png = self.get_screenshot_as_png() >>>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>>> ges/selenium/webdriver/remote/webdriver.py", line 821, in >>>>>>>> get_screenshot_as_png >>>>>>>> return base64.b64decode(self.get_scre >>>>>>>> enshot_as_base64().encode('ascii')) >>>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>>> ges/selenium/webdriver/remote/webdriver.py", line 831, in >>>>>>>> get_screenshot_as_base64 >>>>>>>> return self.execute(Command.SCREENSHOT)['value'] >>>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>>> ges/selenium/webdriver/remote/webdriver.py", line 238, in execute >>>>>>>> self.error_handler.check_response(response) >>>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>>> ges/selenium/webdriver/remote/errorhandler.py", line 193, in >>>>>>>> check_response >>>>>>>> raise exception_class(message, screen, stacktrace) >>>>>>>> UnexpectedAlertPresentException: Alert Text: None >>>>>>>> Message: unexpected alert open: {Alert text : Are you sure you wish >>>>>>>> to close the pgAdmin 4 browser?} >>>>>>>> (Session info: chrome=58.0.3029.81) >>>>>>>> (Driver info: chromedriver=2.29.461585 >>>>>>>> (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac OS X >>>>>>>> 10.12.3 x86_64) >>>>>>>> >>>>>>>> >>>>>>>> ============================================================ >>>>>>>> ========== >>>>>>>> ERROR: runTest (pgadmin.feature_tests.xss_che >>>>>>>> cks_pgadmin_debugger_test.CheckDebuggerForXssFeatureTest) >>>>>>>> Test table DDL generation >>>>>>>> ------------------------------------------------------------ >>>>>>>> ---------- >>>>>>>> Traceback (most recent call last): >>>>>>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>>>>>> line 42, in setUp >>>>>>>> self._screenshot() >>>>>>>> File "/Users/dpage/git/pgadmin4/web/regression/feature_utils/base_feature_test.py", >>>>>>>> line 92, in _screenshot >>>>>>>> python_version)) >>>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>>> ges/selenium/webdriver/remote/webdriver.py", line 802, in >>>>>>>> get_screenshot_as_file >>>>>>>> png = self.get_screenshot_as_png() >>>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>>> ges/selenium/webdriver/remote/webdriver.py", line 821, in >>>>>>>> get_screenshot_as_png >>>>>>>> return base64.b64decode(self.get_scre >>>>>>>> enshot_as_base64().encode('ascii')) >>>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>>> ges/selenium/webdriver/remote/webdriver.py", line 831, in >>>>>>>> get_screenshot_as_base64 >>>>>>>> return self.execute(Command.SCREENSHOT)['value'] >>>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>>> ges/selenium/webdriver/remote/webdriver.py", line 238, in execute >>>>>>>> self.error_handler.check_response(response) >>>>>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa >>>>>>>> ges/selenium/webdriver/remote/errorhandler.py", line 193, in >>>>>>>> check_response >>>>>>>> raise exception_class(message, screen, stacktrace) >>>>>>>> UnexpectedAlertPresentException: Alert Text: None >>>>>>>> Message: unexpected alert open: {Alert text : Are you sure you wish >>>>>>>> to close the pgAdmin 4 browser?} >>>>>>>> (Session info: chrome=58.0.3029.81) >>>>>>>> (Driver info: chromedriver=2.29.461585 >>>>>>>> (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac OS X >>>>>>>> 10.12.3 x86_64) >>>>>>>> >>>>>>> Sure. I will fix this. >>>>>>> >>>>>>>> >>>>>>>> Thanks! >>>>>>>> >>>>>>>> >>>>>>>> On Fri, Apr 28, 2017 at 10:19 AM, Surinder Kumar < >>>>>>>> [email protected]> wrote: >>>>>>>> >>>>>>>>> Hi Dave, >>>>>>>>> >>>>>>>>> Please find updated patch for RM case and a separate patch for >>>>>>>>> Feature tests. >>>>>>>>> >>>>>>>>> *Python:* >>>>>>>>> >>>>>>>>> - Added [default] label for cells with default values while >>>>>>>>> inserting a new row. >>>>>>>>> >>>>>>>>> - Introduced a FieldValidator function for cells that don't allow >>>>>>>>> null values. If user tries to insert null value, field with be highlighted >>>>>>>>> with red borders around. >>>>>>>>> >>>>>>>>> - >>>>>>>>> If a cell contains blank string('') and when we set it to null, >>>>>>>>> the change into the cell is not detected. It was because the comparison >>>>>>>>> for (defaultValue == null) return true if defaultValue is >>>>>>>>> undefined. Hence _.isNull(value) is used to fix this. >>>>>>>>> >>>>>>>>> *Feature Test cases:* >>>>>>>>> >>>>>>>>> - Introduced a new method create_table_with_query(server, >>>>>>>>> db_name, query) in test_utils.py which executes the given query >>>>>>>>> on connected server. >>>>>>>>> >>>>>>>>> - Added a new file test_data.json that has test data for test >>>>>>>>> cases. >>>>>>>>> >>>>>>>>> >>>>>>>>> On Fri, Apr 7, 2017 at 2:21 PM, Dave Page <[email protected]> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> Hi >>>>>>>>>> >>>>>>>>>> On Sat, Apr 1, 2017 at 12:45 PM, Surinder Kumar >>>>>>>>>> <[email protected]> wrote: >>>>>>>>>> > Hi >>>>>>>>>> > >>>>>>>>>> > Issues fixed: >>>>>>>>>> > >>>>>>>>>> > 1. If a column is defined with a default modifier, there is now >>>>>>>>>> way to >>>>>>>>>> > insert the row with those defaults. >>>>>>>>>> > The column will be left blank and it will take default value >>>>>>>>>> automatically. >>>>>>>>>> > >>>>>>>>>> > 2. If a column has a not-null constraint then an error is >>>>>>>>>> returned and the >>>>>>>>>> > row is not inserted. >>>>>>>>>> > The column will be left blank >>>>>>>>>> > >>>>>>>>>> > The default values for new added rows will be displayed on >>>>>>>>>> refresh/execute. >>>>>>>>>> > >>>>>>>>>> > Please find attached patch and review. >>>>>>>>>> >>>>>>>>>> This largely works as expected, but there is some weirdness. I >>>>>>>>>> have a >>>>>>>>>> test table that looks like this: >>>>>>>>>> >>>>>>>>>> CREATE TABLE public.defaults >>>>>>>>>> ( >>>>>>>>>> id bigint NOT NULL DEFAULT nextval('defaults_id_seq'::reg >>>>>>>>>> class), >>>>>>>>>> data_default_nulls text COLLATE pg_catalog."default" DEFAULT >>>>>>>>>> 'abc123'::text, >>>>>>>>>> data_default_no_nulls text COLLATE pg_catalog."default" NOT >>>>>>>>>> NULL >>>>>>>>>> DEFAULT 'def456'::text, >>>>>>>>>> data_nulls text COLLATE pg_catalog."default", >>>>>>>>>> data_no_nulls text COLLATE pg_catalog."default" NOT NULL, >>>>>>>>>> CONSTRAINT defaults_pkey PRIMARY KEY (id) >>>>>>>>>> ) >>>>>>>>>> >>>>>>>>>> Remember that the expected behaviour is: >>>>>>>>>> >>>>>>>>>> - Set a value to empty to update the column to null. >>>>>>>>>> - Set a value to '' to update the column to an empty string >>>>>>>>>> - Set a value to anything else to update the column to that value >>>>>>>>>> >>>>>>>>>> 1) In a row with values in each column, if I try to set the value >>>>>>>>>> of >>>>>>>>>> data_default_nulls to null, the query executed is: >>>>>>>>>> >>>>>>>>>> UPDATE public.defaults SET >>>>>>>>>> data_default_nulls = '' WHERE >>>>>>>>>> id = '2'; >>>>>>>>>> >>>>>>>>>> 2) If I do the same in the data_nulls column, the value is >>>>>>>>>> immediately >>>>>>>>>> shown as [null] and the query executed is: >>>>>>>>>> >>>>>>>>>> UPDATE public.defaults SET >>>>>>>>>> data_nulls = NULL WHERE >>>>>>>>>> id = '2'; >>>>>>>>>> >>>>>>>>>> 3) If I then edit the value in data_default_nulls, it shows the >>>>>>>>>> current value as ''. Removing the quotes (to set it to null) >>>>>>>>>> doesn't >>>>>>>>>> get detected as a change. >>>>>>>>>> >>>>>>>>> Taken care. >>>>>>>>> >>>>>>>>>> >>>>>>>>>> 4) When I manually executed "update defaults set >>>>>>>>>> data_default_nulls = >>>>>>>>>> null where id = 2" in a query tool window, I got: >>>>>>>>>> >>>>>>>>>> 2017-04-07 09:43:02,987: INFO werkzeug: 127.0.0.1 - - [07/Apr/2017 >>>>>>>>>> 09:43:02] "GET /sqleditor/columns/8745675 HTTP/1.1" 500 - >>>>>>>>>> Traceback (most recent call last): >>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>> line 2000, in __call__ >>>>>>>>>> return self.wsgi_app(environ, start_response) >>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>> line 1991, in wsgi_app >>>>>>>>>> response = self.make_response(self.handle_exception(e)) >>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>> line 1567, in handle_exception >>>>>>>>>> reraise(exc_type, exc_value, tb) >>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>> line 1988, in wsgi_app >>>>>>>>>> response = self.full_dispatch_request() >>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>> line 1641, in full_dispatch_request >>>>>>>>>> rv = self.handle_user_exception(e) >>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>> line 1544, in handle_user_exception >>>>>>>>>> reraise(exc_type, exc_value, tb) >>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>> line 1639, in full_dispatch_request >>>>>>>>>> rv = self.dispatch_request() >>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>> line 1625, in dispatch_request >>>>>>>>>> return self.view_functions[rule.endpoint](**req.view_args) >>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>> dmin4/lib/python2.7/site-packages/flask_login.py", >>>>>>>>>> line 792, in decorated_view >>>>>>>>>> return func(*args, **kwargs) >>>>>>>>>> File "/Users/dpage/git/pgadmin4/web >>>>>>>>>> /pgadmin/tools/sqleditor/__init__.py", >>>>>>>>>> line 452, in get_columns >>>>>>>>>> tid=command_obj.obj_id) >>>>>>>>>> AttributeError: 'QueryToolCommand' object has no attribute >>>>>>>>>> 'obj_id' >>>>>>>>>> >>>>>>>>> Fixed. >>>>>>>>> >>>>>>>>>> >>>>>>>>>> 5) When I run the query again in pgAdmin III, then refresh the >>>>>>>>>> data in >>>>>>>>>> pgAdmin 4, the data_default_nulls column is displayed without the >>>>>>>>>> [null] marker (despite having a null value, which I confirmed in >>>>>>>>>> pgAdmin 3). >>>>>>>>>> >>>>>>>>> Fixed. >>>>>>>>> >>>>>>>>>> >>>>>>>>>> I'm sure there are other combinations of issues here. Please fix >>>>>>>>>> and >>>>>>>>>> thoroughly re-test to ensure behaviour is consistent - and to >>>>>>>>>> avoid >>>>>>>>>> future issues, please add some appropriate feature tests to check >>>>>>>>>> nulls, defaults and empty strings are properly handled in view, >>>>>>>>>> insert >>>>>>>>>> and updates. Murtuza recently wrote some feature tests for the >>>>>>>>>> query >>>>>>>>>> tool - you should be able to use those as a starting point. >>>>>>>>>> >>>>>>>>> Added feature tests >>>>>>>>> >>>>>>>>>> >>>>>>>>>> Thanks. >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> Dave Page >>>>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>>>> Twitter: @pgsnake >>>>>>>>>> >>>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>>>> The Enterprise PostgreSQL Company >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> Dave Page >>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>> Twitter: @pgsnake >>>>>>>> >>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>> The Enterprise PostgreSQL Company >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>> >>>>> >>>>> -- >>>>> Dave Page >>>>> Blog: http://pgsnake.blogspot.com >>>>> Twitter: @pgsnake >>>>> >>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>> The Enterprise PostgreSQL Company >>>>> >>>> >>>> >>> >>> >>> -- >>> Dave Page >>> Blog: http://pgsnake.blogspot.com >>> Twitter: @pgsnake >>> >>> EnterpriseDB UK: http://www.enterprisedb.com >>> The Enterprise PostgreSQL Company >>> >> >> > > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-09 10:29 Dave Page <[email protected]> parent: Surinder Kumar <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Dave Page @ 2017-05-09 10:29 UTC (permalink / raw) To: Surinder Kumar <[email protected]>; +Cc: pgadmin-hackers On Tue, May 9, 2017 at 11:03 AM, Surinder Kumar < [email protected]> wrote: > Hi Dave, > > On Mon, May 8, 2017 at 4:37 PM, Dave Page <[email protected]> wrote: > >> >> >> On Mon, May 8, 2017 at 11:51 AM, Surinder Kumar < >> [email protected]> wrote: >> >>> Hi >>> >>> On Mon, May 8, 2017 at 3:51 PM, Dave Page <[email protected]> wrote: >>> >>>> Hi >>>> >>>> On Mon, May 8, 2017 at 11:13 AM, Surinder Kumar < >>>> [email protected]> wrote: >>>> >>>>> Hi Dave, >>>>> >>>>> On Mon, May 8, 2017 at 3:28 PM, Dave Page <[email protected]> wrote: >>>>> >>>>>> Hi >>>>>> >>>>>> On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar < >>>>>> [email protected]> wrote: >>>>>> >>>>>>> Hi Dave, >>>>>>> >>>>>>> The support to handle [null] and [default] values is added for >>>>>>> following formatters: >>>>>>> - JsonFormatter >>>>>>> - NumbersFormatter >>>>>>> - CheckmarkFormatter >>>>>>> - TextFormatter >>>>>>> >>>>>>> Previously when a new row is added, it was not validating each and >>>>>>> every cell for columns with attribute not_null = true. >>>>>>> Introduced a new function validateRow(...) which is called on >>>>>>> adding a new row, it validates each cell with column having >>>>>>> attribute(not_null=true). the corresponding cell will highlighted and save >>>>>>> button will be disabled if value is [null]. >>>>>>> >>>>>> >>>>>> I'm not sure that behaviour is right. What I now see (given the table >>>>>> below) is that: >>>>>> >>>>>> - If I click in the id field of a new row, it forces me to enter a >>>>>> value or hit escape. Why is it trying to force me? It's a serial field, so >>>>>> will get a value on save anyway. >>>>>> >>>>> Yes, It is because I am validating all cell values after it renders. >>>>> It will reverted back. >>>>> >>>>>> >>>>>> - If I do enter a value in the ID field, focus jumps over all the >>>>>> other fields with either a default or no 'not null' option to the >>>>>> data_no_nulls column. Focus should always go to the next field. >>>>>> >>>>>> I think this addition can just be removed - I'm pretty sure the >>>>>> previous behaviour would have been what we want, with the additional >>>>>> formatters fixed. >>>>>> >>>>> But If i remove this addition, the value for column like >>>>> 'data_no_nulls' will be set to '' (blank string), then on save its value >>>>> will be validated on the server side and whatever the error message is will >>>>> be returned back (the same behaviour as in pgAdmin3) >>>>> >>>> >>>> Which is fine I think. If you want to leave the validation there, >>>> that's also fine - but, it a) shouldn't require me to press Esc if I decide >>>> not to fill in that value yet, and b) shouldn't change the tab order of the >>>> fields. It's also broken of course, in that it was trying to force me to >>>> enter a value for a not-null field with a default. >>>> >>> Yes, I will check if we just highlight the field and don't force the >>> user to enter value. >>> This will enable user to edit any field using TAB key even if required >>> field is highlighted red. >>> Should the save button remains disable untill user enters any valid >>> value in 'data_no_nulls' column ? >>> >> >> I think that's fine, yes - as long as we get the validation right :-) >> > For highlighting the error field and enable user to navigate to other > cells using TAB key, > I spend some time looking and debugging into the code and found that it > requires lot of changes in slick.grid.js core functions as there are no > event listeners provided to listen and changes in core file is not > preferred. It may also break other functionalities as code is quite complex. > So, I think we should validate data on server side and display any error > messages on UI. > OK. > >>>> >>> >>>>>> >>>>>>> >>>>>>> Now I will add more feature test cases for remaining formatters. >>>>>>> Will send separate patch for feature test cases once completed. >>>>>>> >>>>>> >>>>>> Thanks. >>>>>> >>>>>> >>>>>>> >>>>>>> Please review updated patch. >>>>>>> >>>>>>> >>>>>>> On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar < >>>>>>> [email protected]> wrote: >>>>>>> >>>>>>>> Hi Dave, >>>>>>>> >>>>>>>> On Tue, May 2, 2017 at 5:21 PM, Dave Page <[email protected]> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> This is looking much better now :-). Couple of thoughts and a bug: >>>>>>>>> >>>>>>>>> - Only the TextFormatter seems to handle both [null] and [default] >>>>>>>>> values. Shouldn't all formatters do so (including Json and Checkmark)? >>>>>>>>> >>>>>>>> Yes, I will apply the same changes for other formatters too. >>>>>>>> >>>>>>>>> For example, "serial" columns currently get displayed as [null] >>>>>>>>> when left blank, but I would expect to see [default]. >>>>>>>>> >>>>>>>>> - I would suggest we put [null] and [default] in a lighter colour >>>>>>>>> - #999999. >>>>>>>>> >>>>>>>>> - With the feature test patch added, I seem to be consistently >>>>>>>>> getting the following failure (immediately after your new tests run): >>>>>>>>> >>>>>>>>> ============================================================ >>>>>>>>> ========== >>>>>>>>> ERROR: runTest (pgadmin.feature_tests.xss_che >>>>>>>>> cks_panels_and_query_tool_test.CheckForXssFeatureTest) >>>>>>>>> Test XSS check for panels and query tool >>>>>>>>> ------------------------------------------------------------ >>>>>>>>> ---------- >>>>>>>>> Traceback (most recent call last): >>>>>>>>> File "/Users/dpage/git/pgadmin4/web >>>>>>>>> /regression/feature_utils/base_feature_test.py", line 42, in setUp >>>>>>>>> self._screenshot() >>>>>>>>> File "/Users/dpage/git/pgadmin4/web >>>>>>>>> /regression/feature_utils/base_feature_test.py", line 92, in >>>>>>>>> _screenshot >>>>>>>>> python_version)) >>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>> line 802, in get_screenshot_as_file >>>>>>>>> png = self.get_screenshot_as_png() >>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>> line 821, in get_screenshot_as_png >>>>>>>>> return base64.b64decode(self.get_scre >>>>>>>>> enshot_as_base64().encode('ascii')) >>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>> line 831, in get_screenshot_as_base64 >>>>>>>>> return self.execute(Command.SCREENSHOT)['value'] >>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>> line 238, in execute >>>>>>>>> self.error_handler.check_response(response) >>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/errorhandler.py", >>>>>>>>> line 193, in check_response >>>>>>>>> raise exception_class(message, screen, stacktrace) >>>>>>>>> UnexpectedAlertPresentException: Alert Text: None >>>>>>>>> Message: unexpected alert open: {Alert text : Are you sure you >>>>>>>>> wish to close the pgAdmin 4 browser?} >>>>>>>>> (Session info: chrome=58.0.3029.81) >>>>>>>>> (Driver info: chromedriver=2.29.461585 >>>>>>>>> (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac OS X >>>>>>>>> 10.12.3 x86_64) >>>>>>>>> >>>>>>>>> >>>>>>>>> ============================================================ >>>>>>>>> ========== >>>>>>>>> ERROR: runTest (pgadmin.feature_tests.xss_che >>>>>>>>> cks_pgadmin_debugger_test.CheckDebuggerForXssFeatureTest) >>>>>>>>> Test table DDL generation >>>>>>>>> ------------------------------------------------------------ >>>>>>>>> ---------- >>>>>>>>> Traceback (most recent call last): >>>>>>>>> File "/Users/dpage/git/pgadmin4/web >>>>>>>>> /regression/feature_utils/base_feature_test.py", line 42, in setUp >>>>>>>>> self._screenshot() >>>>>>>>> File "/Users/dpage/git/pgadmin4/web >>>>>>>>> /regression/feature_utils/base_feature_test.py", line 92, in >>>>>>>>> _screenshot >>>>>>>>> python_version)) >>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>> line 802, in get_screenshot_as_file >>>>>>>>> png = self.get_screenshot_as_png() >>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>> line 821, in get_screenshot_as_png >>>>>>>>> return base64.b64decode(self.get_scre >>>>>>>>> enshot_as_base64().encode('ascii')) >>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>> line 831, in get_screenshot_as_base64 >>>>>>>>> return self.execute(Command.SCREENSHOT)['value'] >>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>> line 238, in execute >>>>>>>>> self.error_handler.check_response(response) >>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/errorhandler.py", >>>>>>>>> line 193, in check_response >>>>>>>>> raise exception_class(message, screen, stacktrace) >>>>>>>>> UnexpectedAlertPresentException: Alert Text: None >>>>>>>>> Message: unexpected alert open: {Alert text : Are you sure you >>>>>>>>> wish to close the pgAdmin 4 browser?} >>>>>>>>> (Session info: chrome=58.0.3029.81) >>>>>>>>> (Driver info: chromedriver=2.29.461585 >>>>>>>>> (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac OS X >>>>>>>>> 10.12.3 x86_64) >>>>>>>>> >>>>>>>> Sure. I will fix this. >>>>>>>> >>>>>>>>> >>>>>>>>> Thanks! >>>>>>>>> >>>>>>>>> >>>>>>>>> On Fri, Apr 28, 2017 at 10:19 AM, Surinder Kumar < >>>>>>>>> [email protected]> wrote: >>>>>>>>> >>>>>>>>>> Hi Dave, >>>>>>>>>> >>>>>>>>>> Please find updated patch for RM case and a separate patch for >>>>>>>>>> Feature tests. >>>>>>>>>> >>>>>>>>>> *Python:* >>>>>>>>>> >>>>>>>>>> - Added [default] label for cells with default values while >>>>>>>>>> inserting a new row. >>>>>>>>>> >>>>>>>>>> - Introduced a FieldValidator function for cells that don't allow >>>>>>>>>> null values. If user tries to insert null value, field with be highlighted >>>>>>>>>> with red borders around. >>>>>>>>>> >>>>>>>>>> - >>>>>>>>>> If a cell contains blank string('') and when we set it to null, >>>>>>>>>> the change into the cell is not detected. It was because the comparison >>>>>>>>>> for (defaultValue == null) return true if defaultValue is >>>>>>>>>> undefined. Hence _.isNull(value) is used to fix this. >>>>>>>>>> >>>>>>>>>> *Feature Test cases:* >>>>>>>>>> >>>>>>>>>> - Introduced a new method create_table_with_query(server, >>>>>>>>>> db_name, query) in test_utils.py which executes the given query >>>>>>>>>> on connected server. >>>>>>>>>> >>>>>>>>>> - Added a new file test_data.json that has test data for test >>>>>>>>>> cases. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Fri, Apr 7, 2017 at 2:21 PM, Dave Page <[email protected]> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> Hi >>>>>>>>>>> >>>>>>>>>>> On Sat, Apr 1, 2017 at 12:45 PM, Surinder Kumar >>>>>>>>>>> <[email protected]> wrote: >>>>>>>>>>> > Hi >>>>>>>>>>> > >>>>>>>>>>> > Issues fixed: >>>>>>>>>>> > >>>>>>>>>>> > 1. If a column is defined with a default modifier, there is >>>>>>>>>>> now way to >>>>>>>>>>> > insert the row with those defaults. >>>>>>>>>>> > The column will be left blank and it will take default value >>>>>>>>>>> automatically. >>>>>>>>>>> > >>>>>>>>>>> > 2. If a column has a not-null constraint then an error is >>>>>>>>>>> returned and the >>>>>>>>>>> > row is not inserted. >>>>>>>>>>> > The column will be left blank >>>>>>>>>>> > >>>>>>>>>>> > The default values for new added rows will be displayed on >>>>>>>>>>> refresh/execute. >>>>>>>>>>> > >>>>>>>>>>> > Please find attached patch and review. >>>>>>>>>>> >>>>>>>>>>> This largely works as expected, but there is some weirdness. I >>>>>>>>>>> have a >>>>>>>>>>> test table that looks like this: >>>>>>>>>>> >>>>>>>>>>> CREATE TABLE public.defaults >>>>>>>>>>> ( >>>>>>>>>>> id bigint NOT NULL DEFAULT nextval('defaults_id_seq'::reg >>>>>>>>>>> class), >>>>>>>>>>> data_default_nulls text COLLATE pg_catalog."default" DEFAULT >>>>>>>>>>> 'abc123'::text, >>>>>>>>>>> data_default_no_nulls text COLLATE pg_catalog."default" NOT >>>>>>>>>>> NULL >>>>>>>>>>> DEFAULT 'def456'::text, >>>>>>>>>>> data_nulls text COLLATE pg_catalog."default", >>>>>>>>>>> data_no_nulls text COLLATE pg_catalog."default" NOT NULL, >>>>>>>>>>> CONSTRAINT defaults_pkey PRIMARY KEY (id) >>>>>>>>>>> ) >>>>>>>>>>> >>>>>>>>>>> Remember that the expected behaviour is: >>>>>>>>>>> >>>>>>>>>>> - Set a value to empty to update the column to null. >>>>>>>>>>> - Set a value to '' to update the column to an empty string >>>>>>>>>>> - Set a value to anything else to update the column to that value >>>>>>>>>>> >>>>>>>>>>> 1) In a row with values in each column, if I try to set the >>>>>>>>>>> value of >>>>>>>>>>> data_default_nulls to null, the query executed is: >>>>>>>>>>> >>>>>>>>>>> UPDATE public.defaults SET >>>>>>>>>>> data_default_nulls = '' WHERE >>>>>>>>>>> id = '2'; >>>>>>>>>>> >>>>>>>>>>> 2) If I do the same in the data_nulls column, the value is >>>>>>>>>>> immediately >>>>>>>>>>> shown as [null] and the query executed is: >>>>>>>>>>> >>>>>>>>>>> UPDATE public.defaults SET >>>>>>>>>>> data_nulls = NULL WHERE >>>>>>>>>>> id = '2'; >>>>>>>>>>> >>>>>>>>>>> 3) If I then edit the value in data_default_nulls, it shows the >>>>>>>>>>> current value as ''. Removing the quotes (to set it to null) >>>>>>>>>>> doesn't >>>>>>>>>>> get detected as a change. >>>>>>>>>>> >>>>>>>>>> Taken care. >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> 4) When I manually executed "update defaults set >>>>>>>>>>> data_default_nulls = >>>>>>>>>>> null where id = 2" in a query tool window, I got: >>>>>>>>>>> >>>>>>>>>>> 2017-04-07 09:43:02,987: INFO werkzeug: 127.0.0.1 - - >>>>>>>>>>> [07/Apr/2017 >>>>>>>>>>> 09:43:02] "GET /sqleditor/columns/8745675 HTTP/1.1" 500 - >>>>>>>>>>> Traceback (most recent call last): >>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>> line 2000, in __call__ >>>>>>>>>>> return self.wsgi_app(environ, start_response) >>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>> line 1991, in wsgi_app >>>>>>>>>>> response = self.make_response(self.handle_exception(e)) >>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>> line 1567, in handle_exception >>>>>>>>>>> reraise(exc_type, exc_value, tb) >>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>> line 1988, in wsgi_app >>>>>>>>>>> response = self.full_dispatch_request() >>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>> line 1641, in full_dispatch_request >>>>>>>>>>> rv = self.handle_user_exception(e) >>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>> line 1544, in handle_user_exception >>>>>>>>>>> reraise(exc_type, exc_value, tb) >>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>> line 1639, in full_dispatch_request >>>>>>>>>>> rv = self.dispatch_request() >>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>> line 1625, in dispatch_request >>>>>>>>>>> return self.view_functions[rule.endpoint](**req.view_args) >>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask_login.py", >>>>>>>>>>> line 792, in decorated_view >>>>>>>>>>> return func(*args, **kwargs) >>>>>>>>>>> File "/Users/dpage/git/pgadmin4/web >>>>>>>>>>> /pgadmin/tools/sqleditor/__init__.py", >>>>>>>>>>> line 452, in get_columns >>>>>>>>>>> tid=command_obj.obj_id) >>>>>>>>>>> AttributeError: 'QueryToolCommand' object has no attribute >>>>>>>>>>> 'obj_id' >>>>>>>>>>> >>>>>>>>>> Fixed. >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> 5) When I run the query again in pgAdmin III, then refresh the >>>>>>>>>>> data in >>>>>>>>>>> pgAdmin 4, the data_default_nulls column is displayed without the >>>>>>>>>>> [null] marker (despite having a null value, which I confirmed in >>>>>>>>>>> pgAdmin 3). >>>>>>>>>>> >>>>>>>>>> Fixed. >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> I'm sure there are other combinations of issues here. Please fix >>>>>>>>>>> and >>>>>>>>>>> thoroughly re-test to ensure behaviour is consistent - and to >>>>>>>>>>> avoid >>>>>>>>>>> future issues, please add some appropriate feature tests to check >>>>>>>>>>> nulls, defaults and empty strings are properly handled in view, >>>>>>>>>>> insert >>>>>>>>>>> and updates. Murtuza recently wrote some feature tests for the >>>>>>>>>>> query >>>>>>>>>>> tool - you should be able to use those as a starting point. >>>>>>>>>>> >>>>>>>>>> Added feature tests >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Thanks. >>>>>>>>>>> >>>>>>>>>>> -- >>>>>>>>>>> Dave Page >>>>>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>>>>> Twitter: @pgsnake >>>>>>>>>>> >>>>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>>>>> The Enterprise PostgreSQL Company >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Dave Page >>>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>>> Twitter: @pgsnake >>>>>>>>> >>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>>> The Enterprise PostgreSQL Company >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Dave Page >>>>>> Blog: http://pgsnake.blogspot.com >>>>>> Twitter: @pgsnake >>>>>> >>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>> The Enterprise PostgreSQL Company >>>>>> >>>>> >>>>> >>>> >>>> >>>> -- >>>> Dave Page >>>> Blog: http://pgsnake.blogspot.com >>>> Twitter: @pgsnake >>>> >>>> EnterpriseDB UK: http://www.enterprisedb.com >>>> The Enterprise PostgreSQL Company >>>> >>> >>> >> >> >> -- >> Dave Page >> Blog: http://pgsnake.blogspot.com >> Twitter: @pgsnake >> >> EnterpriseDB UK: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-10 08:36 Dave Page <[email protected]> parent: Dave Page <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Dave Page @ 2017-05-10 08:36 UTC (permalink / raw) To: Surinder Kumar <[email protected]>; +Cc: pgadmin-hackers Any chance we can get this wrapped up today Surinder? On Tue, May 9, 2017 at 11:29 AM, Dave Page <[email protected]> wrote: > > > On Tue, May 9, 2017 at 11:03 AM, Surinder Kumar < > [email protected]> wrote: > >> Hi Dave, >> >> On Mon, May 8, 2017 at 4:37 PM, Dave Page <[email protected]> wrote: >> >>> >>> >>> On Mon, May 8, 2017 at 11:51 AM, Surinder Kumar < >>> [email protected]> wrote: >>> >>>> Hi >>>> >>>> On Mon, May 8, 2017 at 3:51 PM, Dave Page <[email protected]> wrote: >>>> >>>>> Hi >>>>> >>>>> On Mon, May 8, 2017 at 11:13 AM, Surinder Kumar < >>>>> [email protected]> wrote: >>>>> >>>>>> Hi Dave, >>>>>> >>>>>> On Mon, May 8, 2017 at 3:28 PM, Dave Page <[email protected]> wrote: >>>>>> >>>>>>> Hi >>>>>>> >>>>>>> On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar < >>>>>>> [email protected]> wrote: >>>>>>> >>>>>>>> Hi Dave, >>>>>>>> >>>>>>>> The support to handle [null] and [default] values is added for >>>>>>>> following formatters: >>>>>>>> - JsonFormatter >>>>>>>> - NumbersFormatter >>>>>>>> - CheckmarkFormatter >>>>>>>> - TextFormatter >>>>>>>> >>>>>>>> Previously when a new row is added, it was not validating each and >>>>>>>> every cell for columns with attribute not_null = true. >>>>>>>> Introduced a new function validateRow(...) which is called on >>>>>>>> adding a new row, it validates each cell with column having >>>>>>>> attribute(not_null=true). the corresponding cell will highlighted and save >>>>>>>> button will be disabled if value is [null]. >>>>>>>> >>>>>>> >>>>>>> I'm not sure that behaviour is right. What I now see (given the >>>>>>> table below) is that: >>>>>>> >>>>>>> - If I click in the id field of a new row, it forces me to enter a >>>>>>> value or hit escape. Why is it trying to force me? It's a serial field, so >>>>>>> will get a value on save anyway. >>>>>>> >>>>>> Yes, It is because I am validating all cell values after it renders. >>>>>> It will reverted back. >>>>>> >>>>>>> >>>>>>> - If I do enter a value in the ID field, focus jumps over all the >>>>>>> other fields with either a default or no 'not null' option to the >>>>>>> data_no_nulls column. Focus should always go to the next field. >>>>>>> >>>>>>> I think this addition can just be removed - I'm pretty sure the >>>>>>> previous behaviour would have been what we want, with the additional >>>>>>> formatters fixed. >>>>>>> >>>>>> But If i remove this addition, the value for column like >>>>>> 'data_no_nulls' will be set to '' (blank string), then on save its value >>>>>> will be validated on the server side and whatever the error message is will >>>>>> be returned back (the same behaviour as in pgAdmin3) >>>>>> >>>>> >>>>> Which is fine I think. If you want to leave the validation there, >>>>> that's also fine - but, it a) shouldn't require me to press Esc if I decide >>>>> not to fill in that value yet, and b) shouldn't change the tab order of the >>>>> fields. It's also broken of course, in that it was trying to force me to >>>>> enter a value for a not-null field with a default. >>>>> >>>> Yes, I will check if we just highlight the field and don't force the >>>> user to enter value. >>>> This will enable user to edit any field using TAB key even if required >>>> field is highlighted red. >>>> Should the save button remains disable untill user enters any valid >>>> value in 'data_no_nulls' column ? >>>> >>> >>> I think that's fine, yes - as long as we get the validation right :-) >>> >> For highlighting the error field and enable user to navigate to other >> cells using TAB key, >> I spend some time looking and debugging into the code and found that it >> requires lot of changes in slick.grid.js core functions as there are no >> event listeners provided to listen and changes in core file is not >> preferred. It may also break other functionalities as code is quite complex. >> So, I think we should validate data on server side and display any error >> messages on UI. >> > > OK. > > >> >>>>> >>>> >>>>>>> >>>>>>>> >>>>>>>> Now I will add more feature test cases for remaining formatters. >>>>>>>> Will send separate patch for feature test cases once completed. >>>>>>>> >>>>>>> >>>>>>> Thanks. >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> Please review updated patch. >>>>>>>> >>>>>>>> >>>>>>>> On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar < >>>>>>>> [email protected]> wrote: >>>>>>>> >>>>>>>>> Hi Dave, >>>>>>>>> >>>>>>>>> On Tue, May 2, 2017 at 5:21 PM, Dave Page <[email protected]> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> This is looking much better now :-). Couple of thoughts and a bug: >>>>>>>>>> >>>>>>>>>> - Only the TextFormatter seems to handle both [null] and >>>>>>>>>> [default] values. Shouldn't all formatters do so (including Json and >>>>>>>>>> Checkmark)? >>>>>>>>>> >>>>>>>>> Yes, I will apply the same changes for other formatters too. >>>>>>>>> >>>>>>>>>> For example, "serial" columns currently get displayed as [null] >>>>>>>>>> when left blank, but I would expect to see [default]. >>>>>>>>>> >>>>>>>>>> - I would suggest we put [null] and [default] in a lighter colour >>>>>>>>>> - #999999. >>>>>>>>>> >>>>>>>>>> - With the feature test patch added, I seem to be consistently >>>>>>>>>> getting the following failure (immediately after your new tests run): >>>>>>>>>> >>>>>>>>>> ============================================================ >>>>>>>>>> ========== >>>>>>>>>> ERROR: runTest (pgadmin.feature_tests.xss_che >>>>>>>>>> cks_panels_and_query_tool_test.CheckForXssFeatureTest) >>>>>>>>>> Test XSS check for panels and query tool >>>>>>>>>> ------------------------------------------------------------ >>>>>>>>>> ---------- >>>>>>>>>> Traceback (most recent call last): >>>>>>>>>> File "/Users/dpage/git/pgadmin4/web >>>>>>>>>> /regression/feature_utils/base_feature_test.py", line 42, in >>>>>>>>>> setUp >>>>>>>>>> self._screenshot() >>>>>>>>>> File "/Users/dpage/git/pgadmin4/web >>>>>>>>>> /regression/feature_utils/base_feature_test.py", line 92, in >>>>>>>>>> _screenshot >>>>>>>>>> python_version)) >>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>>> line 802, in get_screenshot_as_file >>>>>>>>>> png = self.get_screenshot_as_png() >>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>>> line 821, in get_screenshot_as_png >>>>>>>>>> return base64.b64decode(self.get_scre >>>>>>>>>> enshot_as_base64().encode('ascii')) >>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>>> line 831, in get_screenshot_as_base64 >>>>>>>>>> return self.execute(Command.SCREENSHOT)['value'] >>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>>> line 238, in execute >>>>>>>>>> self.error_handler.check_response(response) >>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/errorhandler.py", >>>>>>>>>> line 193, in check_response >>>>>>>>>> raise exception_class(message, screen, stacktrace) >>>>>>>>>> UnexpectedAlertPresentException: Alert Text: None >>>>>>>>>> Message: unexpected alert open: {Alert text : Are you sure you >>>>>>>>>> wish to close the pgAdmin 4 browser?} >>>>>>>>>> (Session info: chrome=58.0.3029.81) >>>>>>>>>> (Driver info: chromedriver=2.29.461585 >>>>>>>>>> (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac OS X >>>>>>>>>> 10.12.3 x86_64) >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> ============================================================ >>>>>>>>>> ========== >>>>>>>>>> ERROR: runTest (pgadmin.feature_tests.xss_che >>>>>>>>>> cks_pgadmin_debugger_test.CheckDebuggerForXssFeatureTest) >>>>>>>>>> Test table DDL generation >>>>>>>>>> ------------------------------------------------------------ >>>>>>>>>> ---------- >>>>>>>>>> Traceback (most recent call last): >>>>>>>>>> File "/Users/dpage/git/pgadmin4/web >>>>>>>>>> /regression/feature_utils/base_feature_test.py", line 42, in >>>>>>>>>> setUp >>>>>>>>>> self._screenshot() >>>>>>>>>> File "/Users/dpage/git/pgadmin4/web >>>>>>>>>> /regression/feature_utils/base_feature_test.py", line 92, in >>>>>>>>>> _screenshot >>>>>>>>>> python_version)) >>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>>> line 802, in get_screenshot_as_file >>>>>>>>>> png = self.get_screenshot_as_png() >>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>>> line 821, in get_screenshot_as_png >>>>>>>>>> return base64.b64decode(self.get_scre >>>>>>>>>> enshot_as_base64().encode('ascii')) >>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>>> line 831, in get_screenshot_as_base64 >>>>>>>>>> return self.execute(Command.SCREENSHOT)['value'] >>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>>> line 238, in execute >>>>>>>>>> self.error_handler.check_response(response) >>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/errorhandler.py", >>>>>>>>>> line 193, in check_response >>>>>>>>>> raise exception_class(message, screen, stacktrace) >>>>>>>>>> UnexpectedAlertPresentException: Alert Text: None >>>>>>>>>> Message: unexpected alert open: {Alert text : Are you sure you >>>>>>>>>> wish to close the pgAdmin 4 browser?} >>>>>>>>>> (Session info: chrome=58.0.3029.81) >>>>>>>>>> (Driver info: chromedriver=2.29.461585 >>>>>>>>>> (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac OS X >>>>>>>>>> 10.12.3 x86_64) >>>>>>>>>> >>>>>>>>> Sure. I will fix this. >>>>>>>>> >>>>>>>>>> >>>>>>>>>> Thanks! >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Fri, Apr 28, 2017 at 10:19 AM, Surinder Kumar < >>>>>>>>>> [email protected]> wrote: >>>>>>>>>> >>>>>>>>>>> Hi Dave, >>>>>>>>>>> >>>>>>>>>>> Please find updated patch for RM case and a separate patch for >>>>>>>>>>> Feature tests. >>>>>>>>>>> >>>>>>>>>>> *Python:* >>>>>>>>>>> >>>>>>>>>>> - Added [default] label for cells with default values while >>>>>>>>>>> inserting a new row. >>>>>>>>>>> >>>>>>>>>>> - Introduced a FieldValidator function for cells that don't >>>>>>>>>>> allow null values. If user tries to insert null value, field with be >>>>>>>>>>> highlighted with red borders around. >>>>>>>>>>> >>>>>>>>>>> - >>>>>>>>>>> If a cell contains blank string('') and when we set it to null, >>>>>>>>>>> the change into the cell is not detected. It was because the comparison >>>>>>>>>>> for (defaultValue == null) return true if defaultValue is >>>>>>>>>>> undefined. Hence _.isNull(value) is used to fix this. >>>>>>>>>>> >>>>>>>>>>> *Feature Test cases:* >>>>>>>>>>> >>>>>>>>>>> - Introduced a new method create_table_with_query(server, >>>>>>>>>>> db_name, query) in test_utils.py which executes the given >>>>>>>>>>> query on connected server. >>>>>>>>>>> >>>>>>>>>>> - Added a new file test_data.json that has test data for test >>>>>>>>>>> cases. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Fri, Apr 7, 2017 at 2:21 PM, Dave Page <[email protected]> >>>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>>> Hi >>>>>>>>>>>> >>>>>>>>>>>> On Sat, Apr 1, 2017 at 12:45 PM, Surinder Kumar >>>>>>>>>>>> <[email protected]> wrote: >>>>>>>>>>>> > Hi >>>>>>>>>>>> > >>>>>>>>>>>> > Issues fixed: >>>>>>>>>>>> > >>>>>>>>>>>> > 1. If a column is defined with a default modifier, there is >>>>>>>>>>>> now way to >>>>>>>>>>>> > insert the row with those defaults. >>>>>>>>>>>> > The column will be left blank and it will take default value >>>>>>>>>>>> automatically. >>>>>>>>>>>> > >>>>>>>>>>>> > 2. If a column has a not-null constraint then an error is >>>>>>>>>>>> returned and the >>>>>>>>>>>> > row is not inserted. >>>>>>>>>>>> > The column will be left blank >>>>>>>>>>>> > >>>>>>>>>>>> > The default values for new added rows will be displayed on >>>>>>>>>>>> refresh/execute. >>>>>>>>>>>> > >>>>>>>>>>>> > Please find attached patch and review. >>>>>>>>>>>> >>>>>>>>>>>> This largely works as expected, but there is some weirdness. I >>>>>>>>>>>> have a >>>>>>>>>>>> test table that looks like this: >>>>>>>>>>>> >>>>>>>>>>>> CREATE TABLE public.defaults >>>>>>>>>>>> ( >>>>>>>>>>>> id bigint NOT NULL DEFAULT nextval('defaults_id_seq'::reg >>>>>>>>>>>> class), >>>>>>>>>>>> data_default_nulls text COLLATE pg_catalog."default" >>>>>>>>>>>> DEFAULT 'abc123'::text, >>>>>>>>>>>> data_default_no_nulls text COLLATE pg_catalog."default" NOT >>>>>>>>>>>> NULL >>>>>>>>>>>> DEFAULT 'def456'::text, >>>>>>>>>>>> data_nulls text COLLATE pg_catalog."default", >>>>>>>>>>>> data_no_nulls text COLLATE pg_catalog."default" NOT NULL, >>>>>>>>>>>> CONSTRAINT defaults_pkey PRIMARY KEY (id) >>>>>>>>>>>> ) >>>>>>>>>>>> >>>>>>>>>>>> Remember that the expected behaviour is: >>>>>>>>>>>> >>>>>>>>>>>> - Set a value to empty to update the column to null. >>>>>>>>>>>> - Set a value to '' to update the column to an empty string >>>>>>>>>>>> - Set a value to anything else to update the column to that >>>>>>>>>>>> value >>>>>>>>>>>> >>>>>>>>>>>> 1) In a row with values in each column, if I try to set the >>>>>>>>>>>> value of >>>>>>>>>>>> data_default_nulls to null, the query executed is: >>>>>>>>>>>> >>>>>>>>>>>> UPDATE public.defaults SET >>>>>>>>>>>> data_default_nulls = '' WHERE >>>>>>>>>>>> id = '2'; >>>>>>>>>>>> >>>>>>>>>>>> 2) If I do the same in the data_nulls column, the value is >>>>>>>>>>>> immediately >>>>>>>>>>>> shown as [null] and the query executed is: >>>>>>>>>>>> >>>>>>>>>>>> UPDATE public.defaults SET >>>>>>>>>>>> data_nulls = NULL WHERE >>>>>>>>>>>> id = '2'; >>>>>>>>>>>> >>>>>>>>>>>> 3) If I then edit the value in data_default_nulls, it shows the >>>>>>>>>>>> current value as ''. Removing the quotes (to set it to null) >>>>>>>>>>>> doesn't >>>>>>>>>>>> get detected as a change. >>>>>>>>>>>> >>>>>>>>>>> Taken care. >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> 4) When I manually executed "update defaults set >>>>>>>>>>>> data_default_nulls = >>>>>>>>>>>> null where id = 2" in a query tool window, I got: >>>>>>>>>>>> >>>>>>>>>>>> 2017-04-07 09:43:02,987: INFO werkzeug: 127.0.0.1 - - >>>>>>>>>>>> [07/Apr/2017 >>>>>>>>>>>> 09:43:02] "GET /sqleditor/columns/8745675 HTTP/1.1" 500 - >>>>>>>>>>>> Traceback (most recent call last): >>>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>>> line 2000, in __call__ >>>>>>>>>>>> return self.wsgi_app(environ, start_response) >>>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>>> line 1991, in wsgi_app >>>>>>>>>>>> response = self.make_response(self.handle_exception(e)) >>>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>>> line 1567, in handle_exception >>>>>>>>>>>> reraise(exc_type, exc_value, tb) >>>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>>> line 1988, in wsgi_app >>>>>>>>>>>> response = self.full_dispatch_request() >>>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>>> line 1641, in full_dispatch_request >>>>>>>>>>>> rv = self.handle_user_exception(e) >>>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>>> line 1544, in handle_user_exception >>>>>>>>>>>> reraise(exc_type, exc_value, tb) >>>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>>> line 1639, in full_dispatch_request >>>>>>>>>>>> rv = self.dispatch_request() >>>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>>> line 1625, in dispatch_request >>>>>>>>>>>> return self.view_functions[rule.endpoint](**req.view_args) >>>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask_login.py", >>>>>>>>>>>> line 792, in decorated_view >>>>>>>>>>>> return func(*args, **kwargs) >>>>>>>>>>>> File "/Users/dpage/git/pgadmin4/web >>>>>>>>>>>> /pgadmin/tools/sqleditor/__init__.py", >>>>>>>>>>>> line 452, in get_columns >>>>>>>>>>>> tid=command_obj.obj_id) >>>>>>>>>>>> AttributeError: 'QueryToolCommand' object has no attribute >>>>>>>>>>>> 'obj_id' >>>>>>>>>>>> >>>>>>>>>>> Fixed. >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> 5) When I run the query again in pgAdmin III, then refresh the >>>>>>>>>>>> data in >>>>>>>>>>>> pgAdmin 4, the data_default_nulls column is displayed without >>>>>>>>>>>> the >>>>>>>>>>>> [null] marker (despite having a null value, which I confirmed in >>>>>>>>>>>> pgAdmin 3). >>>>>>>>>>>> >>>>>>>>>>> Fixed. >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> I'm sure there are other combinations of issues here. Please >>>>>>>>>>>> fix and >>>>>>>>>>>> thoroughly re-test to ensure behaviour is consistent - and to >>>>>>>>>>>> avoid >>>>>>>>>>>> future issues, please add some appropriate feature tests to >>>>>>>>>>>> check >>>>>>>>>>>> nulls, defaults and empty strings are properly handled in view, >>>>>>>>>>>> insert >>>>>>>>>>>> and updates. Murtuza recently wrote some feature tests for the >>>>>>>>>>>> query >>>>>>>>>>>> tool - you should be able to use those as a starting point. >>>>>>>>>>>> >>>>>>>>>>> Added feature tests >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Thanks. >>>>>>>>>>>> >>>>>>>>>>>> -- >>>>>>>>>>>> Dave Page >>>>>>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>>>>>> Twitter: @pgsnake >>>>>>>>>>>> >>>>>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>>>>>> The Enterprise PostgreSQL Company >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> Dave Page >>>>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>>>> Twitter: @pgsnake >>>>>>>>>> >>>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>>>> The Enterprise PostgreSQL Company >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Dave Page >>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>> Twitter: @pgsnake >>>>>>> >>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>> The Enterprise PostgreSQL Company >>>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>>> -- >>>>> Dave Page >>>>> Blog: http://pgsnake.blogspot.com >>>>> Twitter: @pgsnake >>>>> >>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>> The Enterprise PostgreSQL Company >>>>> >>>> >>>> >>> >>> >>> -- >>> Dave Page >>> Blog: http://pgsnake.blogspot.com >>> Twitter: @pgsnake >>> >>> EnterpriseDB UK: http://www.enterprisedb.com >>> The Enterprise PostgreSQL Company >>> >> >> > > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-10 08:39 Surinder Kumar <[email protected]> parent: Dave Page <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Surinder Kumar @ 2017-05-10 08:39 UTC (permalink / raw) To: Dave Page <[email protected]>; +Cc: pgadmin-hackers Hi Dave, On Wed, May 10, 2017 at 2:06 PM, Dave Page <[email protected]> wrote: > Any chance we can get this wrapped up today Surinder? > I have fixed RM case, I am currently writing its feature test cases which is taking some time. Should I send patch for RM case only? I will try to complete test cases by today eod. > > On Tue, May 9, 2017 at 11:29 AM, Dave Page <[email protected]> wrote: > >> >> >> On Tue, May 9, 2017 at 11:03 AM, Surinder Kumar < >> [email protected]> wrote: >> >>> Hi Dave, >>> >>> On Mon, May 8, 2017 at 4:37 PM, Dave Page <[email protected]> wrote: >>> >>>> >>>> >>>> On Mon, May 8, 2017 at 11:51 AM, Surinder Kumar < >>>> [email protected]> wrote: >>>> >>>>> Hi >>>>> >>>>> On Mon, May 8, 2017 at 3:51 PM, Dave Page <[email protected]> wrote: >>>>> >>>>>> Hi >>>>>> >>>>>> On Mon, May 8, 2017 at 11:13 AM, Surinder Kumar < >>>>>> [email protected]> wrote: >>>>>> >>>>>>> Hi Dave, >>>>>>> >>>>>>> On Mon, May 8, 2017 at 3:28 PM, Dave Page <[email protected]> wrote: >>>>>>> >>>>>>>> Hi >>>>>>>> >>>>>>>> On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar < >>>>>>>> [email protected]> wrote: >>>>>>>> >>>>>>>>> Hi Dave, >>>>>>>>> >>>>>>>>> The support to handle [null] and [default] values is added for >>>>>>>>> following formatters: >>>>>>>>> - JsonFormatter >>>>>>>>> - NumbersFormatter >>>>>>>>> - CheckmarkFormatter >>>>>>>>> - TextFormatter >>>>>>>>> >>>>>>>>> Previously when a new row is added, it was not validating each and >>>>>>>>> every cell for columns with attribute not_null = true. >>>>>>>>> Introduced a new function validateRow(...) which is called on >>>>>>>>> adding a new row, it validates each cell with column having >>>>>>>>> attribute(not_null=true). the corresponding cell will highlighted and save >>>>>>>>> button will be disabled if value is [null]. >>>>>>>>> >>>>>>>> >>>>>>>> I'm not sure that behaviour is right. What I now see (given the >>>>>>>> table below) is that: >>>>>>>> >>>>>>>> - If I click in the id field of a new row, it forces me to enter a >>>>>>>> value or hit escape. Why is it trying to force me? It's a serial field, so >>>>>>>> will get a value on save anyway. >>>>>>>> >>>>>>> Yes, It is because I am validating all cell values after it >>>>>>> renders. It will reverted back. >>>>>>> >>>>>>>> >>>>>>>> - If I do enter a value in the ID field, focus jumps over all the >>>>>>>> other fields with either a default or no 'not null' option to the >>>>>>>> data_no_nulls column. Focus should always go to the next field. >>>>>>>> >>>>>>>> I think this addition can just be removed - I'm pretty sure the >>>>>>>> previous behaviour would have been what we want, with the additional >>>>>>>> formatters fixed. >>>>>>>> >>>>>>> But If i remove this addition, the value for column like >>>>>>> 'data_no_nulls' will be set to '' (blank string), then on save its value >>>>>>> will be validated on the server side and whatever the error message is will >>>>>>> be returned back (the same behaviour as in pgAdmin3) >>>>>>> >>>>>> >>>>>> Which is fine I think. If you want to leave the validation there, >>>>>> that's also fine - but, it a) shouldn't require me to press Esc if I decide >>>>>> not to fill in that value yet, and b) shouldn't change the tab order of the >>>>>> fields. It's also broken of course, in that it was trying to force me to >>>>>> enter a value for a not-null field with a default. >>>>>> >>>>> Yes, I will check if we just highlight the field and don't force the >>>>> user to enter value. >>>>> This will enable user to edit any field using TAB key even if required >>>>> field is highlighted red. >>>>> Should the save button remains disable untill user enters any valid >>>>> value in 'data_no_nulls' column ? >>>>> >>>> >>>> I think that's fine, yes - as long as we get the validation right :-) >>>> >>> For highlighting the error field and enable user to navigate to other >>> cells using TAB key, >>> I spend some time looking and debugging into the code and found that it >>> requires lot of changes in slick.grid.js core functions as there are no >>> event listeners provided to listen and changes in core file is not >>> preferred. It may also break other functionalities as code is quite complex. >>> So, I think we should validate data on server side and display any error >>> messages on UI. >>> >> >> OK. >> >> >>> >>>>>> >>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> Now I will add more feature test cases for remaining formatters. >>>>>>>>> Will send separate patch for feature test cases once completed. >>>>>>>>> >>>>>>>> >>>>>>>> Thanks. >>>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> Please review updated patch. >>>>>>>>> >>>>>>>>> >>>>>>>>> On Tue, May 2, 2017 at 5:57 PM, Surinder Kumar < >>>>>>>>> [email protected]> wrote: >>>>>>>>> >>>>>>>>>> Hi Dave, >>>>>>>>>> >>>>>>>>>> On Tue, May 2, 2017 at 5:21 PM, Dave Page <[email protected]> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> Hi, >>>>>>>>>>> >>>>>>>>>>> This is looking much better now :-). Couple of thoughts and a >>>>>>>>>>> bug: >>>>>>>>>>> >>>>>>>>>>> - Only the TextFormatter seems to handle both [null] and >>>>>>>>>>> [default] values. Shouldn't all formatters do so (including Json and >>>>>>>>>>> Checkmark)? >>>>>>>>>>> >>>>>>>>>> Yes, I will apply the same changes for other formatters too. >>>>>>>>>> >>>>>>>>>>> For example, "serial" columns currently get displayed as [null] >>>>>>>>>>> when left blank, but I would expect to see [default]. >>>>>>>>>>> >>>>>>>>>>> - I would suggest we put [null] and [default] in a lighter >>>>>>>>>>> colour - #999999. >>>>>>>>>>> >>>>>>>>>>> - With the feature test patch added, I seem to be consistently >>>>>>>>>>> getting the following failure (immediately after your new tests run): >>>>>>>>>>> >>>>>>>>>>> ============================================================ >>>>>>>>>>> ========== >>>>>>>>>>> ERROR: runTest (pgadmin.feature_tests.xss_che >>>>>>>>>>> cks_panels_and_query_tool_test.CheckForXssFeatureTest) >>>>>>>>>>> Test XSS check for panels and query tool >>>>>>>>>>> ------------------------------------------------------------ >>>>>>>>>>> ---------- >>>>>>>>>>> Traceback (most recent call last): >>>>>>>>>>> File "/Users/dpage/git/pgadmin4/web >>>>>>>>>>> /regression/feature_utils/base_feature_test.py", line 42, in >>>>>>>>>>> setUp >>>>>>>>>>> self._screenshot() >>>>>>>>>>> File "/Users/dpage/git/pgadmin4/web >>>>>>>>>>> /regression/feature_utils/base_feature_test.py", line 92, in >>>>>>>>>>> _screenshot >>>>>>>>>>> python_version)) >>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>>>> line 802, in get_screenshot_as_file >>>>>>>>>>> png = self.get_screenshot_as_png() >>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>>>> line 821, in get_screenshot_as_png >>>>>>>>>>> return base64.b64decode(self.get_scre >>>>>>>>>>> enshot_as_base64().encode('ascii')) >>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>>>> line 831, in get_screenshot_as_base64 >>>>>>>>>>> return self.execute(Command.SCREENSHOT)['value'] >>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>>>> line 238, in execute >>>>>>>>>>> self.error_handler.check_response(response) >>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/errorhandler.py", >>>>>>>>>>> line 193, in check_response >>>>>>>>>>> raise exception_class(message, screen, stacktrace) >>>>>>>>>>> UnexpectedAlertPresentException: Alert Text: None >>>>>>>>>>> Message: unexpected alert open: {Alert text : Are you sure you >>>>>>>>>>> wish to close the pgAdmin 4 browser?} >>>>>>>>>>> (Session info: chrome=58.0.3029.81) >>>>>>>>>>> (Driver info: chromedriver=2.29.461585 >>>>>>>>>>> (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac OS X >>>>>>>>>>> 10.12.3 x86_64) >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> ============================================================ >>>>>>>>>>> ========== >>>>>>>>>>> ERROR: runTest (pgadmin.feature_tests.xss_che >>>>>>>>>>> cks_pgadmin_debugger_test.CheckDebuggerForXssFeatureTest) >>>>>>>>>>> Test table DDL generation >>>>>>>>>>> ------------------------------------------------------------ >>>>>>>>>>> ---------- >>>>>>>>>>> Traceback (most recent call last): >>>>>>>>>>> File "/Users/dpage/git/pgadmin4/web >>>>>>>>>>> /regression/feature_utils/base_feature_test.py", line 42, in >>>>>>>>>>> setUp >>>>>>>>>>> self._screenshot() >>>>>>>>>>> File "/Users/dpage/git/pgadmin4/web >>>>>>>>>>> /regression/feature_utils/base_feature_test.py", line 92, in >>>>>>>>>>> _screenshot >>>>>>>>>>> python_version)) >>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>>>> line 802, in get_screenshot_as_file >>>>>>>>>>> png = self.get_screenshot_as_png() >>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>>>> line 821, in get_screenshot_as_png >>>>>>>>>>> return base64.b64decode(self.get_scre >>>>>>>>>>> enshot_as_base64().encode('ascii')) >>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>>>> line 831, in get_screenshot_as_base64 >>>>>>>>>>> return self.execute(Command.SCREENSHOT)['value'] >>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", >>>>>>>>>>> line 238, in execute >>>>>>>>>>> self.error_handler.check_response(response) >>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>> dmin4/lib/python2.7/site-packages/selenium/webdriver/remote/errorhandler.py", >>>>>>>>>>> line 193, in check_response >>>>>>>>>>> raise exception_class(message, screen, stacktrace) >>>>>>>>>>> UnexpectedAlertPresentException: Alert Text: None >>>>>>>>>>> Message: unexpected alert open: {Alert text : Are you sure you >>>>>>>>>>> wish to close the pgAdmin 4 browser?} >>>>>>>>>>> (Session info: chrome=58.0.3029.81) >>>>>>>>>>> (Driver info: chromedriver=2.29.461585 >>>>>>>>>>> (0be2cd95f834e9ee7c46bcc7cf405b483f5ae83b),platform=Mac OS X >>>>>>>>>>> 10.12.3 x86_64) >>>>>>>>>>> >>>>>>>>>> Sure. I will fix this. >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Thanks! >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Fri, Apr 28, 2017 at 10:19 AM, Surinder Kumar < >>>>>>>>>>> [email protected]> wrote: >>>>>>>>>>> >>>>>>>>>>>> Hi Dave, >>>>>>>>>>>> >>>>>>>>>>>> Please find updated patch for RM case and a separate patch for >>>>>>>>>>>> Feature tests. >>>>>>>>>>>> >>>>>>>>>>>> *Python:* >>>>>>>>>>>> >>>>>>>>>>>> - Added [default] label for cells with default values while >>>>>>>>>>>> inserting a new row. >>>>>>>>>>>> >>>>>>>>>>>> - Introduced a FieldValidator function for cells that don't >>>>>>>>>>>> allow null values. If user tries to insert null value, field with be >>>>>>>>>>>> highlighted with red borders around. >>>>>>>>>>>> >>>>>>>>>>>> - >>>>>>>>>>>> If a cell contains blank string('') and when we set it to null, >>>>>>>>>>>> the change into the cell is not detected. It was because the comparison >>>>>>>>>>>> for (defaultValue == null) return true if defaultValue is >>>>>>>>>>>> undefined. Hence _.isNull(value) is used to fix this. >>>>>>>>>>>> >>>>>>>>>>>> *Feature Test cases:* >>>>>>>>>>>> >>>>>>>>>>>> - Introduced a new method create_table_with_query(server, >>>>>>>>>>>> db_name, query) in test_utils.py which executes the given >>>>>>>>>>>> query on connected server. >>>>>>>>>>>> >>>>>>>>>>>> - Added a new file test_data.json that has test data for test >>>>>>>>>>>> cases. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Fri, Apr 7, 2017 at 2:21 PM, Dave Page <[email protected]> >>>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> Hi >>>>>>>>>>>>> >>>>>>>>>>>>> On Sat, Apr 1, 2017 at 12:45 PM, Surinder Kumar >>>>>>>>>>>>> <[email protected]> wrote: >>>>>>>>>>>>> > Hi >>>>>>>>>>>>> > >>>>>>>>>>>>> > Issues fixed: >>>>>>>>>>>>> > >>>>>>>>>>>>> > 1. If a column is defined with a default modifier, there is >>>>>>>>>>>>> now way to >>>>>>>>>>>>> > insert the row with those defaults. >>>>>>>>>>>>> > The column will be left blank and it will take default value >>>>>>>>>>>>> automatically. >>>>>>>>>>>>> > >>>>>>>>>>>>> > 2. If a column has a not-null constraint then an error is >>>>>>>>>>>>> returned and the >>>>>>>>>>>>> > row is not inserted. >>>>>>>>>>>>> > The column will be left blank >>>>>>>>>>>>> > >>>>>>>>>>>>> > The default values for new added rows will be displayed on >>>>>>>>>>>>> refresh/execute. >>>>>>>>>>>>> > >>>>>>>>>>>>> > Please find attached patch and review. >>>>>>>>>>>>> >>>>>>>>>>>>> This largely works as expected, but there is some weirdness. I >>>>>>>>>>>>> have a >>>>>>>>>>>>> test table that looks like this: >>>>>>>>>>>>> >>>>>>>>>>>>> CREATE TABLE public.defaults >>>>>>>>>>>>> ( >>>>>>>>>>>>> id bigint NOT NULL DEFAULT nextval('defaults_id_seq'::reg >>>>>>>>>>>>> class), >>>>>>>>>>>>> data_default_nulls text COLLATE pg_catalog."default" >>>>>>>>>>>>> DEFAULT 'abc123'::text, >>>>>>>>>>>>> data_default_no_nulls text COLLATE pg_catalog."default" >>>>>>>>>>>>> NOT NULL >>>>>>>>>>>>> DEFAULT 'def456'::text, >>>>>>>>>>>>> data_nulls text COLLATE pg_catalog."default", >>>>>>>>>>>>> data_no_nulls text COLLATE pg_catalog."default" NOT NULL, >>>>>>>>>>>>> CONSTRAINT defaults_pkey PRIMARY KEY (id) >>>>>>>>>>>>> ) >>>>>>>>>>>>> >>>>>>>>>>>>> Remember that the expected behaviour is: >>>>>>>>>>>>> >>>>>>>>>>>>> - Set a value to empty to update the column to null. >>>>>>>>>>>>> - Set a value to '' to update the column to an empty string >>>>>>>>>>>>> - Set a value to anything else to update the column to that >>>>>>>>>>>>> value >>>>>>>>>>>>> >>>>>>>>>>>>> 1) In a row with values in each column, if I try to set the >>>>>>>>>>>>> value of >>>>>>>>>>>>> data_default_nulls to null, the query executed is: >>>>>>>>>>>>> >>>>>>>>>>>>> UPDATE public.defaults SET >>>>>>>>>>>>> data_default_nulls = '' WHERE >>>>>>>>>>>>> id = '2'; >>>>>>>>>>>>> >>>>>>>>>>>>> 2) If I do the same in the data_nulls column, the value is >>>>>>>>>>>>> immediately >>>>>>>>>>>>> shown as [null] and the query executed is: >>>>>>>>>>>>> >>>>>>>>>>>>> UPDATE public.defaults SET >>>>>>>>>>>>> data_nulls = NULL WHERE >>>>>>>>>>>>> id = '2'; >>>>>>>>>>>>> >>>>>>>>>>>>> 3) If I then edit the value in data_default_nulls, it shows the >>>>>>>>>>>>> current value as ''. Removing the quotes (to set it to null) >>>>>>>>>>>>> doesn't >>>>>>>>>>>>> get detected as a change. >>>>>>>>>>>>> >>>>>>>>>>>> Taken care. >>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> 4) When I manually executed "update defaults set >>>>>>>>>>>>> data_default_nulls = >>>>>>>>>>>>> null where id = 2" in a query tool window, I got: >>>>>>>>>>>>> >>>>>>>>>>>>> 2017-04-07 09:43:02,987: INFO werkzeug: 127.0.0.1 - - >>>>>>>>>>>>> [07/Apr/2017 >>>>>>>>>>>>> 09:43:02] "GET /sqleditor/columns/8745675 HTTP/1.1" 500 - >>>>>>>>>>>>> Traceback (most recent call last): >>>>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>>>> line 2000, in __call__ >>>>>>>>>>>>> return self.wsgi_app(environ, start_response) >>>>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>>>> line 1991, in wsgi_app >>>>>>>>>>>>> response = self.make_response(self.handle_exception(e)) >>>>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>>>> line 1567, in handle_exception >>>>>>>>>>>>> reraise(exc_type, exc_value, tb) >>>>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>>>> line 1988, in wsgi_app >>>>>>>>>>>>> response = self.full_dispatch_request() >>>>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>>>> line 1641, in full_dispatch_request >>>>>>>>>>>>> rv = self.handle_user_exception(e) >>>>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>>>> line 1544, in handle_user_exception >>>>>>>>>>>>> reraise(exc_type, exc_value, tb) >>>>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>>>> line 1639, in full_dispatch_request >>>>>>>>>>>>> rv = self.dispatch_request() >>>>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask/app.py", >>>>>>>>>>>>> line 1625, in dispatch_request >>>>>>>>>>>>> return self.view_functions[rule.endpoint](**req.view_args) >>>>>>>>>>>>> File "/Users/dpage/.virtualenvs/pga >>>>>>>>>>>>> dmin4/lib/python2.7/site-packages/flask_login.py", >>>>>>>>>>>>> line 792, in decorated_view >>>>>>>>>>>>> return func(*args, **kwargs) >>>>>>>>>>>>> File "/Users/dpage/git/pgadmin4/web >>>>>>>>>>>>> /pgadmin/tools/sqleditor/__init__.py", >>>>>>>>>>>>> line 452, in get_columns >>>>>>>>>>>>> tid=command_obj.obj_id) >>>>>>>>>>>>> AttributeError: 'QueryToolCommand' object has no attribute >>>>>>>>>>>>> 'obj_id' >>>>>>>>>>>>> >>>>>>>>>>>> Fixed. >>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> 5) When I run the query again in pgAdmin III, then refresh the >>>>>>>>>>>>> data in >>>>>>>>>>>>> pgAdmin 4, the data_default_nulls column is displayed without >>>>>>>>>>>>> the >>>>>>>>>>>>> [null] marker (despite having a null value, which I confirmed >>>>>>>>>>>>> in >>>>>>>>>>>>> pgAdmin 3). >>>>>>>>>>>>> >>>>>>>>>>>> Fixed. >>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> I'm sure there are other combinations of issues here. Please >>>>>>>>>>>>> fix and >>>>>>>>>>>>> thoroughly re-test to ensure behaviour is consistent - and to >>>>>>>>>>>>> avoid >>>>>>>>>>>>> future issues, please add some appropriate feature tests to >>>>>>>>>>>>> check >>>>>>>>>>>>> nulls, defaults and empty strings are properly handled in >>>>>>>>>>>>> view, insert >>>>>>>>>>>>> and updates. Murtuza recently wrote some feature tests for the >>>>>>>>>>>>> query >>>>>>>>>>>>> tool - you should be able to use those as a starting point. >>>>>>>>>>>>> >>>>>>>>>>>> Added feature tests >>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks. >>>>>>>>>>>>> >>>>>>>>>>>>> -- >>>>>>>>>>>>> Dave Page >>>>>>>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>>>>>>> Twitter: @pgsnake >>>>>>>>>>>>> >>>>>>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>>>>>>> The Enterprise PostgreSQL Company >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> -- >>>>>>>>>>> Dave Page >>>>>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>>>>> Twitter: @pgsnake >>>>>>>>>>> >>>>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>>>>> The Enterprise PostgreSQL Company >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> Dave Page >>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>> Twitter: @pgsnake >>>>>>>> >>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>> The Enterprise PostgreSQL Company >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Dave Page >>>>>> Blog: http://pgsnake.blogspot.com >>>>>> Twitter: @pgsnake >>>>>> >>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>> The Enterprise PostgreSQL Company >>>>>> >>>>> >>>>> >>>> >>>> >>>> -- >>>> Dave Page >>>> Blog: http://pgsnake.blogspot.com >>>> Twitter: @pgsnake >>>> >>>> EnterpriseDB UK: http://www.enterprisedb.com >>>> The Enterprise PostgreSQL Company >>>> >>> >>> >> >> >> -- >> Dave Page >> Blog: http://pgsnake.blogspot.com >> Twitter: @pgsnake >> >> EnterpriseDB UK: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > > > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-10 08:42 Dave Page <[email protected]> parent: Surinder Kumar <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Dave Page @ 2017-05-10 08:42 UTC (permalink / raw) To: Surinder Kumar <[email protected]>; +Cc: pgadmin-hackers On Wed, May 10, 2017 at 9:39 AM, Surinder Kumar < [email protected]> wrote: > Hi Dave, > > On Wed, May 10, 2017 at 2:06 PM, Dave Page <[email protected]> wrote: > >> Any chance we can get this wrapped up today Surinder? >> > I have fixed RM case, I am currently writing its feature test cases which > is taking some time. > Should I send patch for RM case only? I will try to complete test cases > by today eod. > Yes please. Thanks! -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-10 08:52 Surinder Kumar <[email protected]> parent: Dave Page <[email protected]> 0 siblings, 2 replies; 25+ messages in thread From: Surinder Kumar @ 2017-05-10 08:52 UTC (permalink / raw) To: Dave Page <[email protected]>; +Cc: pgadmin-hackers Hi Dave, Please find attached patch for RM only. *Changes:* - All formatters now handles both [null] and [default] values - the cell values are validated on server side as in pgAdmin3. - added light grey color for cells with [null] and [default] placeholders. On Wed, May 10, 2017 at 2:12 PM, Dave Page <[email protected]> wrote: > > > On Wed, May 10, 2017 at 9:39 AM, Surinder Kumar < > [email protected]> wrote: > >> Hi Dave, >> >> On Wed, May 10, 2017 at 2:06 PM, Dave Page <[email protected]> wrote: >> >>> Any chance we can get this wrapped up today Surinder? >>> >> I have fixed RM case, I am currently writing its feature test cases >> which is taking some time. >> Should I send patch for RM case only? I will try to complete test cases >> by today eod. >> > > Yes please. > > Thanks! > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > -- Sent via pgadmin-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers Attachments: [application/octet-stream] RM_2257_v3.patch (13.4K, 3-RM_2257_v3.patch) download | inline diff: diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql index 759e657..f3353d6 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql @@ -1,4 +1,5 @@ -SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype +SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype, +att.attnotnull as not_null, att.atthasdef as has_default_val FROM pg_attribute att JOIN pg_type ty ON ty.oid=atttypid JOIN pg_namespace tn ON tn.oid=ty.typnamespace diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql index 7536a9c..4f1de2a 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql @@ -1,4 +1,5 @@ -SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype +SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype, +att.attnotnull as not_null, att.atthasdef as has_default_val FROM pg_attribute att JOIN pg_type ty ON ty.oid=atttypid JOIN pg_namespace tn ON tn.oid=ty.typnamespace diff --git a/web/pgadmin/static/css/pgadmin.css b/web/pgadmin/static/css/pgadmin.css index 6508feb..1a2d443 100644 --- a/web/pgadmin/static/css/pgadmin.css +++ b/web/pgadmin/static/css/pgadmin.css @@ -780,4 +780,7 @@ lgg-el-container[el=md] .pg-el-lg-8, } .user-language select{ height: 25px !important; +} +.grey_color { + color: #999999; } \ No newline at end of file diff --git a/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js b/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js index cdfba4d..0bdd1ab 100644 --- a/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js +++ b/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js @@ -110,7 +110,12 @@ // When text editor opens this.loadValue = function (item) { - if (item[args.column.pos] === "") { + var col = args.column; + + if (_.isUndefined(item[args.column.pos]) && col.has_default_val) { + $input.val(""); + } + else if (item[args.column.pos] === "") { $input.val("''"); } else { @@ -145,7 +150,10 @@ }; this.isValueChanged = function () { - return (!($input.val() == "" && defaultValue == null)) && ($input.val() != defaultValue); + // Use _.isNull(value) for comparison for null instead of + // defaultValue == null, because it returns true for undefined value. + return (!($input.val() == "" && _.isNull(defaultValue))) && + ($input.val() != defaultValue); }; this.validate = function () { @@ -253,7 +261,7 @@ this.loadValue = function (item) { var data = defaultValue = item[args.column.pos]; - if (typeof data === "object" && !Array.isArray(data)) { + if (data && typeof data === "object" && !Array.isArray(data)) { data = JSON.stringify(data); } else if (Array.isArray(data)) { var temp = []; @@ -282,7 +290,7 @@ }; this.isValueChanged = function () { - return (!($input.val() == "" && defaultValue == null)) && ($input.val() != defaultValue); + return (!($input.val() == "" && _.isNull(defaultValue))) && ($input.val() != defaultValue); }; this.validate = function () { @@ -498,6 +506,12 @@ }; this.validate = function () { + if (args.column.validator) { + var validationResults = args.column.validator(this.serializeValue()); + if (!validationResults.valid) { + return validationResults; + } + } return { valid: true, msg: null @@ -837,7 +851,12 @@ }; this.isValueChanged = function () { - return (!($input.val() == "" && defaultValue == null)) && ($input.val() != defaultValue); + if ($input.val() == "" && defaultValue == "") { + return true; + } else { + return (!($input.val() == "" && _.isNull(defaultValue ))) && + ($input.val() != defaultValue); + } }; this.validate = function () { diff --git a/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js b/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js index 290bddd..924f523 100644 --- a/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js +++ b/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js @@ -19,8 +19,15 @@ }); function JsonFormatter(row, cell, value, columnDef, dataContext) { - if (value == null || value === "") { - return ""; + // If column has default value, set placeholder + if (_.isUndefined(value) && columnDef.has_default_val) { + return "<span class='pull-left grey_color'>[default]</span>"; + } + else if ( + (_.isUndefined(value) && columnDef.not_null) || + (_.isUndefined(value) || value === null) + ) { + return "<span class='pull-left grey_color'>[null]</span>"; } else { // Stringify only if it's json object if (typeof value === "object" && !Array.isArray(value)) { @@ -42,11 +49,15 @@ } function NumbersFormatter(row, cell, value, columnDef, dataContext) { - if (_.isUndefined(value) || value === null) { - return "<span class='pull-right'>[null]</span>"; + // If column has default value, set placeholder + if (_.isUndefined(value) && columnDef.has_default_val) { + return "<span class='pull-right'>[default]</span>"; } - else if (value === "") { - return ''; + else if ( + (_.isUndefined(value) || value === null || value === "") || + (_.isUndefined(value) && columnDef.not_null) + ) { + return "<span class='pull-right'>[null]</span>"; } else { return "<span style='float:right'>" + _.escape(value) + "</span>"; @@ -57,17 +68,30 @@ /* Checkbox has 3 states * 1) checked=true * 2) unchecked=false - * 3) indeterminate=null/'' + * 3) indeterminate=null */ - if (value == null || value === "") { - return "<span class='pull-left'>[null]</span>"; + if (_.isUndefined(value) && columnDef.has_default_val) { + return "<span class='pull-left grey_color'>[default]</span>"; + } + else if ( + (_.isUndefined(value) && columnDef.not_null) || + (value == null || value === "") + ) { + return "<span class='pull-left grey_color'>[null]</span>"; } return value ? "true" : "false"; } function TextFormatter(row, cell, value, columnDef, dataContext) { - if (_.isUndefined(value) || value === null) { - return "<span class='pull-left'>[null]</span>"; + // If column has default value, set placeholder + if (_.isUndefined(value) && columnDef.has_default_val) { + return "<span class='pull-left grey_color'>[default]</span>"; + } + else if ( + (_.isUndefined(value) && columnDef.not_null) || + (_.isUndefined(value) || _.isNull(value)) + ) { + return "<span class='pull-left grey_color'>[null]</span>"; } else { return _.escape(value); diff --git a/web/pgadmin/tools/sqleditor/__init__.py b/web/pgadmin/tools/sqleditor/__init__.py index d114988..f7466d8 100644 --- a/web/pgadmin/tools/sqleditor/__init__.py +++ b/web/pgadmin/tools/sqleditor/__init__.py @@ -440,8 +440,23 @@ def get_columns(trans_id): columns = dict() columns_info = None primary_keys = None + rset = None status, error_msg, conn, trans_obj, session_obj = check_transaction_status(trans_id) if status and conn is not None and session_obj is not None: + + ver = conn.manager.version + # Get the template path for the column + template_path = 'column/sql/#{0}#'.format(ver) + command_obj = pickle.loads(session_obj['command_obj']) + if hasattr(command_obj, 'obj_id'): + SQL = render_template("/".join([template_path, + 'nodes.sql']), + tid=command_obj.obj_id) + # rows with attribute not_null + status, rset = conn.execute_2darray(SQL) + if not status: + return internal_server_error(errormsg=rset) + # Check PK column info is available or not if 'primary_keys' in session_obj: primary_keys = session_obj['primary_keys'] @@ -449,10 +464,17 @@ def get_columns(trans_id): # Fetch column information columns_info = conn.get_column_info() if columns_info is not None: - for col in columns_info: + for key, col in enumerate(columns_info): col_type = dict() col_type['type_code'] = col['type_code'] col_type['type_name'] = None + if rset: + col_type['not_null'] = col['not_null'] = \ + rset['rows'][key]['not_null'] + + col_type['has_default_val'] = col['has_default_val'] = \ + rset['rows'][key]['has_default_val'] + columns[col['name']] = col_type # As we changed the transaction object we need to @@ -602,6 +624,7 @@ def save(trans_id): status, error_msg, conn, trans_obj, session_obj = check_transaction_status(trans_id) if status and conn is not None \ and trans_obj is not None and session_obj is not None: + setattr(trans_obj, 'columns_info', session_obj['columns_info']) # If there is no primary key found then return from the function. if len(session_obj['primary_keys']) <= 0 or len(changed_data) <= 0: diff --git a/web/pgadmin/tools/sqleditor/command.py b/web/pgadmin/tools/sqleditor/command.py index 1795155..29ed23f 100644 --- a/web/pgadmin/tools/sqleditor/command.py +++ b/web/pgadmin/tools/sqleditor/command.py @@ -442,6 +442,23 @@ class TableCommand(GridCommand): # For newly added rows if of_type == 'added': + + # When new rows are added, only changed columns data is + # sent from client side. But if column is not_null and has + # no_default_value, set column to blank, instead + # of not null which is set by default. + column_data = {} + column_type = {} + for each_col in self.columns_info: + if ( + self.columns_info[each_col]['not_null'] and + not self.columns_info[each_col][ + 'has_default_val'] + ): + column_data[each_col] = None + column_type[each_col] =\ + self.columns_info[each_col]['type_name'] + for each_row in changed_data[of_type]: data = changed_data[of_type][each_row]['data'] # Remove our unique tracking key @@ -450,12 +467,18 @@ class TableCommand(GridCommand): data_type = set_column_names(changed_data[of_type][each_row]['data_type']) list_of_rowid.append(data.get('__temp_PK')) + # Update columns value and data type + # with columns having not_null=False and has + # no default value + column_data.update(data) + column_type.update(data_type) + sql = render_template("/".join([self.sql_path, 'insert.sql']), - data_to_be_saved=data, + data_to_be_saved=column_data, primary_keys=None, object_name=self.object_name, nsp_name=self.nsp_name, - data_type=data_type) + data_type=column_type) list_of_sql.append(sql) # For updated rows diff --git a/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js b/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js index 2062aa2..bd2b3ff 100644 --- a/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js +++ b/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js @@ -557,7 +557,9 @@ define( id: c.name, pos: c.pos, field: c.name, - name: c.label + name: c.label, + not_null: c.not_null, + has_default_val: c.has_default_val }; // Get the columns width based on data type @@ -2077,7 +2079,9 @@ define( 'label': column_label, 'cell': col_cell, 'can_edit': self.can_edit, - 'type': type + 'type': type, + 'not_null': c.not_null, + 'has_default_val': c.has_default_val }; columns.push(col); }); ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-10 14:02 Surinder Kumar <[email protected]> parent: Surinder Kumar <[email protected]> 1 sibling, 1 reply; 25+ messages in thread From: Surinder Kumar @ 2017-05-10 14:02 UTC (permalink / raw) To: Dave Page <[email protected]>; +Cc: pgadmin-hackers Hi Dave, Please find attached patch for Feature test cases for RM_2257 *Implementation detail:* - Added a test_data.json file which contains Insert and Update test related input data - First of all, we create three tables such as a) defaults_text b) defaults_boolean c) defaults_number d) defaults_json These tables has columns with different constraints (default value, not_null etc) to test with various input test data. - Test cases for insert are executed first and then test cases for update. Please review the patch. On Wed, May 10, 2017 at 2:22 PM, Surinder Kumar < [email protected]> wrote: > Hi Dave, > > Please find attached patch for RM only. > > *Changes:* > > - All formatters now handles both [null] and [default] values > > - the cell values are validated on server side as in pgAdmin3. > > - added light grey color for cells with [null] and [default] placeholders. > > On Wed, May 10, 2017 at 2:12 PM, Dave Page <[email protected]> wrote: > >> >> >> On Wed, May 10, 2017 at 9:39 AM, Surinder Kumar < >> [email protected]> wrote: >> >>> Hi Dave, >>> >>> On Wed, May 10, 2017 at 2:06 PM, Dave Page <[email protected]> wrote: >>> >>>> Any chance we can get this wrapped up today Surinder? >>>> >>> I have fixed RM case, I am currently writing its feature test cases >>> which is taking some time. >>> Should I send patch for RM case only? I will try to complete test cases >>> by today eod. >>> >> >> Yes please. >> >> Thanks! >> >> -- >> Dave Page >> Blog: http://pgsnake.blogspot.com >> Twitter: @pgsnake >> >> EnterpriseDB UK: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > > -- Sent via pgadmin-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers Attachments: [application/octet-stream] features_test_cases_RM_2257_v1.patch (23.3K, 3-features_test_cases_RM_2257_v1.patch) download | inline diff: diff --git a/web/pgadmin/feature_tests/test_data.json b/web/pgadmin/feature_tests/test_data.json new file mode 100644 index 0000000..672b217 --- /dev/null +++ b/web/pgadmin/feature_tests/test_data.json @@ -0,0 +1,88 @@ +{ + "table_insert_update_cases": { + + "defaults_text": { + "insert": { + "insert_with_defaults": { + "id": "1", + "data_default_nulls": "abc123", + "data_default_no_nulls": "def456", + "data_no_nulls": "test string" + } + }, + "update": { + "update_with_null_value": { + "id": "12", + "data_default_nulls": "", + "data_nulls": "" + }, + "update_with_empty_string": { + "id": "15", + "data_default_nulls": "''", + "data_nulls": "''" + } + } + }, + "defaults_boolean": { + "insert": { + "insert_with_defaults": { + "id": "1", + "data_default_nulls": "false", + "data_default_no_nulls": "false", + "data_no_nulls": "true" + } + }, + "update": { + "update_with_boolean_value": { + "id": "12", + "data_default_nulls": "true", + "data_nulls": "true" + } + } + }, + "defaults_number": { + "insert": { + "insert_with_defaults": { + "id": "1", + "data_default_nulls": "10", + "data_default_no_nulls": "20", + "data_no_nulls": "10" + } + }, + "update": { + "update_with_null_value": { + "id": "12", + "data_default_nulls": "", + "data_nulls": "" + }, + "update_with_numeric_value": { + "id": "15", + "data_default_nulls": "12", + "data_nulls": "13" + } + } + }, + "defaults_json": { + "insert": { + "insert_with_defaults": { + "id": "1", + "data_default_nulls": "[0,1]", + "data_default_no_nulls": "[2,3]", + "data_no_nulls": "[4,5]" + } + }, + "update": { + "update_with_json_data": { + "id": "12", + "data_default_nulls": "[10,11,12]", + "data_nulls": "[3,4]" + }, + "update_with_null_value": { + "id": "15", + "data_default_nulls": "", + "data_nulls": "" + } + } + } + } +} \ No newline at end of file diff --git a/web/pgadmin/feature_tests/view_data_dml_queries.py b/web/pgadmin/feature_tests/view_data_dml_queries.py new file mode 100644 index 0000000..56d89a4 --- /dev/null +++ b/web/pgadmin/feature_tests/view_data_dml_queries.py @@ -0,0 +1,486 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2017, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +import json +import os +from selenium.webdriver import ActionChains +from regression.python_test_utils import test_utils +from regression.feature_utils.base_feature_test import BaseFeatureTest +import time +from selenium.webdriver.common.keys import Keys +from selenium.common.exceptions import NoSuchElementException + + +CURRENT_PATH = os.path.dirname(os.path.realpath(__file__)) + +try: + with open(CURRENT_PATH + '/test_data.json') as data_file: + config_data = json.load(data_file)['table_insert_update_cases'] +except Exception as e: + print(str(e)) + + +class CheckForViewDataTest(BaseFeatureTest): + """ + Test cases to validate insert, update operations in table + with input test data + + First of all, the test data is inserted/updated into table and then + inserted data is compared with original data to check if expected data + is returned from table or not. + + We will cover test cases for, + 1) Insert with default values + 2) Update with null values + 3) Update with blank string + """ + + scenarios = [ + ("Validate Insert, Update operations in View data with given test " + "data", + dict()) + ] + + # To create column id with nextval, first a sequence must be created. + create_sequence = """ +CREATE SEQUENCE public.defaults_text_id_seq + INCREMENT 1 + START 9 + MINVALUE 1 + MAXVALUE 9223372036854775807 + CACHE 1; + +ALTER SEQUENCE public.defaults_text_id_seq + OWNER TO postgres; +""" + + # query for creating 'defaults_text' table + defaults_text_query = """ +CREATE TABLE public.defaults_text +( + id bigint NOT NULL DEFAULT nextval('defaults_text_id_seq'::regclass), + data_default_nulls text COLLATE pg_catalog."default" DEFAULT 'abc123'::text, + data_default_no_nulls text COLLATE pg_catalog."default" NOT NULL +DEFAULT 'def456'::text, + data_nulls text COLLATE pg_catalog."default", + data_no_nulls text COLLATE pg_catalog."default" NOT NULL, + CONSTRAINT defaults_text_pkey PRIMARY KEY (id) +) +""" + + # query for creating 'defaults_boolean' table + defaults_boolean_query = """ +CREATE TABLE public.defaults_boolean +( + id bigint NOT NULL, + data_default_nulls boolean DEFAULT false, + data_default_no_nulls boolean NOT NULL DEFAULT false, + data_nulls boolean, + data_no_nulls boolean NOT NULL, + CONSTRAINT default_boolean_pkey PRIMARY KEY (id) +) +""" + + # query for creating 'defaults_number' table + defaults_number_query = """ +CREATE TABLE public.defaults_number +( + id bigint NOT NULL, + data_default_nulls numeric(100) DEFAULT 10, + data_default_no_nulls numeric(100) NOT NULL DEFAULT 20, + data_nulls numeric(100), + data_no_nulls numeric(100) NOT NULL, + CONSTRAINT default_number_pkey PRIMARY KEY (id) +) +""" + + # query for creating 'defaults_json' table + defaults_json_query = """ +CREATE TABLE public.defaults_json +( + id bigint NOT NULL, + data_default_nulls json DEFAULT '[0,1]'::json, + data_default_no_nulls json NOT NULL DEFAULT '[2,3]'::json, + data_nulls json, + data_no_nulls json NOT NULL, + CONSTRAINT defaults_json_pkey PRIMARY KEY (id) +) +""" + + def before(self): + connection = test_utils.get_db_connection(self.server['db'], + self.server['username'], + self.server['db_password'], + self.server['host'], + self.server['port']) + test_utils.drop_database(connection, "acceptance_test_db") + test_utils.create_database(self.server, "acceptance_test_db") + test_utils.create_table_with_query( + self.server, + "acceptance_test_db", + CheckForViewDataTest.create_sequence) + + # Create pre-requisite tables + for key in config_data.keys(): + test_utils.create_table_with_query( + self.server, + "acceptance_test_db", + getattr(CheckForViewDataTest, key+'_query')) + + def runTest(self): + self.page.wait_for_spinner_to_disappear() + self._connects_to_server() + + self._tables_node_expandable() + for key in config_data.keys(): + self.driver.switch_to.default_content() + self.page.select_tree_item(key) + # Open Object -> View data + self._check_xss_in_view_data() + + # Run test to insert a new row in table with default values + self._insert_row_in_table(key) + self._verify_insert_data(key) + self._close_query_tool() + + # Run update cells test cases + for key in config_data.keys(): + self.driver.switch_to.default_content() + self.page.select_tree_item(key) + # Open Object -> View data + self._check_xss_in_view_data() + + # Run test to update existing rows in table with given values + self._update_row_in_table(key) + self._close_query_tool() + + def after(self): + time.sleep(1) + self.page.remove_server(self.server) + connection = test_utils.get_db_connection(self.server['db'], + self.server['username'], + self.server['db_password'], + self.server['host'], + self.server['port']) + test_utils.drop_database(connection, "acceptance_test_db") + + def _connects_to_server(self): + self.page.find_by_xpath("//*[@class='aciTreeText' and .='Servers']").click() + self.page.driver.find_element_by_link_text("Object").click() + ActionChains(self.page.driver) \ + .move_to_element(self.page.driver.find_element_by_link_text("Create")) \ + .perform() + self.page.find_by_partial_link_text("Server...").click() + + server_config = self.server + self.page.fill_input_by_field_name("name", server_config['name']) + self.page.find_by_partial_link_text("Connection").click() + self.page.fill_input_by_field_name("host", server_config['host']) + self.page.fill_input_by_field_name("port", server_config['port']) + self.page.fill_input_by_field_name("username", server_config['username']) + self.page.fill_input_by_field_name("password", server_config['db_password']) + self.page.find_by_xpath("//button[contains(.,'Save')]").click() + + def _tables_node_expandable(self): + self.page.toggle_open_tree_item(self.server['name']) + self.page.toggle_open_tree_item('Databases') + self.page.toggle_open_tree_item('acceptance_test_db') + self.page.toggle_open_tree_item('Schemas') + self.page.toggle_open_tree_item('public') + self.page.toggle_open_tree_item('Tables') + + def _check_xss_in_view_data(self): + self.page.driver.find_element_by_link_text("Object").click() + ActionChains(self.page.driver) \ + .move_to_element( + self.page.driver.find_element_by_link_text("View Data")) \ + .perform() + self.page.find_by_partial_link_text("View All Rows").click() + time.sleep(3) + self.page.driver.switch_to.frame( + self.page.driver.find_element_by_tag_name('iframe') + ) + + def _insert_row_in_table(self, table): + xpath_cell1 = "//div[contains(@class, 'new-row')]//div[" \ + "contains(@class, 'r1')]" + + cell1_val = config_data[table]['insert']['insert_with_defaults'][ + 'id'] + + cell2_val = config_data[table]['insert']['insert_with_defaults'][ + 'data_no_nulls'] + + time.sleep(1) + # Insert into first cell + new_row = self.page.find_by_xpath(xpath_cell1) + new_row.click() + field = new_row.find_element_by_xpath(".//input") + field.click() + ActionChains(self.driver).send_keys(cell1_val).perform() + field.send_keys(Keys.TAB) + + # # Insert into last cell. To insert into cells like boolean, + # number, json has different process, so the code is conditional + if table == 'defaults_boolean': + xpath_cell5 = "//*[@id='datagrid']//div[" \ + "contains(@class, 'new_row')]//div[" \ + "contains(@class, 'r5')]" + cell5_el = self.page.find_by_xpath(xpath_cell5) + ActionChains(self.driver).move_to_element(cell5_el).double_click( + cell5_el + ).perform() + time.sleep(0.4) + + checkbox_el = cell5_el.find_element_by_xpath(".//input") + checkbox_el.click() + ActionChains(self.driver).move_to_element(checkbox_el).double_click( + checkbox_el + ).perform() + checkbox_el.send_keys(Keys.TAB) # Press tab key + + elif table == 'defaults_number': + xpath_cell5 = "//*[@id='datagrid']//div[" \ + "contains(@class,'new_row')]//div[" \ + "contains(@class, 'r5')]" + cell5_el = self.page.find_by_xpath(xpath_cell5) + + ActionChains(self.driver).move_to_element(cell5_el).double_click( + cell5_el + ).perform() + + time.sleep(0.5) + cell5_input_el = cell5_el.find_element_by_xpath('.//input') + cell5_input_el.click() + ActionChains(self.driver).send_keys(cell2_val).perform() + cell5_input_el.send_keys(Keys.TAB) # Press tab key + else: + xpath_cell5 = "//*[@id='datagrid']//div[contains(@class, " \ + "'new_row')]//div[contains(@class, 'r5')]" + cell_el = self.page.find_by_xpath(xpath_cell5) + + ActionChains(self.driver).move_to_element(cell_el).double_click( + cell_el + ).perform() + + cell5_el = self.page.driver.find_element_by_css_selector( + "div[style*='z-index: 1000'] textarea" + ) + cell5_el.clear() + cell5_el.click() + time.sleep(0.5) + + ActionChains(self.driver).send_keys(cell2_val).perform() + + self.page.driver.find_element_by_css_selector( + "div[style*='z-index: 1000'] div button:first-child" + ).click() # Click on editor's Save button + + self.page.find_by_id("btn-save").click() # Save data + + def _verify_insert_data(self, table): + time.sleep(0.5) + self.page.find_by_id("btn-flash").click() + time.sleep(1) + main_el = self.page.find_by_xpath('//*[@id="datagrid"]') + cell1 = main_el.find_element_by_xpath( + './/div[contains(@class, "r1")]//span' + ).get_attribute('innerHTML') + try: + cell2 = main_el.find_element_by_xpath( + './/div[contains(@class, "r2")]//span' + ).get_attribute('innerHTML') + except NoSuchElementException: + cell2 = main_el.find_element_by_xpath( + './/div[contains(@class, "r2")]' + ).get_attribute('innerHTML') + + try: + cell3 = main_el.find_element_by_xpath( + './/div[contains(@class, "r3")]//span' + ).get_attribute('innerHTML') + except NoSuchElementException: + cell3 = main_el.find_element_by_xpath( + './/div[contains(@class, "r3")]' + ).get_attribute('innerHTML') + + try: + cell5 = main_el.find_element_by_xpath( + './/div[contains(@class, "r5")]//span' + ).get_attribute('innerHTML') + except NoSuchElementException: + cell5 = main_el.find_element_by_xpath( + './/div[contains(@class, "r5")]' + ).get_attribute('innerHTML') + + test_verify_data = config_data[table]['insert'][ + 'insert_with_defaults'] + + # compare updated cell values with original values + self.assertEquals(cell1, test_verify_data['id']) + self.assertEquals(cell2, test_verify_data['data_default_nulls']) + self.assertEquals(cell3, test_verify_data['data_default_no_nulls']) + self.assertEquals(cell5, test_verify_data['data_no_nulls']) + + def _update_numeric_cell(self, xpath, value): + """ + This function updates the given cell(xpath) with + given value + Args: + xpath: xpath of cell element + value: cell value to update + + Returns: None + + """ + time.sleep(2) + cell_el = self.page.find_by_xpath(xpath) + ActionChains(self.driver).move_to_element(cell_el).double_click( + cell_el + ).perform() + field = cell_el.find_element_by_xpath(".//input") + field.clear() + field.click() + ActionChains(self.driver).send_keys(value).perform() + field.send_keys(Keys.TAB) # Press tab key + + def _update_text_cell(self, xpath, value): + """ + This function updates the given cell(xpath) with + given value + Args: + xpath: xpath of cell element + value: cell value to update + + Returns: None + + """ + cell_el = self.page.find_by_xpath(xpath) + ActionChains(self.driver).move_to_element(cell_el).double_click( + cell_el).perform() + field = self.page.driver.find_element_by_css_selector( + "div[style*='z-index: 1000'] textarea" + ) + field.clear() + field.click() + time.sleep(1) + ActionChains(self.driver).send_keys(value).perform() + time.sleep(1) + self.page.driver.find_element_by_css_selector( + "div[style*='z-index: 1000'] div button:first-child" + ).click() # Click on editor's Save button + + def _update_boolean_cell(self, xpath, value): + """ + This function updates the given cell(xpath) with + given value + Args: + xpath: xpath of cell element + value: cell value to update + + Returns: None + + """ + cell_el = self.page.find_by_xpath(xpath) + ActionChains(self.driver).move_to_element(cell_el).double_click( + cell_el + ).perform() + time.sleep(0.4) + + checkbox_el = cell_el.find_element_by_xpath(".//input") + checkbox_el.click() + ActionChains(self.driver).move_to_element(checkbox_el).double_click( + checkbox_el + ).perform() + checkbox_el.send_keys(Keys.TAB) # Press tab key + + def _update_row_in_table(self, table): + xpath_cell1 = "//div[contains(@class, 'even')]//div[" \ + "contains(@class, 'r1')]" + xpath_cell2 = "//div[contains(@class, 'even')]//div[" \ + "contains(@class, 'r2')]" + xpath_cell3 = "//div[contains(@class, 'even')]//div[" \ + "contains(@class, 'r4')]" + + update_cases = config_data[table]['update'] + + for row in update_cases: + cell1 = update_cases[row]['id'] + cell2 = update_cases[row]['data_default_nulls'] + cell3 = update_cases[row]['data_nulls'] + + if table == 'defaults_boolean': + self._update_numeric_cell(xpath_cell1, cell1) + self._update_boolean_cell(xpath_cell2, cell2) + self._update_boolean_cell(xpath_cell3, cell3) + elif table == 'defaults_number': + self._update_numeric_cell(xpath_cell1, cell1) + self._update_numeric_cell(xpath_cell2, cell2) + self._update_numeric_cell(xpath_cell3, cell3) + else: + self._update_numeric_cell(xpath_cell1, cell1) + self._update_text_cell(xpath_cell2, cell2) + self._update_text_cell(xpath_cell3, cell3) + + self.page.find_by_id("btn-save").click() # Save data + self._verify_update_data(table, row) + + def _get_cell_value(self, table, xpath): + """ + This function extracts the value from markup and then + manipulate if necessary + Args: + table: test case type [we are using table_name] + xpath: xpath for dom element + + Returns: element value + + """ + + cell = self.page.find_by_xpath(xpath).get_attribute('innerHTML') + if cell.find('<span') != -1: + cell = self.page.find_by_xpath( + '{0}//span'.format(xpath) + ).get_attribute('innerHTML') + + if cell == '[null]': + cell = '' + else: + if table == 'defaults_text': + cell = "''" if cell == '' else cell + return cell + + def _verify_update_data(self, table, row): + time.sleep(0.5) + self.page.find_by_id("btn-flash").click() + time.sleep(1) + + cell1_xpath = './/div[contains(@class, "r1")]' + cell1 = self._get_cell_value(table, cell1_xpath) + + cell2_xpath = './/div[contains(@class, "r2")]' + cell2 = self._get_cell_value(table, cell2_xpath) + + cell4_xpath = './/div[contains(@class, "r4")]' + cell4 = self._get_cell_value(table, cell4_xpath) + + test_verify_data = config_data[table]['update'][row] + + # compare updated cell values with original values + self.assertEquals(cell1, test_verify_data['id']) + self.assertEquals(cell2, test_verify_data['data_default_nulls']) + self.assertEquals(cell4, test_verify_data['data_nulls']) + + def _close_query_tool(self): + time.sleep(0.5) + self.page.driver.switch_to_default_content() + self.page.click_element( + self.page.find_by_xpath( + "//*[@id='dockerContainer']/div/div[3]/div/div[2]/div[1]") + ) \ No newline at end of file diff --git a/web/regression/feature_utils/pgadmin_page.py b/web/regression/feature_utils/pgadmin_page.py index f5d0ac7..4bb7d0f 100644 --- a/web/regression/feature_utils/pgadmin_page.py +++ b/web/regression/feature_utils/pgadmin_page.py @@ -33,11 +33,11 @@ class PgadminPage: self.click_modal_ok() self.wait_for_reloading_indicator_to_disappear() - def click_modal_ok(self): + def click_modal_ok(self, btn_label='OK'): time.sleep(0.5) # Find active alertify dialog in case of multiple alertify dialog & click on that dialog self.click_element( - self.find_by_xpath("//div[contains(@class, 'alertify') and not(contains(@class, 'ajs-hidden'))]//button[.='OK']") + self.find_by_xpath("//*//div[contains(@class, 'alertify') and not(contains(@class, 'ajs-hidden'))]//button[.='{0}']".format(btn_label)) ) def add_server(self, server_config): diff --git a/web/regression/python_test_utils/test_utils.py b/web/regression/python_test_utils/test_utils.py index 2b7c695..c50dd31 100644 --- a/web/regression/python_test_utils/test_utils.py +++ b/web/regression/python_test_utils/test_utils.py @@ -172,6 +172,35 @@ def create_table(server, db_name, table_name): except Exception: traceback.print_exc(file=sys.stderr) + +def create_table_with_query(server, db_name, query): + """ + This function create the table in given database name + :param server: server details + :type server: dict + :param db_name: database name + :type db_name: str + :param query: create table query + :type query: str + :return: None + """ + try: + connection = get_db_connection(db_name, + server['username'], + server['db_password'], + server['host'], + server['port']) + old_isolation_level = connection.isolation_level + connection.set_isolation_level(0) + pg_cursor = connection.cursor() + pg_cursor.execute(query) + connection.set_isolation_level(old_isolation_level) + connection.commit() + + except Exception: + traceback.print_exc(file=sys.stderr) + + def create_constraint( server, db_name, table_name, constraint_type="unique", constraint_name="test_unique"): @@ -274,7 +303,6 @@ def drop_database(connection, database_name): connection.commit() connection.close() - def drop_tablespace(connection): """This function used to drop the tablespace""" pg_cursor = connection.cursor() ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-11 08:58 Dave Page <[email protected]> parent: Surinder Kumar <[email protected]> 1 sibling, 1 reply; 25+ messages in thread From: Dave Page @ 2017-05-11 08:58 UTC (permalink / raw) To: Surinder Kumar <[email protected]>; +Cc: pgadmin-hackers Hi There seems to be couple of bugs in this; - When creating a new row with my test table, if I click in the id column, don't change anything, then click in another column, the ID column value changes from [default] to [null], making it impossible to save that row with the default value. In this case I would expect it to stay at [default] unless I explicitly entered a value. - When I add a new row, but leave the id as [default], the row is saved, but the [default] marker changes from gray to black (but only in the id column. - I'm able to edit a freshly added row immediately after saving but before refreshing. This shouldn't be allowed if we don't know what the primary key value is, as it leads to failed updates such as: On Wed, May 10, 2017 at 9:52 AM, Surinder Kumar < [email protected]> wrote: > Hi Dave, > > Please find attached patch for RM only. > > *Changes:* > > - All formatters now handles both [null] and [default] values > > - the cell values are validated on server side as in pgAdmin3. > > - added light grey color for cells with [null] and [default] placeholders. > > On Wed, May 10, 2017 at 2:12 PM, Dave Page <[email protected]> wrote: > >> >> >> On Wed, May 10, 2017 at 9:39 AM, Surinder Kumar < >> [email protected]> wrote: >> >>> Hi Dave, >>> >>> On Wed, May 10, 2017 at 2:06 PM, Dave Page <[email protected]> wrote: >>> >>>> Any chance we can get this wrapped up today Surinder? >>>> >>> I have fixed RM case, I am currently writing its feature test cases >>> which is taking some time. >>> Should I send patch for RM case only? I will try to complete test cases >>> by today eod. >>> >> >> Yes please. >> >> Thanks! >> >> -- >> Dave Page >> Blog: http://pgsnake.blogspot.com >> Twitter: @pgsnake >> >> EnterpriseDB UK: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-11 08:59 Dave Page <[email protected]> parent: Dave Page <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Dave Page @ 2017-05-11 08:59 UTC (permalink / raw) To: Surinder Kumar <[email protected]>; +Cc: pgadmin-hackers Ooops, managed to hit send too soon. ... it leads to failed updates such as: 2017-05-11 09:55:47,570: SQL pgadmin: Execute (void) for server #1 - CONN:2096775 (Query-id: 4540472): UPDATE public.defaults SET data_default_no_nulls = 'asas' WHERE ; 2017-05-11 09:55:47,577: ERROR pgadmin: Failed to execute query (execute_void) for the server #1 - CONN:2096775 (Query-id: 4540472): Error Message:ERROR: syntax error at or near ";" LINE 3: ; On Thu, May 11, 2017 at 9:58 AM, Dave Page <[email protected]> wrote: > Hi > > There seems to be couple of bugs in this; > > - When creating a new row with my test table, if I click in the id column, > don't change anything, then click in another column, the ID column value > changes from [default] to [null], making it impossible to save that row > with the default value. In this case I would expect it to stay at [default] > unless I explicitly entered a value. > > - When I add a new row, but leave the id as [default], the row is saved, > but the [default] marker changes from gray to black (but only in the id > column. > > - I'm able to edit a freshly added row immediately after saving but before > refreshing. This shouldn't be allowed if we don't know what the primary key > value is, as it leads to failed updates such as: > > > On Wed, May 10, 2017 at 9:52 AM, Surinder Kumar < > [email protected]> wrote: > >> Hi Dave, >> >> Please find attached patch for RM only. >> >> *Changes:* >> >> - All formatters now handles both [null] and [default] values >> >> - the cell values are validated on server side as in pgAdmin3. >> >> - added light grey color for cells with [null] and [default] >> placeholders. >> >> On Wed, May 10, 2017 at 2:12 PM, Dave Page <[email protected]> wrote: >> >>> >>> >>> On Wed, May 10, 2017 at 9:39 AM, Surinder Kumar < >>> [email protected]> wrote: >>> >>>> Hi Dave, >>>> >>>> On Wed, May 10, 2017 at 2:06 PM, Dave Page <[email protected]> wrote: >>>> >>>>> Any chance we can get this wrapped up today Surinder? >>>>> >>>> I have fixed RM case, I am currently writing its feature test cases >>>> which is taking some time. >>>> Should I send patch for RM case only? I will try to complete test >>>> cases by today eod. >>>> >>> >>> Yes please. >>> >>> Thanks! >>> >>> -- >>> Dave Page >>> Blog: http://pgsnake.blogspot.com >>> Twitter: @pgsnake >>> >>> EnterpriseDB UK: http://www.enterprisedb.com >>> The Enterprise PostgreSQL Company >>> >> >> > > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-12 05:39 Surinder Kumar <[email protected]> parent: Dave Page <[email protected]> 0 siblings, 0 replies; 25+ messages in thread From: Surinder Kumar @ 2017-05-12 05:39 UTC (permalink / raw) To: Dave Page <[email protected]>; +Cc: pgadmin-hackers Hi Dave, Please find updated patch On Thu, May 11, 2017 at 2:29 PM, Dave Page <[email protected]> wrote: > Ooops, managed to hit send too soon. > > ... it leads to failed updates such as: > > 2017-05-11 09:55:47,570: SQL pgadmin: Execute (void) for server #1 - > CONN:2096775 (Query-id: 4540472): > UPDATE public.defaults SET > data_default_no_nulls = 'asas' WHERE > ; > 2017-05-11 09:55:47,577: ERROR pgadmin: > Failed to execute query (execute_void) for the server #1 - CONN:2096775 > (Query-id: 4540472): > Error Message:ERROR: syntax error at or near ";" > LINE 3: ; > The new rows added is kept disabled untill grid is not refreshed. > > > On Thu, May 11, 2017 at 9:58 AM, Dave Page <[email protected]> wrote: > >> Hi >> >> There seems to be couple of bugs in this; >> >> - When creating a new row with my test table, if I click in the id >> column, don't change anything, then click in another column, the ID column >> value changes from [default] to [null], making it impossible to save that >> row with the default value. In this case I would expect it to stay at >> [default] unless I explicitly entered a value. >> > Fixed. > >> - When I add a new row, but leave the id as [default], the row is saved, >> but the [default] marker changes from gray to black (but only in the id >> column. >> > I forgot to add 'grey_color' class for numeric type fields. Now added. > >> - I'm able to edit a freshly added row immediately after saving but >> before refreshing. This shouldn't be allowed if we don't know what the >> primary key value is, as it leads to failed updates such as: >> >> >> On Wed, May 10, 2017 at 9:52 AM, Surinder Kumar < >> [email protected]> wrote: >> >>> Hi Dave, >>> >>> Please find attached patch for RM only. >>> >>> *Changes:* >>> >>> - All formatters now handles both [null] and [default] values >>> >>> - the cell values are validated on server side as in pgAdmin3. >>> >>> - added light grey color for cells with [null] and [default] >>> placeholders. >>> >>> On Wed, May 10, 2017 at 2:12 PM, Dave Page <[email protected]> wrote: >>> >>>> >>>> >>>> On Wed, May 10, 2017 at 9:39 AM, Surinder Kumar < >>>> [email protected]> wrote: >>>> >>>>> Hi Dave, >>>>> >>>>> On Wed, May 10, 2017 at 2:06 PM, Dave Page <[email protected]> wrote: >>>>> >>>>>> Any chance we can get this wrapped up today Surinder? >>>>>> >>>>> I have fixed RM case, I am currently writing its feature test cases >>>>> which is taking some time. >>>>> Should I send patch for RM case only? I will try to complete test >>>>> cases by today eod. >>>>> >>>> >>>> Yes please. >>>> >>>> Thanks! >>>> >>>> -- >>>> Dave Page >>>> Blog: http://pgsnake.blogspot.com >>>> Twitter: @pgsnake >>>> >>>> EnterpriseDB UK: http://www.enterprisedb.com >>>> The Enterprise PostgreSQL Company >>>> >>> >>> >> >> >> -- >> Dave Page >> Blog: http://pgsnake.blogspot.com >> Twitter: @pgsnake >> >> EnterpriseDB UK: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > > > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > -- Sent via pgadmin-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers Attachments: [application/octet-stream] RM_2257_v4.patch (16.3K, 3-RM_2257_v4.patch) download | inline diff: diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql index 759e657..f3353d6 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql @@ -1,4 +1,5 @@ -SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype +SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype, +att.attnotnull as not_null, att.atthasdef as has_default_val FROM pg_attribute att JOIN pg_type ty ON ty.oid=atttypid JOIN pg_namespace tn ON tn.oid=ty.typnamespace diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql index 7536a9c..4f1de2a 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql @@ -1,4 +1,5 @@ -SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype +SELECT att.attname as name, att.attnum as OID, format_type(ty.oid,NULL) AS datatype, +att.attnotnull as not_null, att.atthasdef as has_default_val FROM pg_attribute att JOIN pg_type ty ON ty.oid=atttypid JOIN pg_namespace tn ON tn.oid=ty.typnamespace diff --git a/web/pgadmin/static/css/pgadmin.css b/web/pgadmin/static/css/pgadmin.css index 6508feb..1a2d443 100644 --- a/web/pgadmin/static/css/pgadmin.css +++ b/web/pgadmin/static/css/pgadmin.css @@ -780,4 +780,7 @@ lgg-el-container[el=md] .pg-el-lg-8, } .user-language select{ height: 25px !important; +} +.grey_color { + color: #999999; } \ No newline at end of file diff --git a/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js b/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js index cdfba4d..faad731 100644 --- a/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js +++ b/web/pgadmin/static/js/slickgrid/slick.pgadmin.editors.js @@ -110,7 +110,12 @@ // When text editor opens this.loadValue = function (item) { - if (item[args.column.pos] === "") { + var col = args.column; + + if (_.isUndefined(item[args.column.pos]) && col.has_default_val) { + $input.val(""); + } + else if (item[args.column.pos] === "") { $input.val("''"); } else { @@ -145,7 +150,14 @@ }; this.isValueChanged = function () { - return (!($input.val() == "" && defaultValue == null)) && ($input.val() != defaultValue); + // Use _.isNull(value) for comparison for null instead of + // defaultValue == null, because it returns true for undefined value. + if ($input.val() == "" && _.isUndefined(defaultValue)) { + return false; + } else { + return (!($input.val() == "" && _.isNull(defaultValue))) && + ($input.val() != defaultValue); + } }; this.validate = function () { @@ -253,7 +265,7 @@ this.loadValue = function (item) { var data = defaultValue = item[args.column.pos]; - if (typeof data === "object" && !Array.isArray(data)) { + if (data && typeof data === "object" && !Array.isArray(data)) { data = JSON.stringify(data); } else if (Array.isArray(data)) { var temp = []; @@ -282,7 +294,11 @@ }; this.isValueChanged = function () { - return (!($input.val() == "" && defaultValue == null)) && ($input.val() != defaultValue); + if ($input.val() == "" && _.isUndefined(defaultValue)) { + return false; + } else { + return (!($input.val() == "" && _.isNull(defaultValue))) && ($input.val() != defaultValue); + } }; this.validate = function () { @@ -498,6 +514,12 @@ }; this.validate = function () { + if (args.column.validator) { + var validationResults = args.column.validator(this.serializeValue()); + if (!validationResults.valid) { + return validationResults; + } + } return { valid: true, msg: null @@ -837,7 +859,14 @@ }; this.isValueChanged = function () { - return (!($input.val() == "" && defaultValue == null)) && ($input.val() != defaultValue); + if ($input.val() == "" && _.isUndefined(defaultValue)) { + return false; + } else if ($input.val() == "" && defaultValue == "") { + return true; + } else { + return (!($input.val() == "" && _.isNull(defaultValue ))) && + ($input.val() != defaultValue); + } }; this.validate = function () { diff --git a/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js b/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js index 290bddd..642fc2f 100644 --- a/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js +++ b/web/pgadmin/static/js/slickgrid/slick.pgadmin.formatters.js @@ -19,8 +19,15 @@ }); function JsonFormatter(row, cell, value, columnDef, dataContext) { - if (value == null || value === "") { - return ""; + // If column has default value, set placeholder + if (_.isUndefined(value) && columnDef.has_default_val) { + return "<span class='pull-left grey_color'>[default]</span>"; + } + else if ( + (_.isUndefined(value) && columnDef.not_null) || + (_.isUndefined(value) || value === null) + ) { + return "<span class='pull-left grey_color'>[null]</span>"; } else { // Stringify only if it's json object if (typeof value === "object" && !Array.isArray(value)) { @@ -42,11 +49,15 @@ } function NumbersFormatter(row, cell, value, columnDef, dataContext) { - if (_.isUndefined(value) || value === null) { - return "<span class='pull-right'>[null]</span>"; + // If column has default value, set placeholder + if (_.isUndefined(value) && columnDef.has_default_val) { + return "<span class='pull-right grey_color'>[default]</span>"; } - else if (value === "") { - return ''; + else if ( + (_.isUndefined(value) || value === null || value === "") || + (_.isUndefined(value) && columnDef.not_null) + ) { + return "<span class='pull-right grey_color'>[null]</span>"; } else { return "<span style='float:right'>" + _.escape(value) + "</span>"; @@ -57,17 +68,30 @@ /* Checkbox has 3 states * 1) checked=true * 2) unchecked=false - * 3) indeterminate=null/'' + * 3) indeterminate=null */ - if (value == null || value === "") { - return "<span class='pull-left'>[null]</span>"; + if (_.isUndefined(value) && columnDef.has_default_val) { + return "<span class='pull-left grey_color'>[default]</span>"; + } + else if ( + (_.isUndefined(value) && columnDef.not_null) || + (value == null || value === "") + ) { + return "<span class='pull-left grey_color'>[null]</span>"; } return value ? "true" : "false"; } function TextFormatter(row, cell, value, columnDef, dataContext) { - if (_.isUndefined(value) || value === null) { - return "<span class='pull-left'>[null]</span>"; + // If column has default value, set placeholder + if (_.isUndefined(value) && columnDef.has_default_val) { + return "<span class='pull-left grey_color'>[default]</span>"; + } + else if ( + (_.isUndefined(value) && columnDef.not_null) || + (_.isUndefined(value) || _.isNull(value)) + ) { + return "<span class='pull-left grey_color'>[null]</span>"; } else { return _.escape(value); diff --git a/web/pgadmin/tools/sqleditor/__init__.py b/web/pgadmin/tools/sqleditor/__init__.py index d114988..f7466d8 100644 --- a/web/pgadmin/tools/sqleditor/__init__.py +++ b/web/pgadmin/tools/sqleditor/__init__.py @@ -440,8 +440,23 @@ def get_columns(trans_id): columns = dict() columns_info = None primary_keys = None + rset = None status, error_msg, conn, trans_obj, session_obj = check_transaction_status(trans_id) if status and conn is not None and session_obj is not None: + + ver = conn.manager.version + # Get the template path for the column + template_path = 'column/sql/#{0}#'.format(ver) + command_obj = pickle.loads(session_obj['command_obj']) + if hasattr(command_obj, 'obj_id'): + SQL = render_template("/".join([template_path, + 'nodes.sql']), + tid=command_obj.obj_id) + # rows with attribute not_null + status, rset = conn.execute_2darray(SQL) + if not status: + return internal_server_error(errormsg=rset) + # Check PK column info is available or not if 'primary_keys' in session_obj: primary_keys = session_obj['primary_keys'] @@ -449,10 +464,17 @@ def get_columns(trans_id): # Fetch column information columns_info = conn.get_column_info() if columns_info is not None: - for col in columns_info: + for key, col in enumerate(columns_info): col_type = dict() col_type['type_code'] = col['type_code'] col_type['type_name'] = None + if rset: + col_type['not_null'] = col['not_null'] = \ + rset['rows'][key]['not_null'] + + col_type['has_default_val'] = col['has_default_val'] = \ + rset['rows'][key]['has_default_val'] + columns[col['name']] = col_type # As we changed the transaction object we need to @@ -602,6 +624,7 @@ def save(trans_id): status, error_msg, conn, trans_obj, session_obj = check_transaction_status(trans_id) if status and conn is not None \ and trans_obj is not None and session_obj is not None: + setattr(trans_obj, 'columns_info', session_obj['columns_info']) # If there is no primary key found then return from the function. if len(session_obj['primary_keys']) <= 0 or len(changed_data) <= 0: diff --git a/web/pgadmin/tools/sqleditor/command.py b/web/pgadmin/tools/sqleditor/command.py index 1795155..9601467 100644 --- a/web/pgadmin/tools/sqleditor/command.py +++ b/web/pgadmin/tools/sqleditor/command.py @@ -442,6 +442,25 @@ class TableCommand(GridCommand): # For newly added rows if of_type == 'added': + + # When new rows are added, only changed columns data is + # sent from client side. But if column is not_null and has + # no_default_value, set column to blank, instead + # of not null which is set by default. + column_data = {} + column_type = {} + pk_names, primary_keys = self.get_primary_keys() + + for each_col in self.columns_info: + if ( + self.columns_info[each_col]['not_null'] and + not self.columns_info[each_col][ + 'has_default_val'] + ): + column_data[each_col] = None + column_type[each_col] =\ + self.columns_info[each_col]['type_name'] + for each_row in changed_data[of_type]: data = changed_data[of_type][each_row]['data'] # Remove our unique tracking key @@ -450,12 +469,19 @@ class TableCommand(GridCommand): data_type = set_column_names(changed_data[of_type][each_row]['data_type']) list_of_rowid.append(data.get('__temp_PK')) + # Update columns value and data type + # with columns having not_null=False and has + # no default value + column_data.update(data) + column_type.update(data_type) + sql = render_template("/".join([self.sql_path, 'insert.sql']), - data_to_be_saved=data, + data_to_be_saved=column_data, primary_keys=None, object_name=self.object_name, nsp_name=self.nsp_name, - data_type=data_type) + data_type=column_type, + pk_names=pk_names) list_of_sql.append(sql) # For updated rows diff --git a/web/pgadmin/tools/sqleditor/static/css/sqleditor.css b/web/pgadmin/tools/sqleditor/static/css/sqleditor.css index 574ef53..1d572ca 100644 --- a/web/pgadmin/tools/sqleditor/static/css/sqleditor.css +++ b/web/pgadmin/tools/sqleditor/static/css/sqleditor.css @@ -420,6 +420,10 @@ input.editor-checkbox:focus { background: #e46b6b; } +.grid-canvas .disabled_row { + background: #c1c1c1; +} + /* color the first column */ .sr .sc:first-child { background-color: #2c76b4; @@ -445,4 +449,4 @@ input.editor-checkbox:focus { .sr.ui-widget-content { border-top: 1px solid silver; -} +} \ No newline at end of file diff --git a/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js b/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js index 2062aa2..ba9dd43 100644 --- a/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js +++ b/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js @@ -557,7 +557,9 @@ define( id: c.name, pos: c.pos, field: c.name, - name: c.label + name: c.label, + not_null: c.not_null, + has_default_val: c.has_default_val }; // Get the columns width based on data type @@ -625,6 +627,12 @@ define( } } } + // Disable rows having default values + if (!_.isUndefined(self.handler.rows_to_disable) && + _.indexOf(self.handler.rows_to_disable, i) !== -1 + ) { + cssClass += ' disabled_row'; + } return {'cssClasses': cssClass}; } @@ -702,6 +710,14 @@ define( // This will be used to collect primary key for that row grid.onBeforeEditCell.subscribe(function (e, args) { var before_data = args.item; + + // If newly added row is saved but grid is not refreshed, + // then disable cell editing for that row + if(self.handler.rows_to_disable && + _.contains(self.handler.rows_to_disable, args.row)) { + return false; + } + if(self.handler.can_edit && before_data && '__temp_PK' in before_data) { var _pk = before_data.__temp_PK, _keys = self.handler.primary_keys, @@ -1629,6 +1645,8 @@ define( self.query_start_time = new Date(); self.rows_affected = 0; self._init_polling_flags(); + // keep track of newly added rows + self.rows_to_disable = new Array(); self.trigger( 'pgadmin-sqleditor:loading-icon:show', @@ -2077,7 +2095,9 @@ define( 'label': column_label, 'cell': col_cell, 'can_edit': self.can_edit, - 'type': type + 'type': type, + 'not_null': c.not_null, + 'has_default_val': c.has_default_val }; columns.push(col); }); @@ -2320,6 +2340,10 @@ define( grid.setSelectedRows([]); } + // Add last row(new row) to keep track of it + if (is_added) { + self.rows_to_disable.push(grid.getDataLength()-1); + } // Reset data store self.data_store = { 'added': {}, ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-12 10:11 Dave Page <[email protected]> parent: Surinder Kumar <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Dave Page @ 2017-05-12 10:11 UTC (permalink / raw) To: Surinder Kumar <[email protected]>; +Cc: pgadmin-hackers Hi, Couple of comments: - Can we improve the speed of this test? Perhaps by adding multiple rows to the table at once, then checking the result after a single save/refresh? We need to keep the feature tests as fast as possible to ensure they remain practical to run. - I get the following failure under Python 2. It passes under Python 3 as you might imagine given the assertion error. Thanks! 2017-05-12 11:00:00,860:ERROR:STDERR:================================== ==================================== 2017-05-12 11:00:00,861:ERROR:STDERR:FAIL: runTest (pgadmin.feature_tests.view_data_dml_queries.CheckForViewDataTest) 2017-05-12 11:00:00,861:ERROR:STDERR:Validate Insert, Update operations in View data with given test data 2017-05-12 11:00:00,861:ERROR:STDERR:---------------------------------- ------------------------------------ 2017-05-12 11:00:00,861:ERROR:STDERR:Traceback (most recent call last): 2017-05-12 11:00:00,861:ERROR:STDERR: File "/Users/dpage/git/pgadmin4/ web/pgadmin/feature_tests/view_data_dml_queries.py", line 160, in runTest 2017-05-12 11:00:00,861:ERROR:STDERR: self._update_row_in_table(key) 2017-05-12 11:00:00,861:ERROR:STDERR: File "/Users/dpage/git/pgadmin4/ web/pgadmin/feature_tests/view_data_dml_queries.py", line 432, in _update_row_in_table 2017-05-12 11:00:00,861:ERROR:STDERR: self._verify_update_data(table, row) 2017-05-12 11:00:00,861:ERROR:STDERR: File "/Users/dpage/git/pgadmin4/ web/pgadmin/feature_tests/view_data_dml_queries.py", line 477, in _verify_update_data 2017-05-12 11:00:00,861:ERROR:STDERR: self.assertEquals(cell2, test_verify_data['data_default_nulls']) 2017-05-12 11:00:00,861:ERROR:STDERR:AssertionError: "''" != u'' 2017-05-12 11:00:00,861:ERROR:STDERR:---------------------------------- ------------------------------------ 2017-05-12 11:00:00,861:ERROR:STDERR:Ran 6 tests in 208.850s 2017-05-12 11:00:00,861:ERROR:STDERR:FAILED 2017-05-12 11:00:00,861:ERROR:STDERR: (failures=1) On Wed, May 10, 2017 at 3:02 PM, Surinder Kumar < [email protected]> wrote: > Hi Dave, > > Please find attached patch for Feature test cases for RM_2257 > > *Implementation detail:* > > - Added a test_data.json file which contains Insert and Update test > related input data > > - First of all, we create three tables such as > a) defaults_text > b) defaults_boolean > c) defaults_number > d) defaults_json > These tables has columns with different constraints (default value, > not_null etc) to test with various input test data. > > - Test cases for insert are executed first and then test cases for update. > > Please review the patch. > > > On Wed, May 10, 2017 at 2:22 PM, Surinder Kumar < > [email protected]> wrote: > >> Hi Dave, >> >> Please find attached patch for RM only. >> >> *Changes:* >> >> - All formatters now handles both [null] and [default] values >> >> - the cell values are validated on server side as in pgAdmin3. >> >> - added light grey color for cells with [null] and [default] >> placeholders. >> >> On Wed, May 10, 2017 at 2:12 PM, Dave Page <[email protected]> wrote: >> >>> >>> >>> On Wed, May 10, 2017 at 9:39 AM, Surinder Kumar < >>> [email protected]> wrote: >>> >>>> Hi Dave, >>>> >>>> On Wed, May 10, 2017 at 2:06 PM, Dave Page <[email protected]> wrote: >>>> >>>>> Any chance we can get this wrapped up today Surinder? >>>>> >>>> I have fixed RM case, I am currently writing its feature test cases >>>> which is taking some time. >>>> Should I send patch for RM case only? I will try to complete test >>>> cases by today eod. >>>> >>> >>> Yes please. >>> >>> Thanks! >>> >>> -- >>> Dave Page >>> Blog: http://pgsnake.blogspot.com >>> Twitter: @pgsnake >>> >>> EnterpriseDB UK: http://www.enterprisedb.com >>> The Enterprise PostgreSQL Company >>> >> >> > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values @ 2017-05-12 10:39 Surinder Kumar <[email protected]> parent: Dave Page <[email protected]> 0 siblings, 0 replies; 25+ messages in thread From: Surinder Kumar @ 2017-05-12 10:39 UTC (permalink / raw) To: Dave Page <[email protected]>; +Cc: pgadmin-hackers Hi Dave, On Fri, May 12, 2017 at 3:41 PM, Dave Page <[email protected]> wrote: > Hi, > > Couple of comments: > > - Can we improve the speed of this test? Perhaps by adding multiple rows > to the table at once, then checking the result after a single save/refresh? > We need to keep the feature tests as fast as possible to ensure they remain > practical to run. > Sure. I will do this and send updated patch. > > - I get the following failure under Python 2. It passes under Python 3 as > you might imagine given the assertion error. > I will check. > > Thanks! > > 2017-05-12 11:00:00,860:ERROR:STDERR:================================== > ==================================== > 2017-05-12 11:00:00,861:ERROR:STDERR:FAIL: runTest > (pgadmin.feature_tests.view_data_dml_queries.CheckForViewDataTest) > 2017-05-12 11:00:00,861:ERROR:STDERR:Validate Insert, Update operations > in View data with given test data > 2017-05-12 11:00:00,861:ERROR:STDERR:---------------------------------- > ------------------------------------ > 2017-05-12 11:00:00,861:ERROR:STDERR:Traceback (most recent call last): > 2017-05-12 11:00:00,861:ERROR:STDERR: File "/Users/dpage/git/pgadmin4/web > /pgadmin/feature_tests/view_data_dml_queries.py", line 160, in runTest > 2017-05-12 11:00:00,861:ERROR:STDERR: self._update_row_in_table(key) > 2017-05-12 11:00:00,861:ERROR:STDERR: File "/Users/dpage/git/pgadmin4/web > /pgadmin/feature_tests/view_data_dml_queries.py", line 432, in > _update_row_in_table > 2017-05-12 11:00:00,861:ERROR:STDERR: self._verify_update_data(table, > row) > 2017-05-12 11:00:00,861:ERROR:STDERR: File "/Users/dpage/git/pgadmin4/web > /pgadmin/feature_tests/view_data_dml_queries.py", line 477, in > _verify_update_data > 2017-05-12 11:00:00,861:ERROR:STDERR: self.assertEquals(cell2, > test_verify_data['data_default_nulls']) > 2017-05-12 11:00:00,861:ERROR:STDERR:AssertionError: "''" != u'' > 2017-05-12 11:00:00,861:ERROR:STDERR:---------------------------------- > ------------------------------------ > 2017-05-12 11:00:00,861:ERROR:STDERR:Ran 6 tests in 208.850s > 2017-05-12 11:00:00,861:ERROR:STDERR:FAILED > 2017-05-12 11:00:00,861:ERROR:STDERR: (failures=1) > On Wed, May 10, 2017 at 3:02 PM, Surinder Kumar < > [email protected]> wrote: > >> Hi Dave, >> >> Please find attached patch for Feature test cases for RM_2257 >> >> *Implementation detail:* >> >> - Added a test_data.json file which contains Insert and Update test >> related input data >> >> - First of all, we create three tables such as >> a) defaults_text >> b) defaults_boolean >> c) defaults_number >> d) defaults_json >> These tables has columns with different constraints (default value, >> not_null etc) to test with various input test data. >> >> - Test cases for insert are executed first and then test cases for update. >> >> Please review the patch. >> >> >> On Wed, May 10, 2017 at 2:22 PM, Surinder Kumar < >> [email protected]> wrote: >> >>> Hi Dave, >>> >>> Please find attached patch for RM only. >>> >>> *Changes:* >>> >>> - All formatters now handles both [null] and [default] values >>> >>> - the cell values are validated on server side as in pgAdmin3. >>> >>> - added light grey color for cells with [null] and [default] >>> placeholders. >>> >>> On Wed, May 10, 2017 at 2:12 PM, Dave Page <[email protected]> wrote: >>> >>>> >>>> >>>> On Wed, May 10, 2017 at 9:39 AM, Surinder Kumar < >>>> [email protected]> wrote: >>>> >>>>> Hi Dave, >>>>> >>>>> On Wed, May 10, 2017 at 2:06 PM, Dave Page <[email protected]> wrote: >>>>> >>>>>> Any chance we can get this wrapped up today Surinder? >>>>>> >>>>> I have fixed RM case, I am currently writing its feature test cases >>>>> which is taking some time. >>>>> Should I send patch for RM case only? I will try to complete test >>>>> cases by today eod. >>>>> >>>> >>>> Yes please. >>>> >>>> Thanks! >>>> >>>> -- >>>> Dave Page >>>> Blog: http://pgsnake.blogspot.com >>>> Twitter: @pgsnake >>>> >>>> EnterpriseDB UK: http://www.enterprisedb.com >>>> The Enterprise PostgreSQL Company >>>> >>> >>> >> > > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > ^ permalink raw reply [nested|flat] 25+ messages in thread
end of thread, other threads:[~2017-05-12 10:39 UTC | newest] Thread overview: 25+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2017-04-01 11:45 [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values Surinder Kumar <[email protected]> 2017-04-04 16:37 ` Matthew Kleiman <[email protected]> 2017-04-07 08:53 ` Dave Page <[email protected]> 2017-04-07 08:51 ` Dave Page <[email protected]> 2017-04-28 09:19 ` Surinder Kumar <[email protected]> 2017-05-02 11:51 ` Dave Page <[email protected]> 2017-05-02 12:27 ` Surinder Kumar <[email protected]> 2017-05-05 11:52 ` Surinder Kumar <[email protected]> 2017-05-08 09:58 ` Dave Page <[email protected]> 2017-05-08 10:13 ` Surinder Kumar <[email protected]> 2017-05-08 10:21 ` Dave Page <[email protected]> 2017-05-08 10:51 ` Surinder Kumar <[email protected]> 2017-05-08 11:07 ` Dave Page <[email protected]> 2017-05-09 10:03 ` Surinder Kumar <[email protected]> 2017-05-09 10:29 ` Dave Page <[email protected]> 2017-05-10 08:36 ` Dave Page <[email protected]> 2017-05-10 08:39 ` Surinder Kumar <[email protected]> 2017-05-10 08:42 ` Dave Page <[email protected]> 2017-05-10 08:52 ` Surinder Kumar <[email protected]> 2017-05-10 14:02 ` Surinder Kumar <[email protected]> 2017-05-12 10:11 ` Dave Page <[email protected]> 2017-05-12 10:39 ` Surinder Kumar <[email protected]> 2017-05-11 08:58 ` Dave Page <[email protected]> 2017-05-11 08:59 ` Dave Page <[email protected]> 2017-05-12 05:39 ` Surinder Kumar <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox