Re: [pgAdmin4][Patch][RM2257]: Query tool - Insert row doesn't use default values

Hi

On Fri, May 5, 2017 at 12:52 PM, Surinder Kumar <
surinder(dot)kumar(at)enterprisedb(dot)com> 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 <
> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>
>> Hi Dave,
>>
>> On Tue, May 2, 2017 at 5:21 PM, Dave Page <dpage(at)pgadmin(dot)org> 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 <
>>> surinder(dot)kumar(at)enterprisedb(dot)com> 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 <dpage(at)pgadmin(dot)org> wrote:
>>>>
>>>>> Hi
>>>>>
>>>>> On Sat, Apr 1, 2017 at 12:45 PM, Surinder Kumar
>>>>> <surinder(dot)kumar(at)enterprisedb(dot)com> 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